Software Engineering

Code Review Best Practices

Effective code review processes that improve code quality, spread knowledge, and maintain team velocity without becoming a bottleneck.

⏱ 9 min read

What it is

Code review is the systematic examination of source code changes by one or more developers other than the author before those changes are merged into the main branch. It serves multiple goals simultaneously: catching defects, spreading knowledge across the team, maintaining consistency with architectural and coding standards, and providing a check against security and performance regressions. Modern code review is primarily done asynchronously through pull requests (PRs) or merge requests (MRs), though synchronous pair reviews remain valuable for complex changes.

Effective code review is as much a cultural practice as a technical one. The most technically rigorous review process fails if it creates adversarial dynamics, slows delivery to the point where developers bypass it, or if reviewers rubber-stamp PRs to avoid conflict. Google's internal research (Caitlin Sadowski et al., 2018) identified that developers value code review most for catching logic errors and knowledge sharing, not style enforcement — which should be automated.

Why it exists

Studies consistently show that code review is one of the most cost-effective defect detection techniques available. IBM research from the 1990s found that inspections removed 60–90% of defects before testing. More recent data from Microsoft shows that code reviews find defects that automated tests miss — particularly design-level issues, missing error handling, and security vulnerabilities. Beyond defect detection, review spreads code ownership across the team, reducing bus-factor risk.

Code review also serves as a forcing function for good practices: knowing that code will be reviewed tends to improve the quality of code written. Authors self-review more carefully, write clearer variable names, and add comments where reasoning is non-obvious. This "audience effect" is a significant side benefit of mandatory code review cultures.

When to use

All production code changes, regardless of size — even one-line changes have caused significant incidents.
Infrastructure as code (Terraform, Kubernetes manifests) changes where errors can affect availability.
Security-sensitive code paths — authentication, authorisation, cryptography, input validation.
Public API changes where breaking backwards compatibility has downstream consequences.
Knowledge transfer when onboarding new team members — reviewing their PRs and explaining rationale.
Architecture boundary changes — code that modifies how modules interact or introduces new dependencies.

When not to use

Generated code: Auto-generated files (migration scripts from ORM, protobuf generated code) should be excluded from review; focus on the generators and schema files instead.
Style enforcement: Automated linters and formatters handle style; code review bandwidth should not be spent on formatting debates.
Emergency hotfixes: P0 incidents may require a post-hoc review; blocking on review during an outage is counterproductive.
Personal experiments on feature branches: Work-in-progress commits should not trigger reviews; draft PRs signal this boundary.

Typical architecture


REVIEW LIFECYCLE
────────────────
  Author                    Reviewer(s)              CI System
    │                           │                        │
    ├─ Self-review first        │                        │
    ├─ Run lint/tests locally   │                        │
    ├─ Open PR (draft if WIP) ──►                        │
    │                           │  ◄─ CI runs: lint,    │
    │                           │     tests, security,  │
    │                           │     coverage checks ──┤
    │  ◄── Review comments ─────┤                        │
    ├─ Address feedback         │                        │
    ├─ Request re-review ───────►                        │
    │  ◄── Approval ────────────┤                        │
    ├─ Merge (squash/rebase) ───────────────────────────►│
    │                                                    │

PR SIZE GUIDELINE
─────────────────
  < 200 lines  → ideal, fast review (20-30 min)
  200-400 lines → acceptable, may need splitting
  400-800 lines → split if possible; requires focused session
  > 800 lines  → reviewer fatigue, missed bugs, slow feedback

AUTOMATED CHECKS (run before human review)
──────────────────────────────────────────
  ✓ Linter / formatter (ESLint, Prettier, Spotless)
  ✓ Unit & integration tests
  ✓ Static analysis (SonarQube, CodeQL, Semgrep)
  ✓ Security scanning (Snyk, Trivy, Dependabot)
  ✓ Coverage delta check (no decrease below threshold)

Pros and cons

Pros

Catches logic errors, security vulnerabilities, and performance issues before production.
Spreads code ownership and domain knowledge across the team, reducing bus-factor risk.
Improves code quality through audience effect — authors write more carefully knowing it will be reviewed.
Enforces architectural consistency and shared standards.
Provides documentation trail of why decisions were made (preserved in PR comments).

Cons

Adds latency to the development cycle if reviewers are slow to respond.
Can become a bottleneck if review responsibilities are not distributed across the team.
LGTM culture — perfunctory approvals without genuine review — provides false security.
Style debates waste reviewer and author time when not automated.
Can create power dynamics where senior developers gatekeep rather than mentor.

Implementation notes

PR size and scope: The 400-line rule of thumb (reviewed by SmartBear in a large study of Cisco code reviews) reflects the finding that reviewers lose effectiveness after reviewing about 400 lines in a single session. Above that, defect detection rate drops sharply. Enforce PR size through team norms and optionally through CI checks that warn when diffs exceed thresholds. Feature flags enable merging incomplete features in small increments while keeping main always deployable.

Feedback tone: Google's engineering team recommends the convention of prefixing review comments with their intent — "nit:" for minor style preferences, "question:" for genuine uncertainty, "suggestion:" for non-blocking improvements, and blocking comments stated as such. The Conventional Comments specification (conventionalcomments.org) formalises this approach. Ask questions rather than asserting ("Why is this cast needed here?" vs "Remove this cast") — questions invite explanation and may reveal context the reviewer lacked. The author is always closer to the context; reviewers should question, not override, unless the concern is clear.

Common failure modes

LGTM culture: Approving PRs without genuine review to avoid conflict or meet velocity metrics — the worst outcome because it provides false confidence.
Review by committee: Requiring 5 approvals for every change creates a bottleneck and diffuses responsibility; one focused review is often more effective than five perfunctory ones.
Style enforcement in review: Spending review cycles on formatting, naming, and whitespace that should be handled by automated tools is a waste of reviewer attention.
Stale PRs: PRs open for more than 48 hours without progress cause merge conflicts, lose context, and demoralise authors; set explicit SLA expectations.
No pre-review automation: Reviewers spending time on issues that linters and automated tests would catch — automated checks should run and pass before a human reviewer is assigned.

Decision checklist

Does your PR have automated checks (lint, tests, security scans) that must pass before human review?
Is the PR small enough for a reviewer to thoroughly review in under 45 minutes (< 400 lines)?
Has the author self-reviewed and addressed obvious issues before requesting review?
Does the PR description explain the "why" of the change, not just the "what"?
Are review comments framed as questions or suggestions rather than demands?
Is there a defined SLA for review response time (e.g., within 1 business day)?

Example use cases

Security-sensitive change: A PR adding authentication middleware requires review by at least one team member with security expertise; automated SAST scanning runs first to catch common vulnerabilities.
Database migration: Schema changes require review confirmation that the migration is backwards-compatible (old and new code can both run) and has a rollback plan.
New team member onboarding: First PRs from new hires are reviewed with mentoring intent — explaining patterns, pointing to relevant documentation, and building shared understanding.

Test-Driven Development — Tests provide reviewers with executable evidence that the code does what it claims.
Technical Debt Management — Code review is a point of debt prevention; approved debt should be logged in the tech debt registry.
API-First Development — API design review should happen on the OpenAPI spec before implementation begins, not on the implementation PR.