Code Review: What to Look For and What to Leave Alone

Most review comments are about naming, formatting and style. Most production incidents are about concurrency, error handling, data volume and things the reviewer had no context for.

For a separate people-operations perspective, the supporting explanation covers documenting serious workplace conduct.

The gap is not a failure of diligence. It is that the easy things are visible in a diff and the expensive things are not.

Automate everything a machine can check

Anything a tool can find should never reach a human reviewer.

Formatting — a formatter, applied automatically, with no discussion. Style debates in review are pure waste and they are also unpleasant, which colours everything else in the conversation.

Linting, static analysis, type checking, dependency vulnerability scanning, test execution.

A review that spends its attention on what a linter would have caught has spent it badly, and the reviewer's remaining attention is finite.

What to actually look at

In rough order of what it costs when missed.

Does it do the right thing

Read the description and the tests first. If you cannot tell from them what the change is supposed to accomplish, that is the first comment.

Check the edge cases the author did not mention. Empty input, single element, maximum size, concurrent invocation, the operation running twice.

Check the error paths. These are where review adds the most value, because they are the paths least likely to have been exercised. What happens when the downstream call fails? Is the error swallowed? Is partial state left behind?

Failure and concurrency

What happens if this runs twice? Retries and redeliveries mean it will. See every request will be retried.

What happens if it fails halfway? Is there a partial write? Is the transaction boundary where it should be?

Is shared state accessed from more than one thread? And if so, is it protected.

Are there new timeouts, retries or unbounded waits? An unbounded wait is a future incident. See timeouts and retries.

Data volume

Does this loop over something that could be large?

Is there a query inside a loop? The N+1 pattern is easy to spot in review and expensive to find in production.

Does it load a whole result set into memory?

Will this query use an index? And is a new index needed — with the write cost that implies. See what happens on write.

Was this tested at realistic size? A change correct at a thousand rows can be an outage at ten million.

Security and data handling

Is user input used to build a query, a path, or a command?

Is authorisation checked, on this path? Authentication and authorisation are different, and the second is more often missed.

Are secrets or personal data being logged?

Are new dependencies justified, and what do they pull in transitively.

Operability

Will you be able to diagnose this at 3am? Are the log messages sufficient, do they carry a trace identifier, is there a metric for the thing that will go wrong.

Is it deployable and reversible? Can old and new code run simultaneously during the rollout — which they will. See schema migrations on a running system.

Is there a feature flag or another way to disable it without a deploy?

Then readability

Will someone unfamiliar understand it in a year? Names that describe intent, comments explaining why rather than what, complexity that is warranted.

Genuinely important, and it belongs after the above.

What to leave alone

Style a formatter handles.

Personal preference presented as a standard. If both forms are fine, the author's choice stands.

Rewriting it the way you would have done it. Unless there is a defect, a different approach is not a review comment.

Speculative generality. "You might want to extend this later" usually produces abstraction that never gets used and always has to be maintained.

Perfection on a change that improves things. A change that makes an area better should not be blocked because it did not make it ideal.

Making the mechanics work

Small changes. The most important structural factor and the one entirely under the author's control. Reviews of large diffs produce comments about naming, because that is what fits in a reviewer's working memory. Under 400 lines is a commonly cited threshold and the direction matters more than the number.

Review promptly. A change waiting two days has an author who has moved on and will address the comments with less context than they had.

Say what kind of comment you are making. "Blocking:", "Suggestion:", "Question:", "Nit:". This removes most of the ambiguity about what has to change, and it is the cheapest improvement available to any review culture.

Ask rather than assert. "What happens if this list is empty?" gets a better response than "this breaks on empty lists," and it is also right more often — the reviewer frequently lacks context.

Approve with comments where nothing is blocking. Holding a change for a nit wastes a round trip.

Talk instead of writing past three exchanges on one thread. A five-minute conversation resolves what ten comments will not.

For authors

Review your own diff first. A significant share of review comments would have been caught by reading it once before requesting review.

Explain why, not what. The diff shows what changed. The description should say why, what alternatives were considered, and what the reviewer should look at carefully.

Flag the risky part yourself. "The concurrency here is the part I am least sure about" directs attention where it is needed and it is a strong signal of a good author.

Separate mechanical from substantive changes. A rename touching 200 files should not be in the same change as a logic fix, because the logic fix becomes invisible.

What review is not for

Not for catching everything. Reviewers miss concurrency bugs, scale problems and integration failures reliably. Tests, staged rollout and monitoring cover what review cannot. See what tests actually catch.

Not for gatekeeping, and not for demonstrating seniority.

Not a substitute for design discussion. A fundamental disagreement about approach at review time means the conversation happened too late. That is a process finding, not a review comment.

The summary

Automate everything a tool can check, so the human attention goes somewhere it matters.

Look at correctness, failure paths, concurrency, data volume and operability — before readability.

Small changes, prompt reviews, and labelled comments do more for review quality than any checklist.

And the highest-value question a reviewer can ask is what happens when this runs twice, halfway, at ten times the volume.