Ryu0705

Posted on Mar 22

Code Review Checklist: 125 Things to Check Before Merging

#codereview #programming #bestpractices #productivity

Every developer has merged a PR and then immediately thought: "Wait, did I check...?"

After years of reviewing code across teams of various sizes, I've compiled a comprehensive checklist covering 125 items across 5 critical dimensions. This isn't theoretical — these are the things that actually cause production incidents, tech debt, and security breaches.

Here's the complete checklist, organized by category.

1. Code Quality (25 items)

Code quality issues are the #1 source of bugs that slip through review. These checks catch the mistakes that your linter won't.

Correctness

[ ] No logic errors (off-by-one, wrong operator, inverted condition)
[ ] Null/undefined access has proper guards
[ ] No incorrect type coercion
[ ] All functions have return statements where expected
[ ] No unreachable code after return/throw
[ ] Errors handled properly (not swallowed, correct catch blocks)
[ ] No race conditions in async code
[ ] All async functions are properly awaited
[ ] Closures don't capture stale references
[ ] No unintended state mutation

// Easy to miss: forgetting await
async function getUser(id) {
  const user = fetchUser(id); // BUG: missing await
  return user.name; // TypeError: cannot read property of Promise
}

// Fixed
async function getUser(id) {
  const user = await fetchUser(id);
  return user.name;
}

Patterns & Consistency

[ ] Code follows existing project patterns
[ ] No DRY violations (duplicated logic > 5 lines)
[ ] No premature optimization or abstraction
[ ] Design patterns used correctly (not forced)
[ ] Error types match project conventions
[ ] Naming conventions followed consistently

Edge Cases

[ ] Empty input handled
[ ] Null/undefined input handled
[ ] Single vs. multiple items handled
[ ] Boundary values handled
[ ] Concurrent access considered

Dead Code

[ ] No unused imports
[ ] No unused variables or functions
[ ] No commented-out code blocks
[ ] No unreachable branches

2. Security (25 items)

Security vulnerabilities are the most expensive bugs to fix after deployment. Every PR deserves a security pass.

Injection Prevention

[ ] SQL queries use parameterized statements
[ ] No eval() or new Function() with user input
[ ] No shell commands with user input
[ ] No innerHTML with user-supplied data
[ ] Template literals sanitized before rendering
[ ] LDAP/XPath queries properly escaped

// VULNERABLE: SQL injection
const query = `SELECT * FROM users WHERE id = ${userId}`;

// SAFE: Parameterized query
const query = "SELECT * FROM users WHERE id = $1";
db.query(query, [userId]);

Authentication & Authorization

[ ] All endpoints have auth middleware
[ ] Resource ownership verified on access
[ ] Passwords hashed with bcrypt/argon2 (not MD5/SHA)
[ ] Session tokens are cryptographically random
[ ] JWT tokens have reasonable expiry
[ ] Failed auth attempts are rate-limited

Data Exposure

[ ] No hardcoded secrets or API keys
[ ] Sensitive data not in logs
[ ] Sensitive data not in URL parameters
[ ] Error messages don't leak internals
[ ] Stack traces hidden in production
[ ] PII encrypted at rest

Input Validation & Headers

[ ] Server-side validation on all input
[ ] Input length limits enforced
[ ] File uploads validated (type + size)
[ ] CORS configured restrictively
[ ] CSRF protection on state-changing ops
[ ] Security headers set (CSP, HSTS, X-Frame-Options)
[ ] Cookies have HttpOnly, Secure, SameSite flags

3. Performance (25 items)

Performance problems rarely show up in development. These checks catch the issues that only appear at scale.

Database

[ ] No N+1 query patterns
[ ] Queries use appropriate indexes
[ ] No SELECT * when specific columns suffice
[ ] Pagination on list endpoints
[ ] Connection pooling used
[ ] Unnecessary JOINs avoided
[ ] Batch operations for bulk writes

// N+1 PROBLEM: 1 query + N queries
const orders = await db.query("SELECT * FROM orders");
for (const order of orders) {
  order.items = await db.query(
    "SELECT * FROM items WHERE order_id = ?", [order.id]
  );
}

// FIXED: 2 queries total
const orders = await db.query("SELECT * FROM orders");
const orderIds = orders.map(o => o.id);
const items = await db.query(
  "SELECT * FROM items WHERE order_id IN (?)", [orderIds]
);

API & Network

[ ] No unnecessary API calls
[ ] HTTP caching headers set
[ ] Payload size minimized
[ ] Large datasets use pagination or streaming
[ ] Timeouts on all external requests
[ ] Connection reuse (keep-alive)

Frontend

[ ] Unnecessary re-renders avoided
[ ] Large lists virtualized
[ ] Images optimized and lazy-loaded
[ ] Bundle size monitored
[ ] Code splitting for routes
[ ] No layout thrashing

Memory & Algorithms

[ ] No memory leaks (listeners cleaned up)
[ ] Large data released when done
[ ] Streams used for large files
[ ] No unbounded caches
[ ] No O(n^2) where O(n) or O(n log n) exists
[ ] Set/Map used for lookups instead of arrays
[ ] Early returns to skip unnecessary work

4. Maintainability (25 items)

Code is read 10x more than it's written. These checks ensure your future self (and teammates) can understand what's happening.

Readability

[ ] Function names describe behavior (verb + noun)
[ ] Variable names are descriptive (no x, temp, data)
[ ] No magic numbers — use named constants
[ ] No deeply nested code (max 3 levels)
[ ] Functions under 50 lines
[ ] Files under 300 lines
[ ] Complex logic has explanatory comments

// BAD: Magic numbers and unclear names
if (d > 30 && t === 2) { ... }

// GOOD: Named constants and descriptive names
const MAX_INACTIVE_DAYS = 30;
const ACCOUNT_TYPE_PREMIUM = 2;
if (daysSinceLastLogin > MAX_INACTIVE_DAYS 
    && accountType === ACCOUNT_TYPE_PREMIUM) { ... }

Structure

[ ] Single responsibility per function
[ ] Single responsibility per module
[ ] Clear separation of concerns
[ ] Dependencies flow one direction
[ ] No circular dependencies
[ ] File organization matches project conventions

Type Safety

[ ] No any types (TypeScript)
[ ] Return types explicitly declared
[ ] Interfaces defined for data structures
[ ] Enums for fixed value sets
[ ] Generics used where appropriate

Error Handling & Documentation

[ ] Errors handled at appropriate level
[ ] Custom error types for domain errors
[ ] Error messages are actionable
[ ] Public APIs have documentation
[ ] Complex algorithms explained
[ ] Business rules documented
[ ] No misleading comments
[ ] TODOs have associated issues

5. Test Coverage (25 items)

Untested code is broken code you haven't found yet. These checks ensure your test suite is actually protecting you.

Coverage Basics

[ ] Happy path tested for each public function
[ ] Error paths tested
[ ] Edge cases tested (null, empty, boundaries)
[ ] New code has corresponding tests
[ ] Modified code has updated tests
[ ] Deleted code has removed tests

Test Quality

[ ] Tests verify behavior, not implementation
[ ] Each test has a clear, descriptive name
[ ] One concept per test
[ ] Tests are independent (no shared state)
[ ] No flaky tests
[ ] Mocks are minimal

// BAD: Testing implementation
test('calls fetchUser and sets state', () => {
  component.loadUser(1);
  expect(fetchUser).toHaveBeenCalledWith(1);
  expect(component.state.loading).toBe(false);
});

// GOOD: Testing behavior
test('displays user name after loading', async () => {
  render(<UserProfile id={1} />);
  expect(await screen.findByText('Jane Doe')).toBeVisible();
});

Critical Paths

[ ] Auth flow fully tested
[ ] Authorization tested (allow + deny)
[ ] Payment flow tested
[ ] Data validation tested (valid + invalid)
[ ] Error recovery tested

Integration & Red Flags

[ ] API endpoints tested with realistic payloads
[ ] Database operations tested
[ ] External services mocked or integration-tested
[ ] Functions > 20 lines have tests
[ ] All switch/case branches tested
[ ] Try/catch error paths tested
[ ] Both conditional branches tested

How to Use This Checklist

You don't need to check all 125 items on every PR. Here's a practical approach:

Small PRs (< 50 lines): Focus on correctness + security
Feature PRs: Full pass on quality + security + tests
Performance-sensitive PRs: Add the performance dimension
Pre-release: Run the full 125-point checklist

The key is having a systematic process rather than relying on gut instinct. Studies show that ad-hoc reviews miss 2-3x more defects than structured reviews.

Automate What You Can

I built Code Review Orchestra — a Claude Code skill that runs all 5 dimensions in parallel, producing a structured review report. It uses the same checklist categories above but applies them automatically to your diffs.

Whether you use a tool or this manual checklist, the important thing is having a consistent, multi-dimensional review process. Your production environment will thank you.

What's on your code review checklist that I missed? Drop it in the comments — I'm always looking to expand the list.

DEV Community