On code review

Code review has become a standard programming practice, for open source projects, but also for many companies that practice it in house. While this practice has proven to be very valuable, many developers struggle with it. So here are some tips and tricks that will help you work more efficiently when code review is involved.

There are (at least) 2 persons involved in code review. Someone that I’ll call a contributor, who want a piece of code to be merged, and a reviewer, who try to confirm that this piece of code meets quality standards and won’t introduce too much complexity compared to the value it provides. Making the whole exercise efficient requires each party to understand and empathize with the other and act accordingly.

Code Style

We all know you have your preferences. Egyptian braces, tab vs spaces and so on. But projects already have a style, and you should use it. The human brain is very good at pattern recognition, and to get the best out of it, it is best if the same thing always looks the same. As a result, having a consistent code style across the project is more important than which style it is.

As a contributor, always make sure to respect the style used in the project you are contributing to. If you don’t, the only thing you’ll achieve is wasting the reviewer’s time as they will ask you to fit the style. In addition, you’ll demonstrate to the reviewer that you lack attention to details, which raises a red flag on your pull request.

As a reviewer, you may want to make following the style of the project easier. Choose a commonly used style, and if possible, provide a code formater that will make sure the style is respected. For instance, LLVM provides configuration for clang-format which will help contributors respect the style.

Tests

Most projects come with a test suite. While not foolproof, it is a very good tool to avoid introducing regression in the codebase and make sure that everything works as intended. Some pieces of code are easier or less easy to test automatically. For instance, a compiler can have an extensive test suite that checks the generated code is what is expected, while GUI is typically very difficult to automatically test.

As a contributor, makes sure you provide tests for the code you want to add. If this is a bug fix for instance, make sure to add a test that demonstrates that the bug is not there anymore. If part of your code is not automatically tested, make sure you explain why and explain what you did on your side to convince yourself the code was correct. Make sure degenerate cases are covered. if you made a sort function, for instance, what happens when the collection passed to it is empty? Has just one element? Already sorted? Sorted in reverse order? What if the comparison function throws an exception? If this is about floating point, put some NaN and infinity in there. Do not test just the happy case. Not only does this make for better tests, but this is also a way to convey to the reviewer that you put some thought into this code and it can be trusted to work as advertised. ABSOLUTELY DO NOT EVER EVER SUBMIT A PATCH THAT BREAKS THE TEST SUITE! You’d be telling the reviewer you do not test your code properly which is a glow-in-the-dark red flag.

As a reviewer, make sure running the test suite is easy. If you have the resources, add continuous integration to check pull requests against the test suite. This way, you won’t waste time on pull requests that do not pass the tests. Travis CI provides services for open source projects to do so. Also make sure adding new tests in the test suite is easy.

Hacks

Sometime, one has to rely on doubtful engineering tactics to get the job done. On the other hand, this builds technical debt and reviewers will only accept these if there is a very good reason to introduce them. For instance, because the alternative would be to do some important refactoring and the addition is valuable enough to not wait for it. Or because the “right” fix depends on some 3rd party project over which the contributor or the reviewer have no control. Sometime, hacks are necessary.

As a contributor, make sure you try to avoid hacks. There are many book on good software engineering practices, one of my favorites is The Pragmatic Programmer. When introducing some hacks, make sure you add comments to the code explaining why this hack is necessary. If you don’t, the reviewer may ask you about alternative approaches you already considered and ruled out, which is a waste of everybody’s time. The reviewer will also have to question your abilities when it comes to software design.

As a reviewer, you are more experienced with the project. When you see a hack in a patch, make sure you propose alternatives that a higher level understanding of the project gives you. A hacks needs to be either documented or eliminated.

Opportunity cost

The reviewer is often himself a contributor to the project. When spending time doing review, the reviewer will not spend time developing. In addition there are mental costs associated with a context switch.

As a contributor, you need to understand that the problem you are trying to solve is probably not the #1 priority of the reviewer. Writing the code is not what takes time when programming (if you are not convinced, film yourself). Coming up with a good design, often by trial and error, considering edge cases and various failure scenarios, testing that everything works as expected. At the end, if the reviewer needs to go through all of this him/herself, you are wasting his/her time. Make sure that you provide tests, document hacks in the code, and explain both design decisions and maybe the most important part: what the code is for and why it is valuable. Understand that every time the reviewer needs to question something, the trust level goes down and the investment required from the reviewer goes up.

As a reviewer, make sure you don’t get overwhelmed by reviews. Both for you and for the project. Sadly, even if a contributor can provide pull requests that require less of your time, including context switches, that’s not enough. Such a contributor would effectively take control over the direction of the project, deciding on its priorities. If you have too many contributors and can’t keep up with the reviews, it is time to add more reviewers to your project. Regular contributors that showed good understanding of the project should be your first candidates. However if one contributor starts taking a lot of your time, you must act. Make sure the contributor understands what is expected of him/her and that his/her contributions improve. If not, you’ll have to take more drastic actions, even maybe “fire” the contributor.

Open source versus in house development

While code review is valuable for both, the dynamic is a bit different as the contributor is different. When in house, you know the contributor will stick around. If you need something from him/her, you can go and ask. If things need to escalate, you can talk to a manager. On the other hand, in the open, you don’t know if the contributor will stick around and you have essentially no leverage.

As a contributor, understand that the bar will be higher for open source. It will be very difficult to cut corners and promise to clean up later because you need that thing right now. After a while, if you are a regular contributor, you can build enough trust to pull that off, but don’t expect it early on. You are essentially starting from zero and have a reputation to build.

As a reviewer, make sure to not be overly cautious when in house. Moving fast has its advantage and if a contributor refuses to do the promised cleanup, you can escalate. On the other hand, in open source, you are comparatively powerless. Essentially, you can ask nicely, refuse to merge, be an asshole or use the ban hammer. You have the stick but no carrot. If you ever wondered why Linus Torvald is such an asshole, you got it right there.

Keep it focused

It is fairly common that some addition to a project requires touching several subsystems. However, the person that is best able to review the change to subsystem A may not be the same as the one best able to review changes to subsystem B. It ends up creating situations where nobody is quite confident enough to merge your change, or ends up in very complex discussions mixing several topics, that are both hard to follow and unlikely to have a good signal to noise ratio.

As a contributor, make sure to do separate pull requests for each subsystem and make the people in charge aware of it. In the pull request description, make sure you explain the larger picture and link to the other pull requests, so the reviewer can understand how the change in his/her subsystem will fit in the larger picture.

As a reviewer, if a pull request with mixed concerns comes to you, ask the contributor to split it up. You can suggest what the important areas to split up are, and provide useful point of contacts to get good feedback in each area.

Conclusion

Congratulations, you’ve reached the end of this article, in a world of constant distraction that’s quite a feat. There is much more to say about code review, but hopefully this will be a source of useful insight into what it is like on the other side of the fence.

I’d like to end on a general piece of advice that does not fit in the above categories: make sure the other party is aware of your progress. You won’t get your code reviewed if the reviewer doesn’t know you updated it. If you are using Github for instance, make sure you comment in the pull request when you update it as the reviewer won’t get a notification otherwise.

Good luck, and happy coding!

Leave a Reply

Your email address will not be published. Required fields are marked *