Best Code Review Practices: Establishing an Efficient Team

In this article we’re going to take a look at some best practices and processes that you can establish in your development team to make the code review process smoother. Code review is a vital part of any development team’s lifecycle, and it’s important that you ensure everyone is on the same page with the reasons for doing code review and how the person writing the code and the person reviewing it can work together for the benefit of everyone.

Perhaps counter-intuitively, I like to take as much of the actual code review out of the hands of the reviewer and into the virtual hands of the computer. There are many automated code formatters and stylistic checkers (in the front-end world, Prettier and ESLint are the most popular examples) and if you do have a style guide or way of formatting code, you should ideally use tools such like these to enforce it, rather than asking a human to do it at code review time.

• An automated tool won’t miss any issues, whereas when reviewing a big change it’s very, very easy for a reviewer to miss a misplaced semicolon or some incorrect indentation.

• Developers can often develop strong preferences to how their code is formatted, but having developers debate these in code review is not productive.

• When we upload a change on my team, checks are automatically kicked off to ensure code is formatted consistently (we run our formatter and ensure there are no changes in any files) and we also run our code linter to check no rules are being broken.

• Automating these checks also removes them from the headspace of the reviewer; now they are free to think about the bigger picture and the code’s architecture and design, not the minute details of its formatting or choice of quotes. This is what you want your reviewers focusing on, everything else is a distraction.

• Another check we run automatically is our unit tests; these are run on every change to ensure no change can introduce a failing test. Until those tests have all completed successfully, it’s impossible to land a change into our codebase.

I once worked on a team where friction was caused by different people on the team having different goals and ideas of what constitutes a code review. Some saw it as a way to ensure a consistent coding style (which I’d argue should be automated, as per the previous point) and others wanted to dive into the code’s architecture and think about the approach (closer to what I’d say the goals are).

You might disagree with the below list, or want to tweak it, which is to be encouraged, but the crucial issue here is to have a set of goals for code review that your entire team sticks to. Mine would look something like:

• Knowledge sharing of your code base
  Code reviews are a great way for the reviewer to have a better understanding of an area of the codebase by regularly reviewing changes in that area (although nothing is a substitute for actually working on changes).

• Learning from each other
  Reviews are a good opportunity for the reviewer or author to learn from each other: perhaps the reviewer will see a new approach to a problem that they’ve not seen before, or the reviewer will spot an opportunity to use a new language feature that the author didn’t know.

• Critique of the code’s design
  It’s important for the reviewer to think at a high level about the approach taken in the change; is this an appropriate solution for the problem at hand? Does it maintain design consistency with the codebase? Is it re-using code in the right way, or re-inventing the wheel?

• Onboarding new developers
  Related to “learning from each other”, if you have a new developer on the team, pairing with them to review some changes, or having more senior developers review their work, will help them get up to speed quickly whilst feeling supported and getting to know their teammates.

It is also important to agree on the non-goals of code review. What are you explicitly not looking to achieve in code reviews?

• Code formatting checks
  As discussed, let’s offload this to automated checks run by the computer.

• Deep architectural review
  There’s not much more frustrating than having a developer leave so many comments and tweaks that it feels like you should just start the change all over again. We do want to use code review to improve the design of a feature, but at the point the code is written the very high level approach should be relatively set. The time for large scale changes to the approach is earlier in the process: perhaps a prototype of a solution, or a design document detailing the approach that will be taken. These can then be discussed and agreed on before diving into the details of the code.

This point has become more relevant now that many of us are working in a distributed fashion across timezones; it’s always important to be empathetic and consider how your code review comment might be read by the person whose code you are reviewing. Comments written and read as text lose all nuance that you could convey when discussing in person, and as such it can be easy to leave a small remark on a part of code and have it misconstrued.

Here’s some things I encourage my team to think about when commenting on code reviews:

• Remember that people can work hard on making changes, and you should be careful not to come across overly blunt or rude when leaving feedback. Especially if done over the internet, tone can be missing and things can easily be misconstrued!

• When leaving a comment, make it clear how severe this comment is in terms of you being happy to leave it be, or wanting to change to be made. Is it “we must make this change before I am happy for this code to land” or “here’s a suggestion, take it or leave it” ?

• If you aren’t sure about something, ask! As mentioned, reviews are a great learning opportunity. You can leave comments asking questions to help you understand.

• If the initial code isn’t in a state where you’re happy for it to land, that’s OK, but try to highlight all the issues in the first code review. It can feel frustrating as an author to fix some code based on comments, only to get a whole new set of comments and required changes in a follow-up review. Ensure that after the first review the author has all they need to get their change to a position where it can land into the codebase.

In the last point we discussed about leaving comments as a reviewer, but as the author of code you need to remember to be open to your colleague’s feedback and see the process as a chance to learn and improve.

You are Not your Code: Be open to Feedback and make the Reviewer’s Job Easier

• Remember that you are not your code, and the reviewer isn’t making comments about you as an engineer, but about your code, and they are not doing it to criticize, but to help you and your team.

• Remember that you are all working towards the same goal of a better, more consistent, cohesive and well-structured codebase.

• Help your reviewer out; if there’s a bit of code that looks a bit “odd” on first glance but there’s a contextual reason why, leave a comment explaining it.

• Sometimes a quick face to face (or video!) chat can make things clearer: if you find yourselves going back and forward on code review, that can be a quicker way to reach a solution.

It’s important that a software development team feels that they have momentum when working on their features, and shipping something that has taken dedicated time and energy always feels great. Therefore it’s important to have an agreed understanding on the importance and relative priority of a code review.

• There’s nothing more frustrating than creating a change and waiting days and days for someone to review it!

• Agree on your team what a reasonable timeframe is to review changes (for my team it’s 24 hours). After that time it’s reasonable for you to chase the reviewer, or ask someone else to review.

• Equally if you’re asked to review something that you know you won’t get round to, say so, so that someone else can be found.

There is an art to producing a good, self contained change to review. If you let your change get too big it becomes harder and more time consuming to review. Keeping them well sized and in a clear related chunk means the reviewer has less context to load into their head. Try to keep this in mind as you are working, and if you are given a big project to work on, think about how you can break it down into smaller chunks.

• Once a new engineer on our team was given their first project, but instead of breaking it down into separate parts, did it all in one huge change. The code review had literally 50+ comments, and it was a energy-drain for all involved, and left that engineer demotivated.

• Their mistake was not splitting up the changes into multiple parts. If you do this, each part is smaller, easier to review, and less work for the reviewer.

• If you need to work on something whilst waiting for code review, you can always make use of version control (such as git) to manage multiple code branches, and build on top of code you’re waiting to be reviewed.

• If you’re not sure how to break a piece of work down into smaller changes, ask someone who might end up reviewing them what they think. Agreeing on how the work will be split up can save time in the review process later as everyone will already be on the same page with the approach.

Code review is an essential part of any good software development team’s process, and I hope this article has given you some practical tips and suggestions on how to keep this process lean, efficient and effective for all involved.