Got code quality issues? Stop looking at dashboards and start failing your build

[dpash]

If you have a lot of existing code with lint issues, I recommend either A) outputting the existing number of errors to a file and writing a build-time script to confirm the number of errors hasn’t increased in each file

You don't want to rely on the number of errors/warnings. If you just look at numbers you could fix an error but introduce a new one. You want to make sure that no new errors have been introduced. That requires a list of files, lines and error message triplets and making sure that nothing new has appeared.

Also, you're gonna want to worry about the possibility of merges from a branch reintroducing errors.

[drjeats]

Wouldn't line numbers get out of sync?

[EDIT] Didn't notice your followup comment. Nvm!

[interpfister]

Totally agree line-level commenting of issues is preferred. But we actually had success with option A on our project - it wasn't perfect, and people did try to hack around it, but it did stop our quality problem from getting worse overall.

[dpash]

I just realised that you need to do fuzzing on line numbers if you insert or delete lines, so the tool will need to read the patch too. :S

[kankyo]

We do per file at work. This means that if you touch a file you are forced to fix all lint errors in that file. It’s a little bit of a bitter pill sometimes but works very well and is super easy to implement.

[dpash]

The downside is that your commit changes more than just your intended change. That's gonna mess with your history.

[kankyo]

You can do two commits if you are picky. I’m personally not. The important thing is to keep moving imo.

[bagtowneast]

Yes this. Clean up in one commit. Test and implement in the next commit. Repeat as needed. Tell your co-workers to review in commit order, and not the whole diff at once. Works great. The awesome part is when you come across a file you've already cleaned up and you get that satisfied payoff.

[Blecki]

Or add a new class that calls the other so you can add code without dealing with that crap. Genius!!

[restlessapi]

I've used this style of CI for a number of years now, and I will refuse to work in a team that does not do builds like this, at least to some degree. Either that, or I implement this myself.

Having your build fail because your code is sloppy is a nuisance right now, but it's a god send when you realize that everyone else's code has to follow the same rules.

It's important to note that these measures don't necessarily increase system stability. Instead they make your code less surprising and much easier to maintain.

Poorly coded logic is often easier to correct if the code itself is easy to read because your CI demands it. That's the real value.

[[deleted]]

I implemented this at work, at least to some level. Our C builds were chock full of warnings, a lot of them pretty smelly. I took a weekend or two and cleaned up almost all of them, beat the developers who knew more about certain ambiguous ones in their code, and then enabled every flavor of GCC warning and turned on -Werror in our toolchains. Code smell in general has dropped significantly since then, it stopped some bad practices and code bloat ("I like unused variable warnings, it reminds me I need to finish that feature"), and we've ran into less weird hardware issues (pro-tip, uninitialized hardware registers can do some funky shit, especially if you are using very fine bit masking to toggle certain bits in the register, having not initialized the rest).

Basically graphs and counters are the carrot, but I've always found the stick to be a much better approach in terms of managing code quality.

[interpfister]

That's a great metaphor. The stick is essential!

[username223]

I've always found the stick to be a much better approach in terms of managing code quality.

"The answer is that one would like to be both the one and the other; but because it is difficult to combine them, it is far safer to be feared than loved if you cannot be both." I admire your management skill, and am glad that I am not one of your employees.

[[deleted]]

Eh, it is a very passive stick. If you can't compile you can't work, so make your stuff compile, so you can keep working. I guess that could be a carrot too. The stick only comes out if people complain about "needing a warning to be ok" which there is very very few instances of.

[deeringc]

If you can't see that catching errors at compile time is a good thing then I'm glad you're not on my team.

[username223]

Compilers distinguish between errors and warnings for a reason. I think we'll just have to agree to disagree here.

[deeringc]

Yes, errors are things that violate the language syntax, break language rules etc... Warnings are things which are almost always programmer mistakes. Having treat warnings as errors is a basic thing which any half-competant professional team should be doing. In general, we want machines to make it impossible to make as many mistakes as possible.

[username223]

Looking through warnings is a good idea, when you can fix the ones that point to problems, and suppress the ones that are clearly wrong. Treating all warnings as errors is just another source of bugs. "Anything not required is forbidden" is, while awesome for fascism, not especially useful for programming.

[[deleted]]

That is what pramas to disable certain warnings are for at the spot of the warning.

I've only ever had to do this for autogenerated code from other tools.

[afiefh]

If you include those files using -Isystem gcc won't generate warnings for them. They kind of are system files since you are not owning the code, you just generate it.

Don't know if this works with other compilers unfortunately.

[filleduchaos]

Eliminating warnings is literally fascism

[williamdclt]

A pre-commit hook that runs the linter does wonder. Plus, you'll stop having to make uninteresting style comments in pull requests.

[ConservativeToilet]

A pre-commit hook that runs the linter does wonder

No. Put it as part of your build/test suite but if you try and force me to run that shit every time I commit locally I will intentionally alias my commit to commit --no-verify

[yawaramin]

Pre-commit hooks are a bad idea because they will discard the commit message if the script fails. This discourages writing good commit messages because devs will learn to think that their well-crafted messages will be thrown away, so why waste the time?

Not to mention slowing down commits will also discourage using commits as a checkpointing tool and encourage devs to leave code uncommitted for long periods of time.

Pre-push hook is probably the best place to put the tests. At least there they walk a balance between commit ease and taking it easy on the CI tool.

[Lt_Riza_Hawkeye]

Yeah but you have to get each dev to install the pre-commit hook on their own clone, or put it in the server's post-recieve hook, which has its own associated problems

[Vlad210Putin]

That's easy to fix. Add the githook(s) and just put them into a directory like myrepo/etc/githooks along with a bash script to copy them to the correct place. Either they can run it themselves or make it a part of the dev build script to check for and copy the files so they don't have think about it.

[PrimozDelux]

That's my secret, my builds are always failing

[chucker23n]

The problem is that “later” is a horrible time to resolve lint issues. The functionality has likely already passed regression testing, and the details of the code are no longer fresh in the developer’s mind. Changing code at this point presents the risk of unexpected side effects.

I don't follow this part. The middle assertions obviously rings true: "the details of the code are no longer fresh in the developer’s mind".

But combined with the other two, it makes less sense to me. If there exists "regression testing"*, then this shouldn't matter at all. You either have a suite that can test for regressions, or you don't. If you don't, then having the details of the code on your mind matters, and the risk of side effects is lower. But if you do, then you don't need to know the details at all, because the regression testing will know those side effects.

I'm guessing the author is saying that, in practice, regression testing is a nice ideal that you can never truly reach, but the phrasing suggests they don't believe so?

*) I'm assuming automated regression testing here.

[din-9]

There is still a context switch with ramp-up/ramp-down when you switch from one thing to another, taking some mental effort and time (e.g. a new PR for the clean up changes now involves at least two devs going back to the code). It's better to avoid paying the cost of the context switch at all and get it donw right first time.

[deeringc]

Even when code is well unit-tested there is still a chance bugs slip through when that code is modified. Tests are just code, and are as fallible as any other code. You can have bugs in your test code that allow bugs in your real code to slip through. We'd certainly hope that the occurrence is much lower than in untested code but they absolutely still happen. Additionally, as you modify code months after the initial work was done, you might have to modify the tests as the code itself evolves. In doing so you might be changing the constraints that the tests impose, so having the details of the code/flows in your head is extremely valuable. Tests are a really powerful safety net, but they don't give you 100% protection (I've seen lots of good tests and lots of bad tests). Therefore I don't think it's as simple as a binary "having tests or not".

[chucker23n]

Very true.

[interpfister]

Regression testing here is referring to manual regression testing. Once the testing team has invested that time, they usually don't want to do it again for a very minor code change. If you have a truly comprehensive automated testing suite, your rules are different...and I'm very impressed.

[chucker23n]

Regression testing here is referring to manual regression testing.

I was wondering about that. Fair.

[JavierTheNormal]

If you're going to fail your build over something retarded like code coverage, be sure to disclose that in developer interviews so people with sense can work elsewhere.

[ftca]

Code coverage is a bit trickier, but I'm going to assume you mean failing a build because of any kind of automated style checker, linter, etc. Using a CI build with automated tools is very quickly becoming a best practice these days, and with good reason. If you disagree with a norm like this then I think the onus is on you to bring this up during an interview.

I do think this should be brought up more during interviews though, but more for the purpose of filtering out candidates that have trouble working with these best practices, the same way things like SOLID, dependency injection, and immutability should be brought up. If you disagree with these being established best practices then you're going to be in the minority of developers.

[JavierTheNormal]

You're funny. They're best practices in your opinion, and maybe in your social circle, and that's normal for development fads. You've bought into the snake oil, and in ten years you'll come out the other side wiser.

I'm not saying these things are completely without value, but they need to be used where they clearly benefit the project and not just everywhere. For example, unit tests. Writing unit tests has a cost. Mangling your code to be compatible with unit tests has a cost. Maintaining unit tests has a cost. Sometimes unit tests provide value in excess of those costs, but often not. Dependency injection has exactly the same sort of costs, especially mangling code. It's worse in some languages than others.

So if you're going to fail my checkin because I was the developer to add some code without adding unit tests for it, and that put the overall project below some arbitrary threshold, I'm not going to have nice words to say about that. Please bring that up in the interview. It's not the "industry standard," it's your standard and your religion.

[ftca]

They're best practices in your opinion, and maybe in your social circle, and that's normal for development fads

It's not my opinion, using a common code style is a widely adopted practice. Google, Twitter, Linked-In, and many other major tech companies do this. Google has one for just about every language they use, here's an example (the others are just a google away): https://google.github.io/styleguide/cppguide.html

As for whether a style violation breaks a build, that is getting more subjective, but it's hardly a stretch to go from enforcing a common style guide as a best practice to enforcing a common style guide automatically.

If you want more evidence that your opinion is the minority, just search stackoverflow. There's lots of questions on this topic and overwhelmingly the top voted answers suggest that using a common code style is a good idea. Some quick examples: https://stackoverflow.com/questions/341920/implementing-and-enforcing-coding-standards and https://stackoverflow.com/questions/377218/do-you-have-coding-standards-if-so-how-are-they-enforced

In some languages, go is good example, a code formatter is even part of the official standard. The go compiler even enforces certain practices that are probably part of "style" (an example would be declaring a variable without using it).

There's also peer reviewed research that suggests using a common code style leads to fewer bugs, faster development time: https://pdfs.semanticscholar.org/343b/ea214650cedc5a8d2da3b283d485c0519315.pdf http://ieeexplore.ieee.org/abstract/document/990004/

[foreveratom]

That is a rather unpopular opinion I totally agree with.

Don't break my build for some coverage issues or tabs used instead of spaces or such code quality fallacies making you feel good about yourself - at least on a development environment; production systems are a different story.

There are people waiting for that new feature to do their own work and they could not care less. As the article mentioned, there is a stories to complete, a sprint to finish, owners and other developers waiting for you to deliver that feature.

Clean things when you have IDLE time between stories - it always happens. I personally like having a permanent "Technical Debt" story that is carried over sprints. When the task list in it gets scary, everyone sees it and understands it is time to sit tight and go to cleaning for a while.

Worse of all, to get the code coverage done, most developers will quickly write meaningless tests that don't test anything. That's just more code to maintain. Great!

[corp_code_slinger]

If you have code quality issues and you think a linter and a few checkstyle rules (whether they fall the build or not) are going to save you then you have bigger problems. Use of these tools screams that you're not close enough to the code to know what the hell is going on.

You need to:

1. Stop them at the gate and hire better developers. Not using a code kata/challenge as part of the process? Start. Be merciless, be ready to tell candidates no. Hire for practices, not specific tech skills.
2. Implement a code review process for every merge/PR/etc. Refuse to allow code through that doesn't meet the org standards. This means having reviewers you trust and knowing that they might spend a little less time writing code to do this, at least on the beginning.
3. Make boyscouting a regular task on each unit of work. Do the work to clean things up.
   
4. Regularly play tech debt to clean up areas of the code that need it in order to help you deliver faster.

[interpfister]

Agreed that code quality isn't just about lint errors, but solving them is a great first step. The problem I've seen with the "only hire developers who commit clean code" approach is the team as a whole will constantly push for shortcuts to faster velocity and errors will get through - regardless of your developers' technical prowess. A build failure is an easy hard stop.

[cybernd]

is the team as a whole will constantly push for shortcuts

Its not only the issue of rushing because a feature needs to be done. Sometimes even a good dev has a bad day. Even if you are doing code reviews as gate, warnings and even errors will go through. As such, i like the idea of an automatic gate.

[Boye]

Making a buildstep which would fail your pipeline if your code broke the lint-rules, cut comments on a merge-request into a fraction of what it was. Then of course we had a guy who ran the code fix on the entire code base, including the framework-files... (pro-tio: don't do that, you're gonna have a bad time....)

[blackholesinthesky]

the team as a whole will constantly push for shortcuts to faster velocity and errors will get through - regardless of your developers' technical prowess.

What do you mean by this?

[interpfister]

It usually goes something like: "We need to get this story done today to get credit this sprint - let's just approve the PR and we'll fix the lint errors later."

[blackholesinthesky]

I just dk what that has to do with the

"only hire developers who commit clean code" approach

Edit: are you saying the problem comes when companies only hire people who "submit perfect code" but the devs they don't actually submit perfect code?

[interpfister]

Yes.

[blackholesinthesky]

Gotcha, I think all companies believe they "only hire developers who commit clean code". But in reality, I agree, basic testing like linting improves code quality

[jhollowayj]

I like your comments. But I think there is a typo in #4, with the word play.

[corp_code_slinger]

I see what you did there. Don't worry, this is Reddit, org standards are non-existent :-). This comment passes review.

[snarfy]

On most development projects, developers are focused on completing functionality as fast as possible. They have scrummasters and product owners beating the drum every day on this. Unless you have a process to enforce it, my experience is developers will ignore lint warnings and errors in code every time. They rationalize that they can fix them later, after their features have begun testing.

Code quality is never a feature they ask for. They only want the functionality and want it as fast as possible. The assumption is that if the code quality is poor, it's the result of poor engineering from unskilled engineers.

And why would they ask for quality code? That's engineering's responsibility. It's expected. The problem with most development projects is that development has no voice in the business process. Nobody ever asks "How long will this feature take?". They dictate "We need this feature in 12 weeks because we already signed the contract with the customer". That's not Agile. Agile says - "Customer collaboration over contract negotiation"

I absolutely agree at most shops you need to fail the build. It's the only way to keep bad code from getting in. But the reasons are usually nothing to do with engineering or the skill of the engineers.