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