/home/roman/stdout

Making code reviews more pleasant using linters

Sep 06, 2017

It’s pretty obvious that code reviews are a very important part of software deveopment. My experience with the process is still somewhat limited, but there are some things that I can already reflect on. Specifically, I want to talk about how useful static checkers can be in making code review process better.

Background

MetaBrainz was the first project where I had to get my code reviewed before it could be merged and deployed. At first I thought that it would be too time consuming to get my changes through and that it will slow down the development process. My opinion has changed pretty quickly and I realized how useful code reviews can actually be.

Initially, my exposure to the code review process outside of MetaBrainz was pretty limited. I had other things to focus on and didn’t have to review any contributions myself. This has changed over time since we started getting patches from other people to the CritiqueBrainz codebase and other projects that I was involved in. My views on what’s important have changed slightly over time. It can be difficult to judge how useful your own reviews are without good feedback on the process. When I did actually get some feedback, it was mostly negative. People would come out and say that my suggestions:

  • don’t actually improve the code quality
  • are pointless
  • are holding them up

etc.

Code style and quality

xkcd #1513

The first point could be attributed to the fact that different people care about different things. For example, I care a lot about code style. I prefer code that I read and write to be easy to interpret for humans. There are many things that can be done to achieve that, so that’s where a lot of my focus went towards. This wasn’t the only thing I paid attention to during code reviews, but it was a pretty significant part. There were a couple of issues with this:

  • I’d rather avoid having to point of style issues in every code review
  • some people care much less about these things, so from their point of view it’s an even more significant waste of time

The first problem has been almost completely solved by:

  1. documenting common issues in the contribution guidelines
  2. automating checking using CI tools1 and static analysis tools2

In projects where these improvements were integrated, I’ve noticed myself focusing much less on basic code style and quality issues. Contributors can already get feedback about that from the CI system, so more time can be spent on parts which these tools can’t help with. This significantly saves everyone’s time

Usefulness and time consumption

As I mentioned, some people were less excited about the idea of making our code base look good, so most of the feedback about that has been taken negatively by them. I’m still of the opinion that this is an important part, especially in open source projects where a lot of people can be involved. A lot of organizations publish their code style guidelines3, there are plenty of books written about this topic4, so that’s not something I just came up with.

Summary

I believe that code review process is much better when as much checking as possible is automated. Developers should be able to quickly run static checking on the codebase to see if there is anything to fix before other developers look at the changes. This saves time and makes it easier to focus on other, perhaps, more fundamental improvements.

The next step would be automation of the formatting process. This is already done in the Go programming language toolkit. It has a gofmt tool which automatically formats code and makes it look the same in code bases which use it, which is pretty much all of them. The rest of common issues are more related to the actual functionality of the code and more general implementation patterns.

It’s also important to pay attention to people’s comments during code review. If they point out an issue then they probably care enough and think that it’s important to spend valuable time to provide feedback. If you don’t think that a suggestion would actually make the code better then it’s important to discuss why.


Extra information:


  1. Jenkins with GitHub integration plugin. [return]
  2. Most of the projects I worked with were written in Python, so the tools I chose are Pylint and Flake8. [return]
  3. Examples: https://google.github.io/styleguide/, http://lars-lab.jpl.nasa.gov/JPL_Coding_Standard_C.pdf (and there are many more for different languages and from different organizations). [return]
  4. Clean Code” by Robert C. Martin, “Code Complete” by Steve McConnell [return]