<- Back to home page

Supplementary Guide to Code Reviews

Code reviews are hard; they are much harder to do than just writing code. The reason for that is the same as why engineers always want to redo things from scratch - you don’t need to try to understand your own ideas. Plus, your own ideas are always focused on solving things you know, so you don’t have to make even bigger efforts to get to know the problem space first.

After spending years at different engineering levels, on different teams with different coding techniques and values, I concluded that it is impossible to write a set of very detailed and specific instructions on how to do code reviews in general, so I’ve given up on that. Instead, in this rather short guide, I want to bring you a toolbox of ideas forming a specific mindset which, hopefully, should help you to build a quick and enjoyable code review process.

What code review is

First, let’s get out of our way one thing that is commonly mistaken for code review: code review is not an evaluation of your work as a professional in general. It is not an exam and the result is not an indicator of you being a good or bad engineer; you won’t get marks. You have chosen the company and your colleagues already saw you as a valuable contributor; you have passed all the interviews. Now, we are building something great together.

Code review is a collaborative process of pushing meh to ok, ok to good, and good to great. Try to see it as a pair programming session where you expect your (reviewing) partner to be slightly less involved.

It is a great tool for you to get help and deliver something better than you would do without it.

It is also an alternative way for you to contribute to the product without writing code directly and a way to showcase techniques and exchange ideas.

While being an extremely valuable tool, code review is just a step in the process of building a product. It is very important to make progress towards the end goal, and, unless you are writing a technical book with snippets of perfect code, the end goal is likely not perfect code.

What to keep in mind before starting the review

  • The author is hoping to make progress as fast as possible, be nice and don’t block their progress whenever possible
  • The author has made the change to push the product forward in a positive direction. Even if this attempt might not be that successful at times, it is typically the intention
  • The author already did their best to follow coding practices and used their best knowledge of tools. If you see something missing (for example, they didn’t run a linter), assume they didn’t know about it
  • You are now equally responsible for moving the progress forward, not a judge, and should share the previously mentioned intents as well

How to start the review

Make every effort to understand what the proposed change is supposed to do and how. Usually, you have a few sources to collect this knowledge:

  • Issue tracker (GitHub, Linear, etc.) describing the desired outcome
  • PR description
  • Discussions in Slack

Of course, the author already has a comprehensive list of all sources; make sure to ask them to add missing parts if needed.

Once you’ve understood the task on a general level, look if it worth paying attention to the order of commits and commit messages.

There are several practices on how to organize them and different people follow different strategies. Even if you personally never write meaningful messages and don’t care about the order (for instance, because all of it will be squashed later) - a lot of people use commit history as a communication tool to actually simplify the review process for you and guide you through the changes. Not everyone mentions that in the PR description, but it might simplify your work.

Before starting the review, have a brief look at the commit history and make a decision if you want to review it commit-by-commit as a story of changes or all changes altogether (or maybe some other way you prefer).

What to look for

A short answer - overall code health. Ensure that the proposed changes do not degrade existing code health, including readability, maintainability, and functionality. A lot of guides tell you that the proposed change should push code in a positive direction, but this can’t happen all the time. Don’t force extra work on people - they will contribute improvements when there is space and time for it.

This topic is highly subjective, opinionated, and cannot be covered with a comprehensive list. It is also highly dependent on team coding practices, tooling, and the language, but I want to be somewhat specific and provide a few examples to form at least a shared understanding of the level we are on.

Readability

  • The change does not add unnecessary new concepts (for example, something new called Manager while the rest of the code consists of Controllers)
  • Code is easy to reason about—it does not hide well-known concepts or unusually reuse them without a clear need
  • Logical code blocks (modules, classes, functions, etc.) do not have unnecessary nesting or unnecessary extra dependencies, which might require the reader to keep a larger context or dependency tree in their head while trying to navigate the code
  • The control flow does not have unnecessary complexity
  • Coding style targets the audience (don’t bring too unfamiliar concepts without a clear need). All great ideas are borrowed from other contexts; borrowing works well and should be praised, but new ideas should also serve a specific purpose and not overload others without a clear need
  • Prefer explicit to implicit unless implicit is a well-established coding practice in the space and (or) removes cognitive load rather than introduces it. This can make code more accessible to people without specific domain knowledge, reduce learning curve, and make it easier to contribute and spot issues

Maintainability

This can be broken down into two parts:

  • Is the code easy to understand?
  • Is the code easy to change and trace down problems?

The “Understanding” part should be covered by having readable code. Here are a few things to watch out for to cover the “Changing” part:

  • The code-executing path is well-defined and can be tracked without pen and paper (to give you an obscure example, there are no two timers that depend on each other and have a recursive function in the end)
  • Changing seemingly unrelated parts should not silently break the proposed change and vice-versa

While you can try to predict possible scenarios, you can’t predict every single potential change. Don’t try to optimize for an uncertain future unless this is something critical - code is easy to change when the time comes and important things always get addressed.

Functionality

The proposed change should actually do what it is intended to do and not break other parts.

It is important to acknowledge that it is not a reviewer’s responsibility to ensure the proposed change is bug-free and works according to the spec. To some extent, you share this responsibility as a team, but a reviewer can’t afford to spend an equal amount of time on the task. However, a reviewer should fully understand the intended outcome and suggested approach to be able to do a quick sanity check on the code.

How to give feedback

Be extremely specific. Never comment or ask questions assuming the author will see the issue if you simply point at a specific line. It is very likely they made a mistake because they didn’t see it in the first place; don’t throw a new riddle on top of it.

Avoid comments like:

  • What is it?
  • Isn’t it a problem?
  • Are you sure?
  • Why?
  • etc.

Instead, mention a specific variable, function, concept, or element you are referring to and expand on details:

  • Is this assignment safe? I see it is also used in place X and place Y and might break in case Z

Don’t sugarcoat things or try to be overly polite. This generates extra noise in the comments. While sometimes it is good to adjust your tone to account for cultural differences, assume the author would make an equal effort to understand you, and you will meet in the middle.

Feel free to suggest ideas, but remember that it is not the author’s responsibility to try them out. Any nice-to-have suggestion should be non-blocking. If all you have are nice-to-have ideas - approve the PR and pitch your ideas in the comment, leaving the choice to the author.

Leave a clear description of a path forward after the review. Point out critical places that must be addressed and places that can be addressed. Leave the choice about optional places to the author, but make sure this is communicated.

Shared responsibility is no one’s responsibility. It is okay to bring other people into the discussion in areas where you don’t feel confident, but be aware that doing so will still leave you responsible for providing feedback and communicating expected changes to the author.