How to Write Better Code Review Comments
Two comments from the same pull request:
Could we pull this into a helper?
Might want to guard against a retry here.
One of those is a preference the author can take or leave. The other means a customer gets billed twice when the payment call times out on the first attempt.
Nothing on the screen separates them. They arrive in the same box, in the same font, with the same hedge in front of them, and the author has no way to know that one is a matter of taste and the other is a production incident waiting to happen.
Faced with a review where everything looks equally optional, the author either treats it all as mandatory and burns an afternoon, or treats it all as optional and merges the double charge.
Spotting a real defect in someone else's code is the hard part of reviewing, and this reviewer did it. Then they wrote it down in the same tentative register they use for suggestions about helper functions, and the information died somewhere between the comment box and the merge button.
Code reviews reliably transmit findings. They rarely transmit judgment, and judgment is the thing you were there to add.
The review only a person can give
Some review comments are mechanical. Formatting, unused imports, type errors, obvious style violations. Those should be settled by tooling before a human opens the diff. If your team is still arguing about them in pull requests, that is a tooling problem being handled in the wrong place.
The work that starts after the mechanical checks is the part that needs you:
- Does this fit the system we already have?
- Does it reuse what exists, or rebuild it?
- Does it fail in a way the author did not consider?
- Does it make the next change harder?
Answering those requires holding the system in your head, remembering what already lives in the codebase, and having a view about where it is going. No linter is going to do it for you.
It is also the part most reviews skip, because it is harder than reading a diff. A dozen comments about naming prove you read the diff. One comment saying "we already have a client for this, it's in billing/client.rb, and this rebuilds most of it" saves the team a month.\
Spend comments where judgment is needed
Every nitpick you leave spends a little of the credibility you will want on the day you leave the comment that matters.
Your comments compete for the same attention. The author has a finite amount of care to distribute across your feedback, and you decide where it goes.
The comments that add value:
- This duplicates something we already have. No tool will catch it. It requires knowing the codebase, which is the one thing you bring that automation does not.
- This breaks under a condition the author didn't consider. Empty inputs, concurrent writes, a retry against a call that isn't idempotent, a partial failure halfway through a multi-step write. You are the second brain on the problem. Spend it on the cases the first brain skipped.
- This makes something later expensive. A pattern that merges once gets copied ten times. Coupling that looks harmless at forty lines is a rewrite at four hundred. Name the future cost specifically, or the comment reads as taste.
- I don't understand this. The most underused comment in code review. If you know this system and still cannot follow the code, the next person will not either. Saying so feels like admitting you are slow. It is the review working.
- This is good, and here is why. Vague praise reads as filler. Tied to a specific decision, it tells the author which of their instincts to keep, and it is often the only signal they get that any of them were right.
Size is a bad test for what belongs in a review. count versus userCount is trivial in a ten-line function and meaningful when there are three counts in scope, with one of them resulting in the customer being charged.
A better test is whether you are willing to rank it. If a comment is too small to explain why it matters, it is probably too small to leave.
|
|
Where can AI save you time?
My friends at Big Creek Growth put together a quick survey to spot the repetitive work you can hand off to automation.
|
|
β
AI makes weak review habits more expensive
AI can produce code that looks reviewable before anyone has understood it. It follows the shape of the surrounding file. It uses familiar names. It arrives with tests, and the tests tend to confirm the implementation that the model has already chosen.
That surface polish is what makes shallow review dangerous. Code that looks wrong gets read carefully. Code that looks right gets a thumbs up.
The generated code will still rebuild a helper that lives three directories over, guard an edge case that cannot happen, and miss the one that will. It produces confident work with no stake in the system it lands in, and you still own what it writes.
Volume makes this problem worse. When anyone can open a six-hundred-line pull request in an afternoon, the review queue grows faster than the attention available to review it. The attention you do have has to land somewhere that counts.
Say what the comment is worth
In a room, you would have said, "This one's a bit of a preference, ignore it if you want," and ten minutes later, "no, stop, this will double-bill." Tone carried the ranking for free. In a pull request, both are grey boxes, and the meaning quietly falls out of the message.
Most teams have two available signals: approval and comment. :shipit: or a paragraph. Nothing in between.
Fixing that does not take a complicated system. Four labels cover most of it: blocking, question, suggestion, and note. The Code Review Emoji Guide is a fuller version of the same idea, using prefixes like π§ for a required change, β for a question that needs a real answer, π± for an idea worth keeping but not acting on now, π for something out of scope that should become a ticket, and β for a nitpick.
Whichever you pick, the label is doing two jobs. The author sees, in one pass, which comments stop the merge and which are for conversation. And you have to decide what you think before you type, which means a fair number of comments never get written at all.
The same finding, with and without weight:
Might want to guard against a retry here.
π§ Blocking. This retries on a 500, but chargeCard isn't idempotent. If the first attempt times out after the charge lands, we bill the customer twice. This needs an idempotency key or a different retry boundary before it merges.
Both comments caught the same thing. The second one tells the author that it stops the merge, and what would clear it.
One honest note about the guide. Its own advice on nitpicks is that they are usually better left unsaid. If you find yourself reaching for β regularly, check your linter configuration before checking your teammate.
Your reviews teach people how you think
Over the course of a year, you will leave a few hundred review comments. Nobody reads your design doc twice. Everybody reads your comments, one at a time, on the day they are trying to get something out the door.
That is the most widely distributed record your team has of how you think. Most engineers spend it proving they noticed the diff.
The reviewers people go looking for are the ones whose comments arrive already ranked. You know which one to fix first, and you know they had an opinion before they hit submit.
β