Skip to main content

Code Review

Code review is the single highest-leverage quality practice a team can adopt. It catches bugs before they reach production, spreads knowledge across the team, and raises the bar on design over time. Done well, it's a conversation between professionals. Done poorly, it's a bottleneck that breeds resentment.

Why this matters​

Code review is where S&P's values of Merit, Teamwork, and Care intersect with engineering. Every review is a chance to make the codebase better, share knowledge with a colleague, and catch something that would have cost 10x more to fix in production. Teams that review well ship faster, not slower, because they spend less time debugging, less time onboarding, and less time arguing about "why was this built this way?"

The standard​

What code review is for​

Code review serves three purposes, in this order:

  1. Correctness: Does this change do what it claims to do? Are there bugs, edge cases, or failure modes the author missed?
  2. Knowledge sharing: Does the rest of the team understand what changed and why? Will someone be able to maintain this in six months?
  3. Quality and consistency: Does this follow the team's conventions? Is it as simple as it can be? Will it be easy to change later?

Code review is not for:

  • Catching formatting issues (that's the linter's job)
  • Debating architectural decisions after the code is written (that's what decision records and up-front design are for)
  • Demonstrating how much the reviewer knows (that's ego, not care)

PR size and structure​

Small PRs get better reviews. This isn't a preference, it's a fact. Reviewers' attention degrades sharply after about 400 lines of diff. A 1,500-line PR will get a rubber stamp, not a review.

What we recommend:

  • Target under 400 lines of meaningful diff (excluding generated files, lock files, and migrations). If you can't get below 400, add a PR description that tells the reviewer where to focus.
  • One concern per PR. A feature is one PR. The refactoring that enabled it is a separate PR that lands first. A bug fix discovered along the way is yet another. Mixing concerns makes the diff harder to understand and the change harder to revert.
  • Stack PRs when the feature is large. Break the work into a chain where each PR is independently mergeable and reviewable. Use the PR description to link the chain and explain where each piece fits.
  • Draft PRs for early feedback. If you want a sanity check on your approach before the code is complete, open a draft PR. Explicitly label what kind of feedback you're looking for ("Is this the right API shape?" is different from "Please review this for merge").

What reviewers look for​

Not every PR needs every kind of review. Use this as a mental checklist, not a gate:

Always check:

  • Does the code match the PR description? If the description says "add pagination to the users endpoint" and the diff also refactors the auth middleware, that's a red flag: it should be split.
  • Are there obvious bugs, race conditions, or unhandled error cases?
  • Are new dependencies justified? Every new library is a maintenance commitment.
  • Is the naming clear? Could you understand this code without the PR description?

Check when relevant:

  • Security: Is user input validated and sanitised? Are secrets kept out of the code? Are there SQL injection, XSS, or IDOR risks? (See the Security section when available.)
  • Performance (Are there N+1 queries, unbounded loops, missing pagination, or unnecessary re-renders? Only flag performance when there's a plausible path to a real problem) don't micro-optimise.
  • Tests: Does the change include tests? Do the tests actually verify the behaviour, or do they just assert that the code runs? Are edge cases covered?
  • API contracts: For changes to REST endpoints or GraphQL schemas: will this break existing clients? Is it backward-compatible? Does it follow the team's API conventions?

Don't check (leave to automation):

  • Formatting, import order, semicolons, that's ESLint + Prettier or Biome, even OpenAPI has a linter.
  • Type correctness, that's TypeScript.
  • Dependency vulnerabilities, that's Snyk / npm audit in CI.

How to give feedback​

The way you phrase feedback determines whether it lands as helpful or hostile. This matters more than most engineers think.

Prefix your comments with intent:

  • nit:: A stylistic preference. Not blocking. The author can take it or leave it.
  • suggestion:: A concrete alternative you think is better. Include the code.
  • question:: You're genuinely asking, not passive-aggressively hinting. "Why did you choose X over Y?" is a real question; make sure it reads as one.
  • blocking:: This must be resolved before merge. Use sparingly. Reserve it for bugs, security issues, or violations of agreed standards.

Ground rules:

  • Comment on the code, not the person. "This function is doing too many things" rather than "You wrote a function that does too many things."
  • Explain why, not just what. "This could cause an N+1 query in production because the ORM will lazy-load each association" is useful. "Don't do it this way" is not.
  • Suggest, don't dictate. Unless it's a bug or a security issue, offer your alternative as an option: "Have you considered X? I think it would handle the edge case at line 42 more cleanly."
  • Acknowledge what's good. If the approach is clever or the tests are thorough, say so. Review isn't only about finding problems.
  • Limit nits. If you have more than three nits on a PR, pick the most impactful one or two. Nobody benefits from a wall of style preferences.

How to receive feedback​

  • Assume good intent. The reviewer is trying to make the code better, not attack you.
  • Don't take it personally. The code is not you. A suggestion to restructure a function is not a comment on your ability.
  • Respond to every comment. Even if it's just "done" or "good point, fixed." Unanswered comments create ambiguity about whether the feedback was seen.
  • Push back when you disagree: with reasoning. "I considered that, but I went with this approach because..." is exactly what code review is for. If the discussion stalls, bring in a third opinion or the relevant DRI.
  • Don't rewrite your PR in response to review. If the feedback is so fundamental that the approach needs to change, close the PR, have a design discussion, and start a new one. Iterating a fundamentally wrong approach through review comments wastes everyone's time.

Turnaround expectations​

Respond to a review request within one working day. This doesn't mean you need to complete the review: it means you acknowledge it, give an ETA if you're busy, or suggest someone else if you're not the right reviewer.

Complete the review within two working days. If a PR sits unreviewed for longer, it blocks the author, the diff grows stale, and merge conflicts accumulate.

As an author, respond to feedback within one working day. Don't let a PR sit in "changes requested" for a week. If you need more time, say so.

What to do when the reviewer is busy:

  • Wait one working day, then ping them directly (Slack, not a passive comment on the PR).
  • If they're still unavailable, reassign to another qualified reviewer or lean on the code review team (who are already on the PR).
  • If no one is available and the change is time-sensitive, escalate to the team lead / DRI or CTO.

Approval and merge rules​

  • Minimum one approval from a team member who did not author the PR. For critical paths (auth, payments, data migrations, infrastructure), require two approvals.
  • Add the code review team to every PR. The S&P code review team is a default reviewer on all pull requests. They provide a cross-project perspective that catches inconsistencies between teams and helps maintain standards across the organisation. This doesn't replace a domain-specific reviewer from your own team: it supplements it.
  • The author merges. The person who wrote the code is the person who merges it, after approvals are in. This keeps the author accountable for the final state of the code.
  • Squash merge by default into dev and them merge into upper branches (staging, main) or the target branch. This keeps the Git history clean and makes reverts straightforward. The squash commit message should be a clear summary of the change, not "squashed commit of 14 commits."
  • Delete the branch after merge. No exceptions. Stale branches are noise.
  • CI must pass before merge. No merging with red pipelines, no "it's probably fine", if the pipeline is flaky, fix the pipeline.

Critical thinking​

  • Review depth should match risk. A copy change in a tooltip doesn't need the same scrutiny as a change to the payment flow. Calibrate your effort to the blast radius of the change.
  • Don't confuse "I would have done it differently" with "this is wrong." Multiple valid approaches usually exist. Only block if the approach has a concrete problem, not because you'd have chosen a different pattern.
  • Team-size pragmatism. On a two-person team, one approval is fine. On a team where nobody else understands the subsystem, pairing or a walk-through may be more valuable than async review. The goal is meaningful review, not box-checking.
  • Review across teams when it matters. If a backend change affects the API contract that the frontend depends on, a frontend engineer should review it. Cross-team review catches integration issues that single-team review misses.
  • Automate what you keep repeating. If you find yourself writing the same review comment on every third PR ("add error handling for this API call", "use the shared pagination util"), that's a sign you need a lint rule, a template, or a better abstraction, not another comment.

Checklist​

For PR authors​

  • My PR has a clear title and description that explains what changed and why based on a common template.
  • The diff is under 400 lines (or I've explained why and guided the reviewer to focus areas).
  • I've self-reviewed the diff before requesting review. I caught the obvious stuff myself.
  • I've added or updated tests for the behaviour I changed.
  • CI is green.
  • I've linked the relevant Jira ticket.
  • I've added the code review team as a reviewer.

For reviewers​

  • I understand what this PR is trying to accomplish (if not, I asked before reviewing).
  • I checked for correctness, not just style.
  • I flagged anything that could break in production, affect security, or surprise a future reader.
  • My blocking comments are clearly marked and explain why they block.
  • I responded within one working day.

AI tips​

  • First-pass review: Before requesting human review, ask AI to review your diff for bugs, edge cases, and missed error handling. AI is good at catching mechanical issues (null checks, off-by-one errors, missing awaits) that humans glaze over in large diffs. Treat this as a pre-review sanity check, not a replacement for human review.
  • Understanding unfamiliar code: When you're reviewing a PR in a part of the codebase you don't know well, paste the relevant files into AI and ask it to explain the existing design before you review the change. You'll give better feedback when you understand the context.
  • Drafting PR descriptions: If you struggle with writing clear PR descriptions, describe the change verbally to AI and let it structure the description. A good PR description makes the reviewer's job dramatically easier.
  • Generating review comments: If you see a problem but can't articulate it clearly, describe the issue to AI and ask it to help you phrase the feedback constructively. This is especially useful for sensitive feedback.

Resources​