How to review code without being a jerk

Code review is the place where a team's culture is most visible. In a good review, the code gets better and both people learn something. In a bad one, the author feels judged, the reviewer feels ignored and the change is merged with both of them annoyed. The difference is rarely technical.

What a review is for

A review has three jobs, in this order.

1. Find mistakes that would hurt users
2. Keep the code understandable for the next person
3. Share knowledge between the author and the reviewer

Notice what is missing. A review is not the place to show that you would have done it differently, or to enforce taste that a formatter could enforce.

Before you comment, understand

Read the description. Read the ticket. Run the code if the change is large. Most bad review comments come from a reviewer who has not yet understood what the change is for.

If there is no description, ask for one. That is a fair first comment.

Ask, do not tell

Compare these two comments on the same line.

This is wrong. Use a map here.

Would a map work here? I think the lookup on line 40 is O(n) inside
a loop, which may hurt with large lists. I might be missing a reason
for the array.

The second is longer. It also explains the concern, leaves room for the author to know something you do not and ends with the same request. People change their code much more willingly when they understand why.

Write every comment as if the author will read it on their worst day of the month.

Jun Watanabe

Label your comments

Not every comment has the same weight. Say which kind it is.

With labels, the author can tell at once whether a review with fifteen comments means "one real problem" or "start again".

Praise, specifically

If something is well done, say so, and say what. "Nice" is noise. "This test covers the timezone case that bit us in March" tells the author what to keep doing.

Automate taste

Arguments about spaces, quotes and import order do not belong in a review. Choose a formatter and a linter, run them in the pipeline and never discuss them again.

{
  "scripts": {
    "lint": "eslint . --max-warnings 0",
    "format": "prettier --check ."
  }
}

If a rule matters enough to mention twice, make it a lint rule. If it does not matter enough to automate, it does not matter enough to block a merge.

Keep changes small

The best thing an author can do for a reviewer is send less. A change of 200 lines gets a careful review. A change of 2,000 lines gets a quick scroll and an approval.

• One purpose per pull request
• Separate refactoring from behaviour changes
• Put generated files and renames in their own commit

If a change must be large, walk the reviewer through it in the description: start here, then this file, ignore that one.

For authors

Review is a two-person job, and authors set the tone as much as reviewers.

• Review your own diff first. You will find a third of the problems.
• Explain the why in the description, not the what. The diff shows the what.
• Reply to every comment, even with "done".
• When you disagree, say so with a reason, and be ready to be wrong.
• Thank people for catching things. They saved you a bug report.

When you disagree

Two comments back and forth is a discussion. Six is a standoff. When a thread gets long, stop writing and talk for five minutes. Most disputes end in the first minute of a call, because tone is restored and the misunderstanding becomes obvious.

If you still disagree, decide who decides. On most teams, the author has the final say on style and approach, and the reviewer has it on correctness and safety.

What to look for, in order

1. Does it do what the description says?
2. What happens with empty, huge or hostile input?
3. Are errors handled, or ignored?
4. Is there a test that would fail without this change?
5. Will someone understand this in a year?
6. Names, structure, duplication.

Most of the value is in the first four. Many reviews spend all their time on the last one.

The point of all this

The code will be rewritten in three years. The habits your team builds while reviewing it will last much longer. Be the reviewer you would want on your own worst change.