10 Commandments of Code Review

There are many blogs that offer a “code review checklist” of technical problems for reviewers to watch out for. But few discuss code review as an interaction between human beings — people with feelings, and wants, and needs, and maybe even secret agendas. This blog post solves that problem! Here are ten commandments to help you review code in a way that earns you the respect and admiration of (and maybe a few warm fuzzies from) your teammates!

Commandments for Reviewers

1. Thou shalt be kind to thy author.

This is the Golden Rule of Code Review. It’s easy to make reviewing personal — to go on an ego trip and impose your will upon the code base “for the good of the team.” Resist the temptation! This is especially important if you’re more senior than the author and/or more familiar with the code they’re modifying. Remember, you want the author to like you as a coworker and to want to include you on their next code review!

Here are some ways to increase your kindness as a reviewer:

• Look for opportunities to say “great idea!” This is harder than finding things that are wrong, but it is helpful feedback, and it feels SO GOOD when you’re the author and you see this! (Admittedly I don’t do this enough!)
• Criticize code or ideas, not people. “This function has a bug,” not “your function has a bug.”
• Review your comments before submitting them, and ask yourself, honestly, whether you’d feel bad if you read this on a code review you authored.

2. Thou shalt speak up if thou dost not understand the code.

This is the most frequently violated commandment, which is why I put it second.

Engineers avoid speaking up for two reasons:

• They think they’ll look stupid if they admit they don’t understand.
• They think they’re not qualified to review the code if they don’t understand it.

Reader, get over your fear of looking stupid! Be better than the engineers who shrug and lurk when they see an incomprehensible diff! Remember that good code can be understood by both humans AND compilers.

This is especially important for engineers who are new to a team or code base. New engineers frequently ignore code reviews and leave them to established engineers. But engineers who are unfamiliar with the code are the best judges of whether the code is easy to understand! So, if you’re new and don’t understand a change, speak up! You’ll help the author write better code, and give them an opportunity to help you by explaining the more difficult parts of the code base.

3. Thou shalt ask questions more than thou demandest changes.

This goes back to the First Commandment of being kind and understanding the author’s intent. Asking a question gives the author a chance to convince you that their decision is better than yours — you’ll be surprised how often they convince you!

Here are some great questions to ask during code review:

• Can you explain why you chose A instead of B? I think B would work better because of X.
• What happens if variable X has value Y? I think that might be a bug, but maybe I just don’t understand.

4. Thou shalt get a third opinion if thou disagreest with thy author.

Once you start asking why the author chose A instead of B, you’ll notice that sometimes the author explains why they liked A better and you’re not convinced. The bad way to solve this problem is to argue for 20 minutes (or 20 days!) until one engineer “wins” and the other feels bad.

Instead, ask a third engineer to read both arguments and give their opinion. And — this is important! — the author and reviewer should agree in advance to let the third engineer have the final word. Once I was the third engineer and, after I decided, the “losing” engineer doubled down and continued to argue with both of us! Predictably, this made us pretty upset with him and all three of us didn’t get anything else done that afternoon.

5. Thou shalt not pile on.

The surest way to make an author feel bad is to have too many engineers come into their code review and all request different changes. It also makes the code review take forever, because the author has to discuss and get approval from so many people. If you’re about to become the third reviewer to request changes on a code review, consider staying quiet instead. If you’re the fourth, you should almost always stay away. Trust the first two reviewers to do a good job.

6. Thou shalt not be a linter.

A linter is a program that puts code through a set of “rules” and then comes up with a list of “problems” that can be detected programmatically. Frequently they are enforcing style rules, like whether an opening brace should be at the end of a line or at the beginning of a new line.

Here are three reasons why you should not lint during code review!

• Linter programs can lint much more quickly, easily, and accurately than you, a meat sack made of soft and mushy and unreliable hardware.
• You should be focused on bigger and more important problems that can’t be identified by a dumb machine.
• Most linter problems are not worth having a back and forth about. Making the author “improve” the position of an opening brace can make them resent you and feel that you don’t respect their time.

Instead of linting during code review, set up a linter for your teammates that runs automatically before they get their change to the code review stage. This will save everyone’s time and feelings!

7. Thou shalt consider allowing the team to fix the problem later.

It’s easy to think “I can’t allow this problem to get into our code base.” But sometimes it’s better to take something as a tech debt problem for later. Look for the following combination:

• Leaving it the way it is isn’t that bad.
• Fixing it would take a long time.

In this case, insisting the author fix the problem immediately will slow down their work, and frequently will make them feel frustrated and resentful towards you. Instead, ask the author to create a tech debt ticket and link the ticket in a TODO comment (and, of course, make sure your team is regularly setting aside time to work on tech debt!). Once that’s done, you can approve the change.

Commandments for Authors

8. Thou shalt test thy changes before code review.

I’ve asked every team I’ve been on to require authors to explain how they tested the change in the discussion/description section of every code review. Multiple teammates have liked this rule so much that, after they left my team, they asked their new team to adopt this rule!

Authors like this rule because it reminds them to follow the team’s testing process. Authors who are new to the team like it because it helps them learn the team’s testing process and expectations. Reviewers like it because it lowers the risk of becoming the reviewer who approved a change that broke everything!

9. Thou shalt self-review before requesting peer review.

I personally forget this far too often, ESPECIALLY for small changes that for some reason I imagine I cannot possibly mess up. Just last week I messed up a git rebase and pulled in a ton of other people’s commits into what should have been a one-line change. I’d have saved myself a lot of embarrassment and my reviewers a bit of time by looking at the diff first. Don’t be that author!

10. Thou shalt not submit diffs larger than 200 lines of code.

Research shows reviewers catch more bugs when code reviews are smaller — around 200 lines of code is the threshold. This makes intuitive sense — we’ve all reviewed huge changes and gotten bored and started half-paying attention after the first five or ten minutes of reading. Don’t do this to your reviewers! Instead, be intentional about splitting large changes into smaller changes before asking for code review.

This is a common problem for interns and other very junior engineers, so remind them of this rule before you send them off to work on their first big feature! More experienced engineers can be judicious about bending this rule occasionally.

The complete list

Inscribe these commandments on stone tablets and hang them near the coffeemaker so everyone has to look at them every day! If we ever go back to the office, that is.

Reviewers

1. Thou shalt be kind to thy author.
2. Thou shalt speak up if you dost not understand the code.
3. Thou shalt ask questions more than thou demandeth changes.
4. Thou shalt get a third opinion if thou disagreest with thy author.
5. Thou shalt not pile on.
6. Thou shalt not be a linter.
7. Thou shalt consider allowing the team to fix the problem later.

Authors

1. Thou shalt test thy changes before code review.
2. Thou shalt self-review before requesting peer review.
3. Thou shalt not submit diffs larger than 200 lines of code.

— —

This week’s piece is authored by:

Joe Klonowski
Manager, Software Development Engineering