Code Review Checklist

Code Review Checklist

Overview

A comprehensive multi-dimensional code review checklist covering correctness, security, performance, maintainability, and style. Includes severity ratings, comment templates, and automation workflows for pre-commit and CI integration. Designed to produce consistent, actionable, and respectful code reviews.

When to Use

• Reviewing a pull request from a teammate
• Performing a self-review before opening a PR
• Auditing code for a production release or security review
• Onboarding new reviewers to team standards
• Setting up automated review gates in CI

Workflow

Phase 1: Pre-Review Preparation

# 1. Understand the PR context
gh pr view <number>          # GitHub CLI: see description + status
gh pr diff <number>          # Full diff
gh pr review <number> --request-changes  # Or --approve / --comment

# 2. Check CI status
gh pr checks <number>

# 3. Fetch the branch locally for deeper analysis
gh pr checkout <number>

# 4. Run automated checks locally
pytest && echo "Tests pass" || echo "Tests fail"
flake8 . && echo "Lint clean" || echo "Lint issues"
mypy . && echo "Types pass" || echo "Type issues"

Phase 2: Correctness (Severity: 🔴 BLOCKER unless fixed)

Logic & Edge Cases:

• Does the code handle the happy path correctly?
• Are edge cases covered? (empty lists, null values, max/min boundaries, negative numbers)
• Are error states handled explicitly? (network failures, file not found, permission denied, timeout)
• Are race conditions possible? (shared state without locks, non-atomic read-modify-write)
• Are off-by-one errors present? (loop bounds, array indices, slice ranges)
• Does the code fail fast on invalid input? (validate early, don't propagate bad data)
• Are transactions properly committed/rolled back on error?
• Are resource leaks possible? (file handles, DB connections, network sockets, locks)

# 🔴 BLOCKER: No input validation
def process_user(user_id, amount):
    db.execute("UPDATE users SET balance = balance - %s WHERE id = %s",
               (amount, user_id))
    # What if user_id doesn't exist? What if amount > balance?

# ✅ CORRECT: Validate first, then act
def process_user(user_id, amount):
    if amount <= 0:
        raise ValueError("Amount must be positive")
    user = db.fetch_one("SELECT id, balance FROM users WHERE id = %s", (user_id,))
    if not user:
        raise ValueError(f"User {user_id} not found")
    if user['balance'] < amount:
        raise ValueError("Insufficient balance")
    db.execute("UPDATE users SET balance = balance - %s WHERE id = %s",
               (amount, user_id))

Phase 3: Security (Severity: 🔴 BLOCKER unless fixed)

Injection & Validation:

• Are all user inputs validated against a whitelist? (never blacklist)
• Is SQL injection prevented? (parameterized queries only — no f-string interpolation)
• Is NoSQL injection prevented? (proper MongoDB/Sanitize for NoSQL queries)
• Is command injection prevented? (avoid os.system(), subprocess(shell=True))
• Are path traversal attacks prevented? (validate file paths, no user-controlled paths)
• Is XSS prevented in rendered output? (escape HTML/JS context appropriately)

Authentication & Authorization:

• Are all endpoints properly authenticated?
• Is authorization checked per resource? (user A shouldn't access user B's data)
• Are secrets and keys kept out of code? (no hardcoded API keys, passwords, tokens)
• Are API keys and tokens properly scoped? (least privilege principle)
• Is rate limiting applied to sensitive endpoints? (login, password reset, payment)

Data Protection:

• Is sensitive data (PII, passwords, tokens) not logged?
• Are passwords properly hashed? (bcrypt, argon2 — not md5, sha1, or plaintext)
• Are secrets in URLs avoided? (don't pass tokens in query params — use headers)

# 🔴 BLOCKER: SQL injection vulnerability
query = f"SELECT * FROM users WHERE email = '{email}'"  # NEVER do this

# ✅ CORRECT: Parameterized query
query = "SELECT * FROM users WHERE email = %s"
cursor.execute(query, (email,))

# 🔴 BLOCKER: Hardcoded secret
API_KEY = "sk-abc123def456"  # NEVER hardcode secrets

# ✅ CORRECT: Environment variable
import os
API_KEY = os.environ["API_KEY"]

Phase 4: Performance (Severity: 🟡 WARNING — context-dependent)

Database:

• Are N+1 queries avoided? (look for queries inside loops)
• Are database indexes used for all WHERE/JOIN/ORDER BY columns?
• Are large result sets paginated? (never SELECT * without LIMIT)
• Are bulk operations used instead of individual inserts/updates?
• Is connection pooling configured correctly?

Compute:

• Are unnecessary computations avoided? (memoize expensive calls, cache repeated results)
• Are large objects avoided in hot paths? (don't serialize gigabytes to JSON)
• Are allocation-heavy patterns replaced with reuse? (object pools, array reuse)
• Is async/parallelism used for I/O-bound tasks? (network calls, file reads)
• Are memory leaks possible? (growing collections, closure references, event listeners)

# 🟡 WARNING: N+1 query pattern
users = db.query("SELECT id, name FROM users")
for user in users:
    # One query per user → N+1!
    orders = db.query(f"SELECT * FROM orders WHERE user_id = {user['id']}")

# ✅ PERFORMANT: Batch query
users = db.query("SELECT id, name FROM users")
user_ids = [u['id'] for u in users]
orders = db.query(
    "SELECT * FROM orders WHERE user_id = ANY(%s)",
    (user_ids,)
)
orders_by_user = defaultdict(list)
for order in orders:
    orders_by_user[order['user_id']].append(order)

Phase 5: Maintainability (Severity: 🟡 WARNING — improve if practical)

Readability:

• Are function names descriptive? (verbs for functions, nouns for classes)
• Are variable names meaningful? (avoid x, tmp, data, thing)
• Is each function doing one thing? (Single Responsibility)
• Are magic numbers replaced with named constants?
• Is deep nesting avoided? (3+ levels → extract method or early return)
• Are comments explaining WHY, not WHAT? (code shows what; comments explain reasoning)

Structure:

• Is the module/package structure logical? (related things grouped, unrelated separated)
• Are imports clean? (no wildcard imports, no circular dependencies)
• Are abstractions appropriate for the complexity? (not over-engineered, not under-engineered)
• Is error handling consistent? (same pattern throughout: return vs raise vs Option)

Testing:

• Are there unit tests for the new logic?
• Do tests cover edge cases (not just happy path)?
• Are tests independent? (no shared state, no order dependency)
• Are tests readable? (descriptive names, Arrange-Act-Assert pattern)
• Is test coverage at or above team threshold?

Phase 6: Style & Conventions (Severity: 🟢 NICE-TO-HAVE)

• Does the code follow the team's formatter? (black, ruff-format, prettier, gofmt, rustfmt)
• Are type hints used where the language supports them?
• Are docstrings present on public APIs?
• Does the code match the project's existing style? (consistency over perfection)
• Are unused imports/variables removed?
• Are TODO/FIXME comments tracked in issue tracker?

Phase 7: Review Comment Templates

## Severity 🔴 BLOCKER
"User input is interpolated directly into a SQL query on line 42.
This is a SQL injection vulnerability. Please use parameterized
queries: cursor.execute(sql, (user_input,)) instead of
cursor.execute(f\"{sql}\")."

## Severity 🟡 WARNING
"There's an N+1 query pattern starting on line 87 — the database
is queried once per iteration of the user list. Consider batching
the queries with a single SELECT ... WHERE id = ANY(%s)."

## Severity 🟢 NICE-TO-HAVE
"The variable name 'd' on line 15 isn't descriptive. Consider
renaming to 'response_data' or 'payload' to clarify what it holds."

## Positive feedback 🌟
"Great use of early returns on lines 10-12 to flatten the nesting
and make the happy path obvious. The error handling is clean."

Phase 8: Automated Review Gates

# .github/workflows/code-review.yml
name: Code Review Gates
on: [pull_request]

jobs:
  review:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4

      - name: Lint
        run: |
          pip install ruff
          ruff check --output-format=github .

      - name: Type Check
        run: |
          pip install mypy
          mypy src/

      - name: Security Scan
        run: |
          pip install bandit
          bandit -r src/ -f sarif

      - name: Complexity Gate
        run: |
          pip install radon
          radon cc src/ -n C | tee complexity.txt
          ! grep -q "F\|C" complexity.txt

      - name: Test Coverage Gate
        run: |
          pytest --cov=src/ --cov-fail-under=80

      - name: Dependency Audit
        run: |
          pip-audit

Common Pitfalls

• Focusing only on style: Style is the least important dimension. Spend review energy on correctness, security, and logic.
• Rubber-stamping: Approving without understanding the code. If you can't explain what a PR does, you haven't reviewed it.
• Subjective gatekeeping: "I would have written it differently" is not a review comment. Judge by correctness and maintainability, not personal preference.
• Bike-shedding: Spending too much time on trivial issues (naming, formatting) while missing real bugs.
• Reviewing too late: Code review should happen within hours, not days. Stale reviews block progress.
• Too many comments per review: Focus on the 3-5 most important issues. Flooding a PR with 50 comments is demoralizing.
• No positive feedback: Reviews aren't just for finding problems. Acknowledge good patterns and clever solutions.
• Not checking test quality: Looking at production code but ignoring tests. Tests are equally important to review.
• Reviewing giant PRs: Break large changes into reviewable chunks (<400 lines per PR recommended).

Verification Checklist

• Correctness: All edge cases considered, error states handled, no off-by-one
• Security: No injection vulnerabilities, auth checked per resource, no secrets in code
• Performance: No N+1 queries, pagination present, no obvious memory leaks
• Maintainability: Good naming, single responsibility, appropriate abstractions
• Style: Consistent with project conventions, formatted, type-hinted
• Tests: Cover the new logic, include edge cases, independent and readable
• Comments provided at appropriate severity levels (🔴 🟡 🟢 🌟)
• No bias toward personal preference — objective criteria only
• Reviewed within same business day (or within 24h)
• PR is of reviewable size (<400 lines), or chunked into manageable pieces
• Automated gates passed (lint, security scan, complexity, coverage)
• Reviewer understands the code path end-to-end