code-review-checklist

Comprehensive checklist for reviewing code changes to ensure quality, security, and maintainability.

Quick Review Checklist

Before approving code:

• [ ] Code is simple and readable
• [ ] Functions/variables well-named
• [ ] No duplicated code
• [ ] Proper error handling
• [ ] No hardcoded secrets
• [ ] Input validation implemented
• [ ] Tests included
• [ ] No console.log statements

Security Checks (CRITICAL)

Secrets & Credentials

• [ ] No hardcoded API keys
• [ ] No hardcoded passwords/tokens
• [ ] No credentials in comments
• [ ] .env in .gitignore
• [ ] Secrets in environment variables

Input Validation

• [ ] All user input validated
• [ ] SQL queries parameterized
• [ ] Output escaped (XSS prevention)
• [ ] File uploads validated
• [ ] Path traversal prevented

Authentication & Authorization

• [ ] Auth required on protected routes
• [ ] Users can only access their data
• [ ] Admin routes require admin role
• [ ] JWT properly validated
• [ ] Sessions secure

Code Quality (HIGH)

Function Quality

• [ ] Functions < 50 lines
• [ ] Single responsibility
• [ ] Clear, descriptive names
• [ ] Minimal parameters (< 5)
• [ ] No deeply nested logic (< 4 levels)

File Organization

• [ ] Files < 800 lines
• [ ] Logical grouping
• [ ] Clear directory structure
• [ ] Related code together
• [ ] Separation of concerns

Code Clarity

• [ ] Self-documenting code
• [ ] Comments explain "why", not "what"
• [ ] No magic numbers
• [ ] Consistent naming conventions
• [ ] No abbreviations (unless standard)

Error Handling

• [ ] Try-catch blocks where needed
• [ ] Errors logged properly
• [ ] User-friendly error messages
• [ ] Error propagation correct
• [ ] No swallowed errors

Immutability

• [ ] No object mutation
• [ ] Use spread operator for copies
• [ ] Array methods (map, filter, reduce)
• [ ] No direct array modification
• [ ] Const by default

Example:

// ❌ BAD: Mutation
function updateUser(user, name) {
  user.name = name  // Mutates!
  return user
}

// ✅ GOOD: Immutability
function updateUser(user, name) {
  return {
    ...user,
    name
  }
}

Testing (HIGH)

Test Coverage

• [ ] New code has tests
• [ ] Edge cases covered
• [ ] Error paths tested
• [ ] Integration tests for APIs
• [ ] E2E tests for critical flows

Test Quality

• [ ] Tests are independent
• [ ] No flaky tests
• [ ] Clear test names
• [ ] Proper setup/teardown
• [ ] Mocks used appropriately

Performance (MEDIUM)

Algorithms

• [ ] Time complexity acceptable
• [ ] No O(n²) when O(n log n) possible
• [ ] Efficient data structures used
• [ ] No unnecessary iterations
• [ ] Database queries optimized

React Performance

• [ ] No unnecessary re-renders
• [ ] useMemo for expensive calculations
• [ ] useCallback for stable callbacks
• [ ] Keys on list items
• [ ] Code splitting where appropriate

Database

• [ ] Indexes on queried columns
• [ ] No N+1 queries
• [ ] Pagination for large datasets
• [ ] Connection pooling used
• [ ] Query limits set

Caching

• [ ] Cache expensive operations
• [ ] Cache invalidation strategy
• [ ] No stale data issues
• [ ] Cache keys well-designed

Best Practices (MEDIUM)

Dependencies

• [ ] No unnecessary dependencies
• [ ] Dependencies up to date
• [ ] npm audit clean
• [ ] License compatibility checked
• [ ] Bundle size considered

Accessibility

• [ ] ARIA labels where needed
• [ ] Keyboard navigation works
• [ ] Color contrast sufficient
• [ ] Screen reader friendly
• [ ] Focus management correct

Code Style

• [ ] Consistent formatting
• [ ] Linter passing
• [ ] TypeScript types correct
• [ ] No any types
• [ ] No TODO comments without tickets

Documentation

• [ ] Public APIs documented
• [ ] README updated if needed
• [ ] Complex logic explained
• [ ] Breaking changes noted
• [ ] Migration guide if breaking

Common Issues to Flag

Anti-Patterns

// ❌ Magic numbers
if (status === 200) { }

// ✅ Named constants
const HTTP_OK = 200
if (status === HTTP_OK) { }

// ❌ Nested ternaries
const result = a ? b ? c : d : e

// ✅ If-else or early return
if (!a) return e
return b ? c : d

// ❌ Large objects in state
const [user, setUser] = useState(bigObject)

// ✅ Only store what's needed
const [userId, setUserId] = useState(id)

Security Anti-Patterns

// ❌ String concatenation in queries
db.query(`SELECT * FROM users WHERE id = ${id}`)

// ✅ Parameterized queries
db.query('SELECT * FROM users WHERE id = ?', [id])

// ❌ Eval with user input
eval(userCode)

// ✅ Don't use eval at all

// ❌ innerHTML with user data
div.innerHTML = userComment

// ✅ textContent or framework escaping
div.textContent = userComment

Performance Anti-Patterns

// ❌ Filter then map
items.filter(x => x.active).map(x => x.name)

// ✅ Reduce or single pass
items.reduce((acc, x) => {
  if (x.active) acc.push(x.name)
  return acc
}, [])

// ❌ useEffect without dependencies
useEffect(() => fetchData())

// ✅ Specify dependencies
useEffect(() => fetchData(), [userId])

Review Priority Levels

CRITICAL (Must Fix)

• Security vulnerabilities
• Data corruption risks
• Breaking changes without migration
• Secrets exposure
• Authentication bypass
• SQL injection

HIGH (Should Fix)

• Performance bottlenecks
• Missing tests for new features
• Incorrect error handling
• Large functions (>50 lines)
• Deep nesting (>4 levels)
• Mutation patterns

MEDIUM (Consider Fixing)

• Style inconsistencies
• Missing documentation
• Optimization opportunities
• TODO without tickets
• Minor accessibility issues

LOW (Nice to Have)

• Refactoring suggestions
• Additional examples
• Minor improvements
• Naming improvements

Review Output Format

For each issue found:

[PRIORITY] Brief description
File: path/to/file.ts:line
Issue: Detailed explanation
Fix: How to resolve

// ❌ Current code
// ✅ Suggested code

Example:

[CRITICAL] Hardcoded API key
File: src/api/client.ts:42
Issue: API key exposed in source code
Fix: Move to environment variable

const apiKey = "sk-abc123"  // ❌ Bad
const apiKey = process.env.OPENAI_API_KEY  // ✅ Good

Approval Criteria

• ✅ Approve: No CRITICAL or HIGH issues
• ⚠️ Warning: Only MEDIUM issues (can merge with caution)
• ❌ Block: CRITICAL or HIGH issues found

When to Use This Checklist

• ✅ After writing new code
• ✅ Before creating pull request
• ✅ During code review
• ✅ After bug fixes
• ✅ After refactoring
• ✅ Before merging to main

Review Workflow

1. Quick scan

   • Run linter
   • Check for obvious issues
   • Verify tests pass

2. Security review

   • Check for secrets
   • Validate input handling
   • Verify auth/authorization

3. Quality review

   • Function sizes
   • Code complexity
   • Error handling

4. Test review

   • Coverage adequate
   • Edge cases tested
   • Tests passing

5. Performance review

   • Algorithms efficient
   • No obvious bottlenecks
   • Database queries optimized

Tools to Use

Linting

npm run lint
npx eslint . --fix

Type Checking

npm run typecheck
npx tsc --noEmit

Testing

npm test
npm run test:coverage

Security

npm audit
npx eslint . --plugin security

Bundle Size

npm run build
npx bundle-analyzer

Common Mistakes

1. Forgetting to validate input
2. Hardcoding configuration
3. Not handling errors
4. Writing tests after code (should be TDD)
5. Creating large files/functions
6. Using mutation instead of immutability
7. Leaving console.log statements
8. Not updating documentation
9. Ignoring accessibility
10. Adding unnecessary dependencies

Quick Reference

Good Practices Checklist

• [ ] KISS (Keep It Simple)
• [ ] DRY (Don't Repeat Yourself)
• [ ] YAGNI (You Aren't Gonna Need It)
• [ ] Single Responsibility
• [ ] Small Functions & Files
• [ ] Immutability
• [ ] Error Handling
• [ ] Input Validation
• [ ] Test Coverage
• [ ] Clear Naming

File Size Guidelines

• Typical: 200-400 lines
• Maximum: 800 lines
• If larger: Extract utilities, split by concern

Function Size Guidelines

• Typical: 10-20 lines
• Maximum: 50 lines
• If larger: Extract helper functions

Nesting Guidelines

• Maximum: 4 levels
• If deeper: Extract functions, use early returns

Resources

• Clean Code
• Airbnb JavaScript Style Guide
• OWASP Top 10