Code review culture that helps teams

The problem: reviews that nobody enjoys

A Pull Request with 1,800 changed lines waits three days. The reviewer skims, writes "LGTM" and approves. A week later a bug appears in code the reviewer technically approved. Another team has the opposite problem: every Pull Request gets thirty comments about naming and brace style, the author feels attacked, and the next PR is bigger because "if I have to go through this, I will batch my changes." Both teams have reviews. Neither has a review culture.

What reviews are for

A review is a conversation with several purposes, in roughly this order of value:

1. Correctness and risk. Does it do what it claims? Are edge cases, failures and security handled?
2. Design fit. Does it match the architecture and avoid creating a problem for the next feature?
3. Shared knowledge. At least one other person now understands this code.
4. Readability and consistency. Could a newcomer maintain it?

Reviews are not the place to find formatting issues; a formatter and linter in CI do that without arguments. They are also not a gate where seniors prove they are smarter. Their purpose is to ship better software and spread understanding.

Small Pull Requests are the foundation

Review quality drops sharply as size grows, because the reviewer cannot hold the whole change in their head. Aim for a Pull Request that can be understood in one sitting and does one thing. Practical techniques:

• Separate refactoring from behaviour changes. Rename or move code in one PR, change logic in the next.
• Use feature flags or additive changes (new column, then code that uses it) so incomplete work can merge safely.
• Write a description that answers: what, why, how to test, and what you are unsure about.
• Review your own diff first. Many comments are things you would have caught in two minutes.

Comment kinds: say what you mean

Authors waste energy guessing whether a comment is mandatory. Agree on simple prefixes:

Praise is not decoration. It tells people which patterns to repeat.

Tone: from harsh to useful

The same finding can build or damage trust. Compare:

Harsh:
  This is wrong. Why would you catch Exception here?

Useful:
  blocking: catch (Exception e) will also swallow
  InterruptedException and programming errors, and the caller
  then sees an empty result with no trace of the failure.
  Could we catch the specific PaymentGatewayException, log it
  with the order id, and rethrow a domain exception?
  Example: see OrderService.cancel().

The useful version names the kind, explains the consequence, proposes a direction and points to an example. It comments on the code, not the person ("this method" instead of "you"). It asks a question when the reviewer might be missing context. Use "we" for shared ownership, and avoid sarcasm, which does not survive text.

Author responsibilities

The author owns the change until it is merged and running. That means:

• Keep the PR small and the description clear.
• Run tests and the formatter before requesting review.
• Respond to every comment, even with "done" or "disagree, because...".
• Do not take comments personally; they are about the change.
• Do not force-push over review history without telling reviewers what changed.

A short reviewer checklist

Use it as a prompt, not a bureaucracy.

• Does the code do what the description says?
• Are failure paths handled (timeouts, nulls, partial failure)?
• Are inputs validated and access checked?
• Is there a test that would fail if this behaviour broke? See testing strategy for backends.
• Are names, logging and error messages helpful to the person on call?
• Will database migrations and rollbacks be safe?
• Is anything surprising that deserves a comment or a decision record?

How to disagree

Disagreement is normal. A reasonable path:

1. Restate the other person's point to confirm you understood it.
2. Explain your reasoning with the concrete risk or benefit, not preferences.
3. If two rounds of comments do not converge, switch to a short call or chat; text escalates easily.
4. If it is a matter of taste, follow the existing team convention or let the author decide.
5. If it is a significant design choice, write the outcome down, for example as an ADR (see writing ADRs).

Ask who has the final say beforehand. For most teams it is the author for local choices and the tech lead for cross-cutting ones.

For a newcomer, review is also the fastest way to learn. Ask them to read other people's Pull Requests as well: it shows the code base, the reasons behind decisions and the team's expectations more vividly than documents do. As a reviewer, do not hesitate to ask questions; "why did you do it this way?" is not criticism but a tool for learning and clarification. Over time, such questions also reveal which decisions deserve a short written note.

If the same question keeps appearing in different PRs, it shows that a team rule is not written down, and it is time to turn it into a short team document or an ADR.

Common mistakes

• Reviewing only for style while ignoring logic and failure modes.
• Approving without reading because the author is senior or the deadline is near.
• Letting Pull Requests sit; slow review is a hidden tax on every engineer.
• One person reviewing everything, which creates a bottleneck and a single point of knowledge.
• Rewriting the author's solution in your style instead of reviewing it on its merits.
• Using review to settle architecture arguments that should have happened earlier.

Trade-offs

Fast turnaround and thorough review pull in opposite directions. A pragmatic arrangement is a response time target agreed by the team, plus more scrutiny for risky areas (payments, authentication, migrations) and lighter review for low-risk changes. Pair programming can replace some reviews and gives immediate feedback, but it does not leave a written trail. Automated checks reduce noise yet can create false confidence; humans still need to think about design.

Checklist for the team

• Maximum PR size or scope is agreed.
• Comment prefixes are documented.
• Formatting and linting are automated.
• Expected review response time is known.
• Reviewers rotate.
• Risky areas have extra attention.

Where to go next

• If you lead a team, continue with tech lead: the first 90 days.
• Strengthen what reviewers look for in testing strategy for backends.
• Check your level with the skills assessment.