code-review-checklist

Code Review Checklist

Pre-review (always start here)

• Read PR description and linked issue — understand the why
• Check CI passes before spending time on review
• Pull branch locally if logic is complex

Functionality

• Solves stated problem and meets acceptance criteria
• Edge cases: null/empty inputs, concurrent calls, network failure
• Error handling: errors caught, message doesn't expose internals
• No off-by-one, loop termination, or race conditions

Security (block if any fail)

• No SQL injection — use parameterized queries, not string concat
• No XSS — escape all user-controlled output in DOM
• No hardcoded secrets — use environment variables
• Authentication required on all protected routes
• Authorization checks presence AND ownership (not just auth)
• File uploads validated: type, size, content

// ❌ SQL injection
const q = `SELECT * FROM users WHERE email = '${email}'`;

// ✅ Parameterized
db.query("SELECT * FROM users WHERE email = $1", [email]);

// ❌ Hardcoded secret
const KEY = "sk_live_abc123";

// ✅ Env variable
const KEY = process.env.API_KEY;
if (!KEY) throw new Error("API_KEY is required");

Performance

• No N+1 queries — check ORM calls inside loops
• Database queries use indexes for filter/sort columns
• No unbounded queries — always paginate or limit
• No blocking main thread with sync I/O (Node.js)
• Caching used for repeated expensive operations

Code quality

• Names describe intent (calculateTotalPrice not calc)
• Functions have single responsibility (< ~30 lines is a signal)
• No dead code or commented-out blocks
• DRY — no copy-paste of more than 3 lines
• Follows existing project conventions and patterns
• Abstractions are deep enough to justify themselves; thin pass-through wrappers fail the deletion test

Tests

• New behavior has test coverage
• Happy path + at least 1 failure/edge case tested
• Tests use real assertions, not just "doesn't throw"
• No brittle tests that break on unrelated changes