Are code reviews hurting your team?
At the risk of sounding too cliché, the answer is clearly a maybe. More interesting than that though, is understanding how you can ensure they aren’t and put a process in place that is helpful instead of hurtful. Code reviews that are treated as chores and don’t add the value that was promised are more prevalent in our field than what we might like. However, if we start with the assumption that everyone is smart and doing their best, what are we left with?
The purpose of Code Reviews
In my opinion, most of the times people complain about a code review process, it stems from a lack of a shared understanding of what problems it is solving.
According to Wikipedia a Code Review is:
a software quality assurance activity
Google defines it’s purpose as:
… make sure that the overall code health of Google’s code base is improving over time.
We could look at more definitions, but they would all lead us through the same rabbit hole because both “quality” and “code health” are highly subjective concepts. It is hard to build a deterministic process around these concepts, therefore we are left with a faint sense that code reviews should make your code better. As a developer myself, I need something more concrete.
A few things I think we should aim for when thinking about a code reviewing process are:
- Two brains are better than one (three brains are not necessarily better than two)
- Leveling up team members
- Spreading knowledge, both technical and product wise
- Boring processes will yield boring results
You might agree with all, some or none of them, but more important is that you and your team can agree on a list similar to this, because if you can’t, maybe Code Reviews are not the right tool for you.
Thinking through a Code Review process
Now that we have established a baseline to help us think through what we want out of the process, let’s look at each point separately and how we can incorporate it.
Two brains are better than one
This one is a no-brainer (pun intended), when you are coding by yourself you are deep down in the weeds and it is easy to miss some things that a fresh pair of eyes will catch instantly. The fact that most code reviewing tools show only the diff for that particular patch of work, makes it even easier for a reviewer to catch some of those silly mistakes. A particular pain point for most reviewers is not having enough context around a change to move past reviewing how something is done, into why something was done that way. Honestly, this is an extremely hard problem to solve simply with a code review. Some suggest that you should download the changes and run them locally, try a few alternatives, maybe even read the specifications for the task, if they exist, but even if you are familiar with that part of the codebase it is a hard task. A decent solution I’ve seen to this is to ensure that there is a technical specifications document that provides context as to why things are being approached that way. This document can and should be reviewed before any code is written, thus minimizing the time wasted. Bare in mind though, that as a startup this might be more process than what you can chew. Another possible solution is to pair program. By pairing you are in essence moving the review process from after the code is written to while the code is being written. Not everyone enjoys it and it consumes a lot of energy, but it certainly is a very good tool to be able to reach for. When pairing you might even do away with code reviews altogether if the pair is confident in both the language they are using and the part of the product they are building. How many reviewers is a good amount of reviewers? It really depends on the dynamics of your team, but I’d say that a second reviewer will add very little and a third one is almost always a net loss in productivity (unless there is some very specific knowledge you need).
Leveling up team members
More junior members of your team will learn a great deal from getting reviews from more senior members. It is, however, very important that there is a culture of positive feedback and of non-blaming, in order to create a safe environment for people to grow. Aggressive or demeaning comments in a review should simply not happen. There are multiple articles on code review etiquette, I recommend this one. Although a code review can be a good way to learn, it can also take a long time in an asynchronous back and forth between reviewer and reviewee. In that case, consider a synchronous mechanism of communication, such as a chat or preferably getting on a call. Pairing to start with is also a nice alternative here, depending on how much you prioritize the leveling up of said team member.
Spreading knowledge
The simple existence of code reviews, more or less guarantees that there is no part of a codebase that only one person knows or has ever seen. Other than building a feature on a given codebase, this is probably the best mechanism I’ve seen to keep people in the loop of what is happening. Any process that entails keeping documentation up to date, I wouldn’t trust, as documentation can and will be outdated at some point. Periodical presentations on certain topics or parts of the system are also great, but nothing beats code reviews as a systematic way of sharing what is being worked on and how.
Boring processes will yield boring results
Having a code review process with unnecessary steps is sure to make your team stop caring. Code reviews should not be about code formatting, an automatic linter can do that. They shouldn’t be about ensuring tests exist and pass, a CI can do that. The more of these repetitive tasks you can get rid of for your team, the less you leave for them that is not providing a good review. There’s a lot of companies that require a certain number of approvals, or even worse, that a certain person approves, in order for code to be merged. I understand that sometimes this is a legal requirement, but take into consideration that albeit well intentioned this could be a blocker for your team and in time become a pro forma, executed blindly.
Final Process
To sum up, we want a code review process in which anyone can review anyone’s code, providing useful comments and positive feedback. This should lead to a well rounded team that is not afraid to tackle issues in any part of the codebase, that is excited by the possibility of helping build the best product they can.
Common pitfalls
Even taking into account all of what we’ve discussed so far, there are some common pitfalls when doing code reviews
Reviews taking too long
It is extremely common to have reviews taking too long, either because there is no reviewer available, or because there are many comments back and forth. The former can be solved by ensuring that when a code is up for review, there is a reviewer assigned and nagging them until they review it or find someone else to review it. It is important to understand that a couple of people at 50% is better than one at 100% and another at 0. The productivity is the same, but the mental toll is not. The latter is usually a sign that the initial specifications or assumptions were wrong, there is a junior developer involved or that there is a debate of opinions. All of which can and should be solved by moving to a synchronous mechanism of communication.
Reviewer is always the same
The number of reviews any one team member does should be more or less the same, but it is not uncommon to see a big disproportion here. The best way I know of how to fix this is to have an automatic mechanism of assigning reviews that ensures everyone gets their turn.
Too many comments
This usually happens on small sets of changes (which is what you should aim for), in which a reviewer has a tendency to put too much of their personal opinion into a review and question everything that is done in a different way of what they would’ve done it. Raising the issue and working on code review etiquette is my favorite approach here.
No comments
Also common is a code change being approved without comments. This could be a good sign, nothing to point out, or, especially in large change sets, that the reviewers skimmed through the changes without paying proper attention. My recommendation here is to aim for smaller change sets, breaking down a feature into smaller chunks, or pairing on the feature instead.
About the author
Luis is a co-founder of Subvisual, a Venture Studio with offices in Portugal and the US that focuses on helping early stage entrepreneurs bring their ideas to life. Luis has been developing web applications for over 10 years, mostly using Ruby and Ruby on Rails and Elixir and Phoenix. On top of that, he has taught web technologies to hundreds of people, through workshops, bootcamps and university classes.
Interested in a Featured Blog Spot?
If you are interested in having a featured article in our blog, please reach out on @reviewpad with your proposal.
