Code Review

Also Known As

TL;DR

Explanation

Code review is one of the most cost-effective quality practices: bugs caught at review are orders of magnitude cheaper to fix than in production. Effective reviews focus on correctness, security, design quality, and knowledge transfer — not style (automate that with linters). Reviewers should ask: Does this code do what it claims? Are edge cases handled? Are there security implications? Is it testable? Timely reviews (within hours, not days) and a respectful culture are essential. Pair programming can replace some async review in high-trust teams.

Common Misconception

Why It Matters

Common Mistakes

• Reviewing style and formatting instead of logic and design — automate style with linters, save human attention for intent.
• Approving large PRs without reading them carefully to avoid conflict — large PRs should be split, not rubber-stamped.
• Leaving vague comments ("fix this") without explanation or suggestion — reviewers should explain the problem and propose a direction.
• Treating code review as a gate rather than a conversation — the goal is shared understanding, not gatekeeping.

Avoid When

• Using code review as a gatekeeping ritual that blocks merges for trivial style issues — automate style with linters.
• Reviewing 1000-line PRs — large reviews are ineffective; keep PRs small and focused.
• Reviewing without running the code or tests — a review that only reads the diff misses runtime behaviour.
• Nitpicking in a way that demoralises authors — distinguish must-fix blocking issues from optional suggestions.

When To Use

• Every change to a shared codebase — a second pair of eyes catches bugs, design issues, and missing test cases.
• Enforcing architectural decisions and conventions that cannot be automated.
• Knowledge sharing — reviewers learn the codebase and authors learn from reviewer expertise.
• Security-sensitive changes — a dedicated security review catches vulnerabilities that functional review misses.

Code Examples

// Code review anti-patterns:
// Reviewer: 'LGTM' after 30 seconds on a 500-line PR
// No review checklist — misses security, performance, edge cases
// Author defensive about feedback — treats review as criticism
// PRs too large — impossible to review meaningfully
// No review culture — all PRs merged by author without review

// Better:
// PRs < 400 lines
// Checklist: correctness, security, tests, naming, edge cases
// Reviewer explains why, not just what to change

# Security-focused code review checklist (PHP)

# Authentication & Authorisation
[ ] Session regenerated after login
[ ] Every endpoint has authorisation check
[ ] Sensitive actions require re-authentication

# Input Handling
[ ] All user input validated before use
[ ] No raw user input in SQL queries
[ ] HTML output escaped with htmlspecialchars()
[ ] File uploads: MIME check + random filename + outside webroot

# Cryptography
[ ] Passwords: password_hash()/password_verify() — not md5/sha1
[ ] Tokens: random_bytes() — not rand()/uniqid()
[ ] Sensitive comparisons: hash_equals()

# Error Handling
[ ] display_errors = Off in production
[ ] No stack traces in API responses
[ ] Errors logged internally, not exposed

# Dependencies
[ ] composer audit passes with no critical CVEs

References

• ↗ https://google.github.io/eng-practices/review/

Tags

Related Terms