Code review is the single highest-leverage habit a team can have, and it is also the one most often done badly. I have been on the receiving end of reviews that felt like an interrogation and reviews that approved 800 lines with a thumbs-up emoji in four seconds. Both are failures. A good review catches real problems, spreads knowledge, and leaves the author feeling supported rather than judged.
Review for the things humans are good at
Do not spend your attention on formatting, import order, or whether a line is too long. A linter and an auto-formatter handle those, and they never get tired or grumpy. If your team argues about style in PR comments, you have a tooling gap, not a discipline problem. Configure the formatter, commit the config, and move on.
What machines cannot check is whether the code is correct, whether it solves the actual problem, and whether someone six months from now will understand it. That is where my attention goes:
- Does this do what the description says it does?
- What happens at the edges: empty input, nulls, concurrent access, a network call that hangs?
- Is there a simpler approach hiding behind this one?
- Will the naming make sense to someone who was not in the room?
Keep changes small enough to actually review
The hard limit on review quality is size. Research and my own experience both say the same thing: past a few hundred lines, defect detection falls off a cliff because reviewers skim. When I get a giant PR, I ask the author to split it. A series of small, focused PRs gets genuine scrutiny on each one. This is also why I rebase and squash carefully before opening a PR, a habit I described in git workflow best practices.
Comment like a colleague, not a compiler
Tone is most of the job. The same point lands completely differently depending on phrasing. I ask questions instead of issuing verdicts, and I make it clear which comments are blocking versus optional.
# Instead of:
This is wrong.
# Try:
What happens here if items is empty? I think
we'd index past the end. Could we guard with a
length check, or is that case impossible upstream?
# And label the small stuff:
nit: could inline this, non-blockingMarking nits as non-blocking is a small thing that removes a huge amount of friction. The author knows what they must fix to merge versus what is just my taste. I also make a point of saying when something is genuinely good. A review that is only criticism trains people to dread the process.
Review promptly and finish what you start
A PR sitting unreviewed for two days blocks a person and rots as main moves underneath it. I treat review requests as near the top of my queue, ideally same day. The cost of a stalled PR compounds: the author context-switches away, then has to reload everything when feedback finally arrives.
When I review, I try to give all my feedback in one pass rather than dribbling comments out over three rounds. Nothing is more demoralizing than fixing everything, getting re-reviewed, and discovering five new comments that were always visible.
The author has homework too
Reviews go faster when the author makes them easy. Before I request review I write a description that explains what changed and why, I leave inline comments on my own diff pointing out anything non-obvious, and I make sure CI is green. A good description can cut review time in half because the reviewer is not reverse-engineering intent from the diff.
- State the problem, not just the solution.
- Call out anything you are unsure about and want eyes on.
- Include screenshots or sample output for anything user-facing.
Disagree, then commit
Sometimes the author and reviewer just see it differently. When the disagreement is about taste rather than correctness, I default to the author’s call after voicing my view once. Dragging a PR through five rounds over a stylistic preference burns goodwill that you will want later. Save the firm stands for things that actually matter: correctness, security, and the data contracts I care so much about in REST API design guidelines. Get those right and let the small stuff go.