Code review: Why are we doing it?

Developers across many organisations adopted code reviews as one of their core practices. Although that sounds like a very reasonable thing to do, over time the goal that led to the adoption of the practice is forgotten and the only thing left is the mindless application of the practice itself.

Why do we need code reviews?

Maintain quality. Disseminate knowledge. Avoid serious bugs in production. Maintain consistence in the design and architecture of the software. All very compelling and logical answers, no doubt, but if those are the goals, is code review the most efficient way to achieve them?

Discussing a practice like code review requires understanding its context:

• How often is it conducted?
• Is it done before or after code merges?
• How many team members are involved?
• What's the typical size of the code under review?
• Are there distributed teams involved?
• Who holds the authority to reject code changes?
• What are the consequences of rejection?
• Is the code base handled internally or is it open source?
• What criteria are used in reviews? Finding bugs, ensuring consistency, checking for architectural integrity, or compliance with non-functional requirements?

Knowing the answers to these questions are extremely important in order to decide if code review (and how it is done) is actually the best way to achieve our goals.

Code reviews are painful

Well, code review is a practice and can’t be blamed if the code being reviewed sucks completely. Code review cannot be blamed if you have chosen to review a huge chunk of code weeks after it was merged. What may be painful is the way you decided to do code reviews and not the practice itself.

Challenges and Considerations

• Before it is merged into production: We work in pairs and with very short-lived small branches (approximately 2 hours of coding and very few classes changed). Pairs raise a pull requests (an average of 2 to 5 per day) and another pair reviews their code. Since we work in very small increments, reviews, although often, are very easy and quick to do. Code is rarely rejected and when it is, it is very easy and quick to fix. This way we avoid a lot of re-work and decrease the chances of accumulating technical debt.

• Disseminate knowledge and avoid unnecessary work: Having pairs reviewing each other’s code helps to disseminate knowledge of different parts of the system and also avoid that certain pairs do unnecessary work.

• Review code submissions during an interview process: Code doesn’t lie. CVs do. In the first phase of our interview process, we ask candidates to submit a code assignment. We then review it and give them a comprehensive feedback. This is our opportunity to evaluate exactly where candidates are in terms of their basic skills as developers.
• Mentoring apprentices: Our craftsmen regularly review all katas and pet projects developed by their apprentices. We find code reviews an extremely valuable teaching technique since the apprentices first try to solve the exercises to the best of their abilities and then they are shown better ways of doing it, if there are.
• Reviewing different approaches / experiments: Sometimes we don’t easily agree in how a piece of code should be written. When that happens, we normally work a bit in isolation to experiment and then have the code reviewed by the rest of team. It’s easier to debate over something concrete than having hypothetical conversations with endless “but what if… “ questions.

When not to use code reviews

• Verify if there are bugs: I find this pointless. We have tests for it. Code is only reviewed after all tests pass. Tests are part of the code review though.

• After the code is merged into production: That is too late. What do we do if we find that the code is not good enough? If the code is already written, it’s working, it’s in production, and won’t be changed anytime soon, why review it and change it? I would wait to change it if I ever need to work on it.

• When the code being reviewed is too large: Reviewing large changes are painful and disrespectful. In my last company, we decided as a team that we would reject large changes. The pair of developers would need to break that change in smaller changes and commit them bit by bit so the reviewer could make sense of it. That was also used to teach them a lesson. Only small commits would be reviewed and merged into production. Period.

• Check for code standards: I normally use Java and we have plenty of static analysis tools that are executed during our builds. If the code doesn’t comply with the bare minimum of quality defined (by the team) in these tools, the build fails. No code is reviewed if the branch build is failing.

There are other situations where I would or wouldn’t use code reviews but those are the most important ones.

Pair Programming vs. Code Reviews

Code reviews are quick and cheap to do when you work in small increments, in pairs, and constantly rotate pairs. As domain and technical knowledge are quickly spread across the team, code reviews become more a mechanism of knowledge sharing than actually a quality gate. Pairing give us a much quicker feedback loop and helps to minimise many mistakes. In our experience, code written by a pair has a much higher chance to be accepted in a code review than when it is written by a single developer.

Conclusion

Many problems during code reviews can be avoided when we have a good team culture. Practices without goals are pointless. Before choosing a practice, figure out exactly what you want to achieve and only then choose practices that will help you with that. Practices can not be blindly adopted. In order for them to work, you need to have the right context and mindset. Otherwise you run into the risk of blaming the practices for your own deficiencies.