Should everything be code reviewed?

An issue that I haven’t seen addressed in depth in is whether you should review every commit to your code base or only aim to cover critical parts of your application. As usual, there isn’t a single answer to the question, and there are advantages (and disadvantages) to both. The answer will often depend on your team’s context, so let’s dive right in to see if we can make some decisions on when to use a certain methodology over another.

Let’s inspect every single line of code!

It may seem a bit ambitious to do this when first considering it, but it’s probably the easiest solution to implement from an execution point of view. It removes all questions as to whether some part of the code should be reviewed or not. And if you integrate code review to your development process, treating it like any other step (such as coding, QA, deployment, etc), it’ll become a habit for your team and be done automagically. The best integration will automatically generate a review for all the code that was committed for a given feature, saving the developer from having to sort it out themselves. This can be accomplished with both a dedicated code review tool as well as a GitHub-like pull request.

With this approach, you should see fewer issues getting through to production and improved long-term maintainability to the code. The logic behind this is simple: the more eyes that look at the code, the more different perspectives that’ll look at the problem differently and see potential defects and improvements. It’s important to have a well defined process for reporting and tracking found defects. Without a clear process, you may end up losing track of defects, and the person who asked for the review might feel discouraged from being drowned in defects that they aren’t sure really need to be resolved. Having too many defects brought to the forefront will take much longer to complete the task, or pollute the backlog with issues that may never be important enough to warrant time being spent on them.

Reviewing all code that is committed will also help to keep the code more consistent and feel like it was all done by a single team, not by 4 different developers with different approaches.

It’s difficult to use the review-everything approach when starting a new project. There’s a few reasons why it’s a special case, but the main issue is due to the high velocity at which code is being churned as the project takes shape. A group review in a conference room might be more productive for the first few iterations of development, or at least until things start to stabilize. Block out half a day, put everyone in a room for an hour (and no more), and let them identify and triage the discovered defects. Finally, split up the defects among the team members, and take the rest of the allotted time to resolve them.

The other factor to keep in mind is that reviewing every commit is time consuming. Thirty minutes per review might not seem like much, but if you’re on a team of four developers, that’s between three or four hours dedicated to code review. At that point, you need to start considering how many people – and which people – are invited to your code review. You should also have a look at ways to improve your code review process, from the tool you use to how you track and choose which defects to address.

Suggestions to improve review-every-line-of-code

• Strive to keep the reviews short. No code review should take more than 30 minutes to inspect. Otherwise, developers aren’t going to pay as much attention as they should and issues will go unnoticed or ignored due to the mountain of code to review.
• Automate a part of your process by checking for code formatting and naming conventions automatically, failing the build if any of these inspections don’t adhere to the standards. This will go a long way to reduce the “background noise” comments that often permeate reviews.
• Tightly integrate reviews to your development process so that it becomes seamless and second-nature. If you use a project management tool such as JIRA to track your development workflow, there could be a step for code review. That step would ideally integrate with your code review tool of choice to simplify the work of the developer preparing the review.
• Above all else, make sure your team continues to communicate throughout the review process. Make sure everyone agrees with the defects to be addressed and don’t let developers feel bad about the code they wrote.

Review Critical Components Only

Choosing to review only critical components is accepting the Pareto rule, which states that you’ll get 80% of the benefit for reviewing 20% of the code. If you chose to go down this route, you’re saying that you want to focus on what’s most important to your code-base. There are two major blockers to achieving this. First up is figuring out what is and isn’t important to your project. And then you need to find an easy way to enforce and apply those rules religiously to the code.

How do you choose what’s important?

There’s a few strategies we can use to determine what should be reviewed. For most of these, you’ll want some metrics on your application to make a proper decision. If you’re working on a greenfield project, take a best guess as to which parts will be the most affected by the following strategies.

You can try to review only the most used parts of the application. The reasoning is fairly obvious. The code paths that are executed more frequently will gain the most from being as defect free as possible. Sure, the other parts of the code could also benefit from having an extra set of eyes look at them, but if the edge case code is rarely used, a slight improvement to it isn’t going to be worth the return on investment.

A complementary way to improve on the previous point is to also review the most maintained parts of the application. Code that changes very often is likely to acquire “gunk” along the way, which can make them more susceptible to becoming a big ball of mud. In a way, you’re saying that you’ll review the parts of the code which are more likely to acquire bugs as time goes on and therefore need to be validated by the team.

Yet another complimentary strategy can be to review code that was written by the newest developers that joined your team. Whether they’re juniors or seniors, there’s a learning opportunity for both the existing team and the new teammates. Every developer has a different “signature style” and learning how another person codes goes a long way to understanding their code philosophies. Learning the tendencies of your team mates will improve the code of the application, as well as promote a learning environment where all developers feel like they are contributing.

A no-brainer when it comes to review is security. Security is of course a very large topic, and I won’t even attempt to discuss the biggest issues here as there are so many facets to it. I however would recommend reading through OWASP’s guide to code review for security. It explores how to review many security-related topics. And keep your eyes open for a review of those guidelines on this blog soon enough!

At the end of the day, you need to figure out what’s critical to your code-base. It may be none of the above mentioned areas, but make sure you find it and review the crap out of it.

Suggestions to enable reviewing critical code only

• Make sure your entire team has a clear definition and rule for what does and doesn’t need to be reviewed. If the definition isn’t clear enough, you may end up with important parts of your application that don’t get reviewed.
• Naming of packages and namespaces becomes especially meaningful, allowing the team to easily identify the parts of the code that are to be reviewed.
• Track the code review as a separate task in your project management tool to avoid the review step being forgotten.

Final thoughts

The “critical code review” is probably in an organization’s best interest. Developers don’t waste time reviewing and fixing code with a relatively low value to the business. On the other hand, the parts that are important will get more attention therefore reducing the risks of lurking defects. The major problem with this approach is developing a systematic way to ensure that the review is always done. Mitigate that risk by tracking the code review as its own task!

As I mentioned in my closing thoughts of the last article, it’s more important to get started doing reviews than the actual process you choose to execute them in. If you aren’t doing any reviews yet, try doing it on the most valuable parts of your app to start. If you’re already a seasoned code review team, try and improve upon your process with the above suggestions.