Code Review Practices

Overview

Code review is the practice of having one or more engineers read a proposed change before it merges. The goals are catching bugs, sharing knowledge, maintaining code quality, and ensuring consistency with architecture decisions. Effective reviews focus on correctness, clarity, and maintainability. Review culture significantly impacts team psychological safety.

Origin

Formal code inspection was developed at IBM by Michael Fagan in 1976. Peer review spread as a lighter alternative. Google's internal review tool Mondrian (2006) popularised rigorous web-based review. GitHub's pull request model (2008) democratised peer review for open source. "Code Review Best Practices" (Karl Wiegers) and Google Engineering Practices documentation are the primary references.

Examples

// BAD review comment - opinion without rationale:
// "I don't like this."

// BAD review comment - what but not why:
// "Use a Map here."

// GOOD review comment - explains the issue and offers a solution:
// "This object lookup (users[id]) is O(n) for iteration over all users
// to build the initial index. Switching to Map preserves O(1) lookup
// and makes the intent explicit. See MDN Map vs Object performance notes."

// GOOD - distinguishing blocking vs non-blocking feedback:
// [blocking] This will throw if response.data is undefined when the API
// returns 204 No Content. Add a null check or short-circuit before destructuring.

// [nit] Variable name 'data' is too generic here; 'confirmedOrders' would
// match the domain language and make the filter on line 42 self-explanatory.

// [question] Why does this need to be a class rather than a module-level
// function? Is there shared state I am missing?

// GOOD - praise specific patterns worth reinforcing:
// Nice use of Result type here instead of throwing; this makes the caller
// handle both paths explicitly and is exactly the pattern we discussed in RFC-14.

# Reviewing a PR locally with full context

# 1. Fetch the PR branch for local testing
# gh pr checkout 247

# 2. View PR diff with syntax highlighting
# gh pr diff 247

# 3. Add inline comments via gh (or via GitHub web UI)
# gh pr review 247 --comment --body "Concerns about N+1 on line 34"

# 4. Approve after addressing concerns
# gh pr review 247 --approve --body "LGTM after addressing the caching concern"

# 5. Request changes with a summary
# gh pr review 247 --request-changes --body "Please address the blocking comments"

# GitHub CLI v2.40+ supports inline comments:
# gh pr comment 247 --body "..." --line 34 --file src/services/order.ts

# Useful for automating review reminders in CI (post as PR comment):
# gh pr comment ${PR_NUMBER} --body "Tests passed. Ready for review."

Use Cases

• 01Knowledge sharing: junior engineers learn from reviewer comments; senior engineers see novel approaches from contributors they do not work with daily
• 02Bug prevention: studies (IBM, Microsoft Research) consistently show code review catches 60-90% of defects at a fraction of the cost of post-deployment bugs
• 03Architecture enforcement: reviewers ensure PRs align with established patterns (repository layer, error handling conventions) that are hard to encode in automated tools
• 04Documentation of intent: review comments and PR descriptions become a searchable record of why changes were made, more contextual than git blame alone

When Not to Use

• //Do not require review for trivial automated changes (dependency bumps, generated file updates); use a separate bot PR that auto-merges on green CI
• //Do not block a hotfix in production on a full review cycle; a lighter "one reviewer with fire drill context" approval process is appropriate for production incidents
• //Do not use reviews to enforce personal style preferences that are not captured in the agreed style guide; automate enforceable rules and leave reviews for logic and design

Technical Notes

• CODEOWNERS files (GitHub, GitLab) automatically assign reviewers based on file paths; teams own their own directories and are always included on PRs touching their code
• Google's study of code review at scale (Sadowski et al., 2018) found that most reviews have fewer than 10 files changed, take under an hour, and the most common review feedback is about code comprehensibility, not correctness
• Conventional reviewers focus on three layers: correctness (does it do what it claims), design (is this the right abstraction), and craft (will this be maintainable in 12 months). Addressing all three in a single pass without a checklist causes reviewers to focus only on the most visible layer
• PR size matters: Microsoft Research found that reviewers can effectively review about 200-400 lines of code per hour; PRs larger than 400 lines see significantly lower defect detection rates