Cover

Code Reviews: why and how?

Introduction
Code Reviews: why and how?

code_review
Of all the practices implemented to improve code quality, such as unit testing, continuous integration, continuous deployment, daily stand-ups, I find the most important one is doing proper code reviews.

Code reviews have a lot of advantages:

  • It’s much easier to spot problems with other people’s code than with your own
  • Knowledge of the codebase is spread through the team, thereby avoiding the pitfall where certain developers have sole knowledge of a subsystem
  • General knowledge of the team is improved as new methods or practices are visible to everyone
  • Adherence to code standards are enforced

It can be a bit awkward in the beginning, but once you have found a good flow, you’ll notice a big change in how software is developed, not only in terms of quality but also in terms of the morale and coherence of the development team.

Rules for reviewing code

For code reviews to work in your benefit, a certain set of rules need to be followed. It’s easy to just start using some software, but if you aren’t reviewing code consistently, it won’t bring you all the benefits listed above. Here are a few the rules I have experienced to positively influence the gains of code reviews as well as satisfaction among developers.

Small commits, good commit messages

Having a lot of small grained commits with decent commit messages makes it much easier to review code as everything will be explained step-by-step. It also makes it easier for the reviewer to see the thinking behind the implementation.

As an added benefit, git blame will serve as your living documentation for all your code.

Review the code, not the developer

Since you’re reviewing someone’s work, it might be tempting to critique that person. However, if this culture persists, you’ll more often than not get developers that aren’t happy anymore or who will start hiding their not-so-nice code.

This doesn’t mean you should let code pass that doesn’t live up to the standard, it just means you should critique the code instead of the person. This can be a subtle difference. Instead of saying “You didn’t follow the standard here”, which can sound accusatory (intentional or not), say “this code should be formatted differently”. Note the difference in tone.

This responsibility also falls on the developer. As a developer you have to get into the mindset that you are not your code. Even if a comment might sound accusatory, don’t take it personally and remember that it’s a comment on the code, not on you or your behavior.

Multiple reviewers

Always assign more than 1 person to review the code. That way, if there are any disagreements, it will be easily solved by a majority vote. If you assign only 1 developer, you might run into a discussion without end between the developer and the reviewer.

Have a checklist

Having a list of things to look out for, makes it easier to conduct code reviews systematically. Here’s my personal list of things to check for (in order of priority):

  • Function: does it do what it needs to do? (A good spec goes a long way here)
  • Bugs
  • Test coverage
  • Adherence to architecture and patterns
  • Duplicate code
  • Code style and formatting

Having a checklist also makes it easier for the developer to run through it before submitting the code for review.

When to review

This depends a bit on the confidence of the team and how well they work together.

In teams with low to medium confidence, I would opt for a feature-branch strategy where code is reviewed first and only then integrated into the main line. In this case, you have to make sure that code is reviewed as soon as possible, since you don’t want any branches to live for a long time only to see that your properly reviewed code can’t be integrated anymore without merge conflicts.

In teams with a high confidence level, I would opt to integrate directly into the main line and do reviews after the fact. The reason this can only be done in high confidence teams is multiple:

  • It might open you up to code reviews that are never completed
  • A developer can ignore the code review
  • Bad code can be committed

Review process

Whether you sit down together in front of the screen or have software to be able to complete your task, make sure reviewing code is as accessible as possible. The last thing you want is that developers come to see it as a chore. A fast code review process will yield more code reviews and better results.. I had very good experiences with Kiln. It does more than just code reviews, but I particularly liked their interface (YMMV).

Quick tip: Review only tests
If you have limited time and have good test coverage (or it’s enforced by your build process), you can choose to only review the tests. The reasoning is that if the tests are good, the implementation will be good as well. By reviewing the tests you will see what the public API looks like, so you’ll get a good feel about the code. Use this tip sparingly though, a full review is still useful.

Pitfalls

Apart from following the above rules, there are a few pitfalls and anti-patterns to look out for.

Gaming the system

If you don’t have complete buy-in from the whole team, this issue might come up. It happens when 2 or more devs on the team decide to approve each other’s code without properly reviewing it. They’re technically following the rules, but in practice the code is not reviewed at all. A possible remedy is to make sure that the rule of 2 or more reviewers is appended with a rule that says you have to select two different people all the time.

Review gate

This problem can manifest itself in a few ways:

  • One dev never approves of any review so code is unnecessarily blocked in the review. If this happens frequently, find out what the underlying issue is.
  • All code needs to pass past one developer before it hits main. While this can sometimes be good for a short while, it’s never a good plan to do this long-term. If really necessary, than at least that role should be switched regularly. Otherwise developers might stop caring about their code at all.
  • Complete lockdown: this happens where developers are taken away permission to touch the main line at all. If you really have big code quality issues it can be a good temporary measure, but otherwise this is the fastest way to sink your team’s morale.

Conclusion

Code reviews are, in my experience, the most valuable tool for improving software quality, distributing knowledge and enforcing a common coding standard.

If you’re just starting out with code review, keep in mind that it needs perfecting. Don’t worry when it’s not yet creating that huge change you expected. If you improve the process bit by bit and get more experienced, you’ll soon see the benefits.

Kenneth Truyers
View Comments
Next Post

Technical debt: managing code quality

Previous Post

Build 2016 announcements