Code reviews are a standard process anywhere but the tiniest startups. They can improve both the quality of the services we build and the quality of our teams. Unfortunately, many teams treat code reviews as a bureaucratic exercise, a box to check for compliance reasons. Others see the value but nevertheless fail to train new hires or build a strong review culture.
Let's talk a bit about code reviews. This will be a two-parter. This week we'll consider why we should do code reviews and what the process should look like. Next week we'll approach it from the perspective of you, the reviewer, and how to do a good code review.
Why Code Review?
The obvious reason is to find flaws in the code (a form of static testing). Flaws found at the code review stage are easier to investigate and resolve than flaws found after a deployment. This is especially true for deep issues, like database load, that might not be apparent until you have production levels of traffic flow through the system.
An equally important reason is to share knowledge with other engineers. It is a great way to learn new techniques, see alternate solutions to similar problems, and keep apprised of changes in the system. No amount of lunch n' learn meetings or formal training can replace a constant stream of watching how the rest of your team solves problems.
Code reviews also improve maintainability over the long term. The need for another human to understand your change encourages readable, consistent, and well-documented code. Encouraging at least one other engineer to understand how the code works lessens the risk of the author becoming a single point of failure.
How to structure code reviews?
Once we accept the “Why” of code reviews, we need to consider the Who, What, When, and How. (I tried to think of a non-trivial Where to round out the Six W’s, but came up blank).
Who: Everyone who writes code
Everyone who writes code should also review code.
There are teams where a subset of the engineers are allowed to approve code changes. This is justified by claiming the code quality will be higher if all code is approved by one of the "proven" approved approvers.
This leads to two problems. First, it makes the small group of reviewers a bottleneck, delaying reviews for the team. Second, it can create an "Us vs. Them" mentality. The non-approvers may learn to view the approvers as roadblocks to progress. The approvers may learn to view the non-approvers as consuming much of their time, preventing them from focusing on “more important” tasks. This does not lead to a great working environment.
As for the claim that code quality will be higher, this may be true! But newer reviewers will have fresh viewpoints and learn more from the process. Even if some individual PRs have lower quality, the team and product as a whole will benefit.
What: All code should be reviewed
I’ve heard claims that some code is too short or too simple to deserve code review. Unfortunately, short and simple changes can also contain flaws, and I’ve seen site incidents stemming from one-liners. Short and simple code should receive a short and simple review.
When: Often and for short sessions
Have you ever been studying, reading a dense book or academic papers, and looking up realize that you haven’t retained a single idea? The same thing happens with reviewing code. It requires a level of concentration that you can only maintain for so long before diminishing returns set in.
So keep your code reviewing sessions short, while increasing the frequency enough to get through your backlog. Encouraging your team to write small, discrete pull requests with excellent context will go a long way toward making this possible.
How: Use a lightweight approach with a quick turnaround time
Limit the amount of process. One of the best ways to keep the process simple is to delegate as much as you can to linters and other static analysis tools. They can handle style, formatting, test coverage, and even some basic security checks.
Having a style guide with automated linter enforcement saves us from draining, recurring debates about style and formatting. Tabs vs spaces anyone? Auto-fix options in modern linters also have removed the drudgery of having to manually match style.
Other options to simplify the process include:
Minimize the number of required reviewers.
If you use checklists, keep them simple.
File new tickets for large changes or refactoring.
Let’s talk communication
Asynchronous communication can be difficult. Getting intent and tone across during a review can be a challenge. You and the other engineer may have strong opinions about the “right” way to do things. All these can lead to disagreements, hurt feelings, or simply slow down the process.
In order to keep things flowing and maintain some harmony, consider these tips:
Ask, don’t command. When requesting a change, frame it as a question instead of a demand. This encourages further communication and doesn’t put the author on the defensive.
Be humble. Stay respectful and avoid using hyperbole or sarcasm. The goal is to work together to improve the code, not to be “right”.
Suggest alternatives and discuss trade-offs. The author may have already considered and discarded your alternative. Your goal is to understand why. Stick to the pros and cons of different approaches and seek a quick resolution.
Express how strong your opinions are. When you make a suggestion, let the author know how strongly you feel about it. If you feel it is absolutely necessary, say so. If it is a “nice to have” or a nitpick, say so. This will help the author prioritize their changes.
Talk synchronously when necessary. If the comment threads are getting too long or complicated, take it offline and talk in person or have a video chat. Be sure to summarize the conversation in a comment afterward to keep the decisions transparent and in one place.
When things get too philosophical, let the author decide. If the discussion isn’t going anywhere and you can’t make a data-based decision, it’s best to let the author make the call. The goal is to move quickly and make the code work well, not to achieve perfection.
From Theory to Practice
This installment examined code reviews from a process level. In the next installment we will continue the code review topic, but approach it from the specifics of what to look in the code when reviewing.
If you have any examples of good or bad code review experiences, let us know in a comment or email reply!