Code Review: Why It Matters
Trevor Lalish-Menagh, @trevmex, https://trevmex.com
Code review is the art of studying another person's code, making suggestions and observations and learning from one another. It is a powerful tool for building knowledge of a code base, creating strong bonds in a team and improving code quality. Used wisely, code review can help make a product better and be released with fewer defects. Used poorly, in an unsafe environment, it can paralyze teams and cause strife. Whether you participate in a code review process today or you are looking at experimenting with it in your team, this article hopes to guide you to a better experience and understanding of the art of code review.
Before we dive deeply into the topic of code review, though, we first have to talk about something else: fear. Code review, the practice of allowing others to study and comment on code that you have written, exposes you, the writer of code, to extreme vulnerability. For many people, their code is their heart and soul, exposing that to others is a very personal thing to do, and can only work well when a coder is placed in a safe environment. Therefore, the first thing we must make clear when talking about code review is that effective code review, that is effective and constructive criticism of code, can only be received when the person receiving the review is made to feel secure in their position and that their work is valued to the organization. Without this cornerstone of trust, code review, and really any constructive criticism, will be met with paranoia and fear.
The source of that fear often comes from an individual thinking that they will be "found out" as a bad programmer (this type of fear is referred to as imposter syndrome [1]). Imposter syndrome is a common phenomenon among women in knowledge work, but affects many developers, especially those at higher skill levels than their peers. It is our job as people taking part in a code review to understand that this anxiety might exist in us and others, be respectful of that and comment appropriately. A code review is not a place for blaming or shaming, it is a tool we use to grow as developers, help improve quality and support one another.
Types of Code Review
There are three main types of code review we think about when we think about code review: group code review, post-commit code review and pre-commit code review. Group code review, the least effective and most intimidating of the three, is where a group of people get in a room together, the developer puts their code on a big screen and walks everyone through it. This has the double-down sides of putting the individual developer on the spot and gives others power to shame and posture over the individual. Both of these activities have the result of weakening the safety of a work environment, making trust and collaboration more difficult to achieve. For these reasons, group code review is not what I focus on when I talk about code review.
That leaves post-commit and pre-commit code review. Post-commit code review is the method of reviewing a single code change, usually in the form of a patch that has been applied to a repository of code after it has been checked into that repository. This has the advantage of ensuring the code gets into a deployable state (i.e. in the code repository) fast, but has the downside of potentially letting defects pass through into the repository without review. The lack of gating that post-commit review offers makes it a weaker choice than pre-commit review, but still far better than group review, since post-commit review can be accomplished in a one-on-one basis and at the reviewer's leisure.
Pre-commit code review, using specialized tools like gerrit [2] or Review Board [3], allow you to gate a code patch and ensure it passes the code review process before being added to the code repository. This approach gives you all the advantages of post-commit review as well as ensuring code is understood and approved of by more than just one person on your team before it goes out to production. I have found that once adopted, this method is the most effective and least likely to be ignored of the three.
Why Review Code?
Regardless of the method your team chooses to go about code review, there are a number of important benefits that can be gained from studying and commenting on other's code and having your own code examined. Key among these benefits is mentorship. Everyone, from the most junior to the most senior developer of your team has some bits of knowledge that others on the team do not possess. By showing others how you develop code they can learn tips and tricks that you might have learnt long ago and use without thinking. On the other side, studying the code of others, especially the code of younger developers can expose you to different ways of approaching a problem and can help you expand the way you might tackle similar problems in the future.
Another reason to do code review is to reduce what we call the bus factor [4]. The bus factor is a rather morose analogy that goes as follows: if Jim the Coder, who has been the only developer working on a particular sub-system gets hit by a bus tomorrow, how long will it take the team to get up to speed on Jim's work? If no one else has been looking at the code Jim was working on it could be a very long time until another person would be able to get up to speed on the system. In extreme cases, the system might have to be rewritten altogether. By ensuring that another person reviews all code, the process of code review helps to mitigate the concerns that arise with the bus factor.
The most compelling reason you might seek out code review, though, is to find defects as early in your development process as possible. Two sets of eyes are better than one, and with another person examining code besides the developer that wrote it, less defects could slip by before code gets out to your repository. Not every defect will be caught, nobody is perfect, after all, but it certainly can help.
How to Review Code
The mechanics of code review are quite simple:
The devil is in the details, though. Giving high quality feedback is the real art of the code review, and there are a few areas that will give you the most return on your investments. The first place you will want to focus on while reviewing code is logic statements. This might sound intuitive, but when faced with another person's code, one can quickly get overwhelmed. Concentrating on logic statements (if/else, switch/case, loops, etc.) first can help focus your review. You want to try and understand the reasoning behind the logic, and ensure that it makes sense, e.g. does an if statement have an else attached to it? Does a switch's case break properly? What are the exit conditions of this loop?
Beyond basic logic, you can dive into cyclomatic complexity [5] issues. Cyclomatic complexity is the measurement of how complex a block of code is. Generally, if a block of code is deeply indented, meaning it has many nested if statements and loops, that is a sign that the code could be improved upon. This can often be achieved through refactoring [6], the art of redesigning code to improve understandability without mutating the outcome of the code itself. Improving understanding in a piece of code is a main goal of good code review.
To that point, another useful item to look out for in code review is proper naming. I have often heard from less experienced software engineers that the naming of variables, functions and classes are not very important. This cannot be farther from the truth, though. We write code for two audiences: a machine and a human. The machine does not care about names, in truth it ignores them almost all together. The human, on the other hand, cares deeply about names. Meaningful names turn confusing code into something that makes sense in the context of a larger system. Single letter variables and non-descriptive function names should be an artifact of a bygone era, if you see them in code you are reviewing, call it out.
When you are reviewing code, it is easy to get angry at what you see, especially if you spot errors or are reviewing code that was written in a different way than you expected. In these situations many people have a tendency to be passive aggressive. This is especially true when using a code review tool that is not in-person, and reviews are written down and reported back to the developer. It is because of this I would strongly encourage everyone that writes a review of a piece of code to reread their review and imagine themselves on the receiving end of their criticism before posting it. Reviewing code is about improving the system as a whole, it is not a tool to be used for lording one's superiority over another.
If you are implementing code review for the first time in your team, I would encourage you to not set any artificial restrictions on who reviews code, when they review it and how it is reviewed. Many managers, when faced with implementing a code review system tend to want to add rules around how the process is done. If you can resist the urge to do this, you will find a few things will emerge from the process. The team will start to form their own rules around how and when code is reviewed. Having rules grow organically instead of being imposed, especially when attempting to invoke major culture change can give the team a sense of ownership to the process they otherwise would lack.
An interesting side effect of giving a team control over doing code reviews is that it will brew conflict, oftentimes about what might be considered trivial issues. In my experience, chief among these trivial issues that always surfaces is the fight over tabs versus spaces to indent code. This is a classic case of bikeshedding [7]. Bikeshedding is the process of obsessing over seemingly meaningless details for much too long while ignoring more pressing issues. This happens largely because of lack of familiarity in a system. When presented with an unfamiliar piece of code and asked to comment on it, if a developer cannot understand what is going on, but can see that the code has mixed tabs and spaces, they then have something concrete to comment on, however trivial. This is an important phase to recognize in the adoption of code review, and can give the team a chance to come together to discuss how to develop better code. In the tabs versus spaces situation, a team might hold a meeting to codify basic coding standards and produce an artifact to hand down to new developers, or implement a static code analysis tool [8].
Unfortunately, as is true with any process change, there will be opponents. Most opponents' issues with the process come back to fear. The number one complaint I hear when implementing code review on a team is that it will take too much time. This argument implies that un-reviewed code and reviewed code will have equal value downstream. Though if even one defect is found earlier in the process, or a chunk of code is made easier to understand the team is ultimately saving time in the long run. In two teams I implemented code review on in a large multimedia company, we found that although initial velocity dipped, it quickly recovered and then surpassed pre-review levels over the course of a year study.
There are many tools you can utilize to do code review. Gerrit is a tool that was invented by the Google Android core team to perform remote code review with git [9] repositories. Review Board is a tool that can be used with any number of version control systems and achieves a similar purpose. These tools give the developer a web interface to study and comment on code at the line, file and patch levels. They support email messaging and continuous integration options. Both gerrit and Review Board are free to use.
There is no reason to wait to perform code reviews with a tool, however. Your team can start implementing code review tomorrow by simply reviewing code in person. Once a developer is ready to commit a patch of code, they will call over another developer to walk them through their changes. Commenting is done face-to-face and changes happen right away. I have worked with a number of teams that prefer this method and do it quite effectively. Toolchains can sometimes get in the way of inherently humanistic systems, like code review.
The Side Effects of Code Review
Reviewing code has some added benefits beyond those stated above. One surprising change that often occurs is smaller code patches being submitted. This is due to the fact that people tend to not want to have to understand large and complex changes all at once. This results in larger code patches lingering in a code review queue for sometimes days at a time. Smaller code patches are easier to understand, and thus easier to review. They are also easier to revert if something is wrong with them. Smaller code patches benefit the team as a whole because of this.
Reviewing code and having your code reviewed and a safe environment also has the powerful effect of humanizing one's coworkers. When faced with a system that is not working, it is easy to blame other's work, but code review puts a face to the code itself. The act of reviewing code opens up lines of communication the previously might not have existed. Once you have worked together with another person on a line of code, it is much harder to blindly judge them for their oversights.
Over time, teams that perform effective code review also gain more trust from their peers and supervisors. This is due to many of the effects stated above: less defects, stronger lines of communication and increased velocity. It takes time to build trust, but a team that is confident enough to share its code with each other will quickly rise to that level.
I would be remiss in this article if I did not mention pair programming [10]. Pair programming is the practice of two developers sitting together and coding together. One developer writing code, and another reviewing the code as it is written. The topic of pair programming is worthy of an article in and of itself, but needless to say that if you are in a team that is making effective use of pair programming you have already achieved many of the goals of effective code review. I like to present code review as an introductory step towards pair programming. It is an easier system to implement and can achieve many of the same goals in a less disruptive manner.
The moral of the story is that code review has the potential to make your team stronger, your code cleaner and you a better developer. If you haven't tried it out before, I encourage you to run an experiment with your team to implement code review with either a tool or by hand for four to six weeks. If you are in a team that is already implementing code review, I hope I have been able the shed some light on the practice. Together we develop better software. Happy coding!
References
Related code review and inspection articles
Four ways to a Practical Code Review
Click here to view the complete list of archived articles
This article was originally published in the Spring 2015 issue of Methods & Tools