A Pattern for Code Review Comments

Updated: 1 October 2024

I've seen code reviews be a source of friction in a team many times through the years and have tried many different approaches to making code reviews a smoother, and more pleasant, experience for everyone. I've never written it up before, but here, drawing from inspiration from several colleagues past and present, are my current suggestions:

Automate The Small Stuff: Avoid death by a thousand cuts

You really want human review time to be spent on stuff that only humans can comment on. That really means how easily can I understand the solution in this code? and does this solution solve the problem correctly?

Anything the computer can assess automatically, it should.

That means, use linters and prettifiers to remove all need for discussion about formatting. Use static code analysers to flag issues. Use package scanners to warn about obsolete, unmaintained, or insecure dependencies. Use complexity metrics, and measure test coverage.

That frees the humans to spend time on the human stuff.

Assume Good Faith: Comments are generally well intentioned

Most of the following suggestions are about how to write helpful and supportive code reviews, but you as the receiver of a review can help a lot too…

Generally people want the team to succeed, they want you to succeed, and they want us all (themselves included) to get better at what we do. Sometimes that's not always clear in the way things are hastily phrased.

Always read comments with the assumption that they are written in good faith!

Collaborate: A review is pair-programming

The author of code being reviewed has typically spent much more time exploring and working on the problem than any of the reviewers. This means the reviewer must try to reach the same level of understanding as the author. To do that, we can think of the review as a collaboration.

The reviewer and the author are peers collaborating on a draft of the code looking for ways to improve it. They are not teacher and student, or programmer and gatekeeper. If you're looking for a non-technical parallel then the relationship in publishing between an author and editor might be a good one to consider.

The motivation of both the author and the reviewer is to reach the best code for your situation.

A review is pair-programming, either synchronously with a realtime review, or asynchronously with a pull request. When you think of it as pair-programming, it becomes more obvious to make suggested edits to the code that the author can simply accept and merge, rather than having a back-and-forth in messages, talking at cross-purposes. Use the code to have the conversation.

Defenceless Review: You are not your code

It's common, and normal, to feel like we have to defend our code, and our choices. Adopting a defensive response to review comments is the opposite side of the coin to a reviewer making attacking comments. If we assume good intent, and that the reviewer and author are pair-programming, we can avoid adopting that defensive position.

The code we write is a reflection of ourselves and our ability in that moment, so this can be difficult, and you can defend your choices and code as you may know more than the reviewer. Adopting a pair-programming stance, as peers, you both have the opportunity to find the best solution, and for you both to learn.

Revert to Pair Programming: PRs are not the only way…

We can consider a PR to be asynchronous pair-programming, and thinking about it that way helps us adopt a better collaborative stance. Async communication has many benefits, but sometimes it might be easier, and better, to adopt a different review approach. If a PR seems like it's not the right approach at all then maybe a synchronous pair-programming session would be more productive?

Authority: Everyone's special

We don't all have the same experience, or the same amount of experience, or the same quality of experience. The author of the code knows some things, and the reviewer of the code knows some things. Often a perceived conflict is because one of us is missing information the other knows. This is how to consider authority in a peer-review context.

But… suggesting everyone's opinion is equally valid is also not true. Different members of the team will have different depths of experience in particular areas and that experience should often be recognised. A junior engineer with just a year or two experience does not have the same view of code quality as an experienced senior engineer who has worked with the same codebase for many years — but a junior frontend engineer may know better how to make code choices in a react app than a more experienced backend engineer with little frontend experience.

Both the author and reviewer must approach the review with open minds.

Clarifying Review Comments: That's not what I meant…

When we leave written comments it's easy for them to be ambiguous. There are a few techniques that can help…

Use code changes to illustrate your comments

Code is how we communicate with each other as engineers. The code we write is a description of the solution to a problem and we write the code as best we can for the next engineers to read.

The most efficient way for us to communicate with each other as engineers is often through code itself. That's what a PR is, after all, a set of unambiguous changes to our written description.

If the PR is the most efficient way for us to communicate changes, then we can use the same technique to suggest changes to the PR itself. If the change is simple, in one place, make the change and submit it as a suggestion on the PR. A suggestion allows the author to see exactly what you mean, and accept it automatically if they agree.

A code suggestion is the most efficient comment for small, clear, changes.

Replace ambiguous identifiers and reasons with clear statements:

Often, we use it, this, that, here in our writing and it is clear to us what we meant at the time of writing. These words are often unclear to someone else reading the comment later i.e. which this we meant.

We can make this our comments clearer by replacing the ambiguous word with an explicit identifier.

Use prefixes to clarify intent

Having made our comments much clearer by using code, and more specific language, we can also clarify how important each comment is. It's often difficult for the reader to get a sense of how much the reviewer cares about each change or suggestion.

To provide a better indication of the response we're expecting (from no response needed all the way to a required change) we can prefix our comment. Here's a scheme that my recent employer was using and has kindly said I can publish: