Code reviews, or the practice of going over another’s code, have become an integral part of our team’s development workflow. There are several types of code reviews, but this article will focus on a few key scenarios where code reviews have served to significantly enhance our process and the quality of our work.
At Advomatic, we’ve been using code reviews for several years, but we’re always looking to change things up, try out new ideas, and learn from our strategic experimentation. Over the past year, we’ve been thinking hard about the purpose and value of code reviews, and how we can improve our process. This is what we’ve come up with.
Quality Control
Quality control is the most common way we use code reviews. Every ticket that involves substantial* commits is code reviewed by a different developer on the team. Usually, we divide the reviews between the front-end and back-end teams, so developers are only responsible for their area of expertise. Sometimes, on small teams, we bring in developers from another team, just to provide reviews. These reviews regularly focus on a set of common standards, including:
- Is this the right approach for the problem? Maybe you know some trick that the author doesn’t.
- Coding conventions/standards: Honestly, we don’t put too much weight on this. Yes, we adopt Drupal coding conventions, but there’s no value to our clients in spending the time to add a missing space in
if(or fix a comment line that went longer than 80 characters. - Proper documentation: are functions well commented? Is the purpose of the code explained or easily understandable, particularly for complex solutions or unique requirements? Can we understand why a bit of unusual code is doing what it’s doing?
- Efficiency: is the code as concise as it can be? Is there any unnecessary duplication? Is it sufficiently modular?
- Clean-up: has all experimental and debug code been removed?
- Security: Is all output of user-generated-content properly checked/escaped? Are all potential breakage points properly handled?
- Organization: is the code located in the proper module or directory? Does it leverage existing functionality?
- Bugs: Are there edge-case bugs that you can see aren’t handled by the code?
Knowledge Sharing
One of the challenges of organizing a team is that our work can become siloed. If each person is working on a separate feature, they alone are familiar with the specifics when it comes time to make a change. By having team members review one another’s code before delivering it to the client, we help ensure that critical details are passed around to everyone.
Often, there is a team member with a particular skill or expertise and it makes sense to have them take on tasks in that area. With code reviews, junior developers can learn from their more experienced teammates and be ready to tackle a particular challenge on a future project.
The Hiring Process
We’ve recently incorporated code reviews into the interview process. We’ve long required that candidates give us a sample of their work and then perform internal reviews to evaluate their potential strengths and weaknesses. However, after flipping that process on its head, we’ve found we learn even more by having the job seeker review a flawed (usually in multiple ways) code sample. Not only does this help to reveal what the candidate knows, but it also illuminates how they communicate and what they might be like as a team member.
Best Practices
As you can imagine, who does the review and how they do it are essential ingredients for a successful outcome. It is crucial that everyone comes into the process with the proper attitude and communicates using an effective tone. For a senior team member, there’s a difficult balance between providing enough information to home in on a problem, while at the same time encouraging self-sufficient problem solving. In these cases, it helps to frame suggestions in terms of the desired outcome, rather than describing a specific solution. For example, instead of saying, “Make the database call happen outside of the foreach loop,” the reviewer might propose, “Could we improve performance by altering where the database call happens?”
On the topic of tone, it’s best to keep things friendly, provide lots of encouragement around the criticism, and focus on critiquing the code – not the coder. It is also important to consider how much to say and when to say it. If a junior developer is struggling with a solution, it can be discouraging to hear multiple comments about where the comment lines are wrapping and missing punctuation (not that those comments don’t sometimes have their place).
When the process is handled with careful intent, the code review process can be an effective tool for team building and knowledge sharing. Our tickets are regularly filled with responses like, “Great catch! I can’t believe I missed that,” and “Good suggestion, I’m on it!”
One thing to remember is that while this arrangement is working well for us, your team might need something different. A two-person team building a website for a small non-profit has different needs from an 80-person team working on an online banking website. Find the balance that works for you.
What are the key ways you use code reviews in your process? Are there any important principles or best practices you’d add to the ones listed here?
Thanks to Dave Hansen-Lange for reviewing (and improving) this post.
* That’s right, we don’t code review everything. If there’s no value (particularly to the client) in having someone review the code (maybe only a few lines were changed, maybe the new code is self-evident), then we skip it.