Mistakes I See Engineers Making in Their Code Reviews

If tons of PRs are being blocked, it’s usually a sign that there’s too much gatekeeping going on.

Comments in an approval read like “this is great, just some tweaks if you want”. Comments in a blocking review read like “here’s why I don’t want you to merge this in”.

As a reviewer, when you come across cases where you would have done it differently, you must be able to approve those cases without comment, so long as either way is acceptable. Otherwise you’re putting your colleagues in an awkward position. They can either accept all your comments to avoid conflict, adding needless time and setting you up as the de facto gatekeeper for all changes to the codebase, or they can push back and argue on each trivial point, which will take even more time.

in normal circumstances, a high rate of blocked reviews represents a structural problem.

What do you do when there are twenty places in the diff that you’d like to see updated - for instance, twenty instances of camelCase variables instead of snake_case? Instead of leaving twenty comments, I’d suggest leaving a single comment explaining the stylistic change you’d like to make, and asking the engineer you’re reviewing to make the correct line-level changes themselves.

Probably my most controversial belief about code review is that a good code review shouldn’t contain more than five or six comments.

Code is now easy to generate using LLMs, but it’s still just as hard to review. Many software engineers now spend as much (or more) time reviewing the output of their own AI tools than their colleagues’ code.

One dynamic I’ve seen play out a lot is where one team owns a bottleneck for many other teams’ features

An approval should mean “I’m happy for you to merge, even if you ignore my comments”. Just leaving comments should mean “I’m happy for you to merge if someone else approves, even if you ignore my comments.”

Code review is not the time for you to impose your personal taste on a colleague.

But one of my strongest opinions about software engineering is that there are multiple acceptable approaches to any software problem, and that which one you choose often comes down to taste.

When you receive a review with a hundred comments, it’s very hard to engage with that review on anything other than a trivial level.

one of the most straightforwardly useful comments is “you don’t have to add this method here, since it already exists in this other place”. The diff itself won’t help you produce a comment like this.