Code Review

Code Review Best Practices

Core Principles

Review Mindset:

• Be constructive, explain the "why"
• Prioritize by severity (critical vs nice-to-have)
• Suggest alternatives, not just problems
• Acknowledge good work

Goals:

1. Catch bugs before production
2. Improve code quality
3. Share knowledge
4. Prevent security vulnerabilities
5. Ensure consistency

Security Review

OWASP Top 10 Critical Checks

Injection Attacks:

• SQL: Use parameterized queries, never concatenate user input
• Command: Avoid shell commands with user input, sanitize properly
• Code: Validate all eval(), exec(), dynamic execution
• NoSQL/LDAP/XML: Use safe APIs

Authentication & Authorization:

• Verify auth checks on protected endpoints
• Proper session management (timeout, secure cookies)
• Authorization logic prevents privilege escalation
• Password policies enforced

Sensitive Data:

• No hardcoded secrets (API keys, passwords, tokens)
• Encryption at rest and in transit
• PII/PHI minimally logged
• No sensitive data in URLs or error messages

XSS Prevention:

• All user input escaped/sanitized for output context
• Content-Security-Policy headers configured
• Framework built-in escaping (React JSX, template engines)

Deserialization:

• Never deserialize untrusted data without validation
• Prefer JSON over pickle/marshal
• Validate object types post-deserialization

Misconfiguration:

• No default credentials in production
• Error messages don't leak internals
• Security headers present (HSTS, X-Frame-Options)

API Security:

• Rate limiting on sensitive endpoints
• CORS properly configured
• Input validation on all endpoints
• No sensitive data in GET requests

Common Security Issues

• Path Traversal: Validate paths, prevent ../ attacks
• SSRF: Validate URLs, restrict internal network access
• Open Redirects: Whitelist redirect destinations
• Race Conditions: Check TOCTOU bugs
• Timing Attacks: Constant-time comparison for secrets
• Regex DoS: Avoid complex regex on user input

Code Quality

Readability & Maintainability

Naming:

• Clear, descriptive names that reveal intent
• Consistent conventions (camelCase, snake_case, PascalCase)
• Avoid abbreviations unless domain-standard

Function Design:

• Single responsibility, small (<50 lines ideal)
• Clear input/output, minimal side effects
• Pure functions where possible

Complexity:

• Low cyclomatic complexity (<10)
• Avoid deep nesting (max 3-4 levels)
• Early returns to reduce nesting
• Extract complex conditions into named functions

Documentation:

• Comments explain "why", not "what"
• Public APIs documented
• No commented-out code (use version control)

Error Handling

• All errors handled (catch, log, recover or fail fast)
• No bare except/catch without handling
• Informative error messages (what, why, action)
• Proper error types (not generic Exception)
• Resources cleaned up (finally/defer/using)

Anti-patterns:

• Swallowing exceptions silently
• Catching too broadly
• Using exceptions for flow control
• Returning null instead of error
• Not validating inputs

Code Duplication

• No copy-paste code blocks
• Extract common logic to reusable functions
• Use inheritance/composition appropriately
• Be pragmatic: 3+ copies = refactor time

Performance

Algorithm & Data Structure Efficiency

• Check time complexity (O(n²) → O(n log n) or O(n))
• Appropriate space complexity
• Right data structure (HashMap vs Array, Set vs List)
• Efficient algorithms for common problems

Common Issues

Database:

• N+1 query problems (use joins/batch)
• Missing indexes on filtered/sorted columns
• SELECT * instead of specific columns
• Queries inside loops

Caching:

• Repeated expensive calculations
• Duplicate API calls
• Static data not cached
• Appropriate cache TTL

Resource Management:

• Files/connections/streams closed
• No memory leaks (circular refs, event listeners)
• Unbounded collections (need limits/pagination)

Frontend:

• Unnecessary re-renders (React useMemo, useCallback)
• Large bundles (code splitting)
• Images not optimized
• Blocking JavaScript in critical path

Architecture & Design

SOLID Principles

• Single Responsibility: One reason to change
• Open/Closed: Open for extension, closed for modification
• Liskov Substitution: Subtypes substitutable for base types
• Interface Segregation: Small, focused interfaces
• Dependency Inversion: Depend on abstractions

Other Key Principles

• DRY: Don't Repeat Yourself
• KISS: Keep It Simple
• YAGNI: Don't over-engineer
• Separation of Concerns: Distinct responsibilities
• Composition over Inheritance

Code Structure

• Proper layer separation (presentation, business, data)
• Dependencies flow one direction
• No circular dependencies
• Modules cohesive and loosely coupled
• Configuration separated from code

Testing

Coverage & Quality

• Critical paths tested
• Edge cases covered (empty, null, max values)
• Error paths tested
• Tests deterministic (no flaky tests)
• Tests isolated (no shared state)

Test Quality:

• Arrange-Act-Assert structure
• One assertion focus per test
• Descriptive test names
• No test logic (tests are simple)
• Realistic test data
• External dependencies mocked appropriately

Review Process

Before Review

1. Understand context (PR description, tickets)
2. Check scope (< 400 lines ideal)
3. Run code locally
4. Verify CI passes

During Review

1. Start with architecture/approach
2. Use security/performance/quality checklists
3. Review tests first (explain intended behavior)
4. Ask questions, don't assume

Providing Feedback

Structure:

• Severity: Critical (must fix) vs Nice-to-have
• What: Specific issue
• Why: Why it matters
• How: Concrete alternative

Tone:

• Use "we" language: "We should..." not "You should..."
• Ask questions: "Have we considered...?"
• Be specific: "Function has complexity 32, consider refactoring"

Example: ❌ "This is bad" ✅ "Using string concatenation in loop creates O(n²) complexity. Use StringBuilder for O(n)."

Priority Checklist

Critical (Must Fix)

• Security vulnerabilities (injection, XSS, auth bypass)
• Data loss/corruption risks
• Memory/resource leaks
• Breaking API changes without versioning
• Race conditions/concurrency bugs

High Priority

• Performance issues (O(n²), N+1 queries)
• Missing input validation
• Hardcoded configuration
• Missing error logging
• No tests for new functionality

Medium Priority

• Code duplication (3+ instances)
• High complexity (>15 cyclomatic)
• Missing documentation for public APIs
• Deep nesting (>4 levels)
• Large functions (>100 lines)

Low Priority

• Minor style inconsistencies
• Could be more idiomatic
• Variable names could be clearer
• Magic numbers → constants

Language-Specific Notes

JavaScript/TypeScript:

• Use const over let, avoid var
• Proper async/await (handle rejections)
• Use === not ==
• TypeScript: Proper types, avoid any
• Null/undefined handling

Python:

• Follow PEP 8
• Type hints for public APIs
• Context managers (with)
• No mutable default arguments

Java:

• Proper exception hierarchy
• Try-with-resources
• Access modifiers (private/protected/public)
• Immutability (final fields)

Go:

• Error handling on every call
• defer for cleanup
• No goroutine leaks (context cancellation)
• Small, focused interfaces

Best Practices Summary

1. Security First: OWASP Top 10 vulnerabilities
2. Test Coverage: Meaningful tests exist
3. Error Handling: All errors handled
4. Performance: Watch O(n²), N+1, memory leaks
5. Readability: Clear, self-documenting code
6. Maintainability: Low complexity, no duplication
7. Documentation: Public APIs documented
8. Consistency: Follow team conventions
9. Constructive: Be helpful, not critical
10. Prioritize: Critical first, style last