Skip to content

code-review-best-practices.md

Latest commit

 

History

History
136 lines (104 loc) · 3.71 KB

code-review-best-practices.md

File metadata and controls

136 lines (104 loc) · 3.71 KB

Code Review Best Practices

Comprehensive guidelines for conducting effective code reviews.

Review Philosophy

Goals of Code Review

Primary Goals:

  • Catch bugs and edge cases before production
  • Ensure code maintainability and readability
  • Share knowledge across the team
  • Enforce coding standards consistently
  • Improve design and architecture decisions

Secondary Goals:

  • Mentor junior developers
  • Build team culture and trust
  • Document design decisions through discussions

What Code Review is NOT

  • A gatekeeping mechanism to block progress
  • An opportunity to show off knowledge
  • A place to nitpick formatting (use linters)
  • A way to rewrite code to personal preference

Review Timing

When to Review

Trigger Action
PR opened Review within 24 hours, ideally same day
Changes requested Re-review within 4 hours
Blocking issue found Communicate immediately

Time Allocation

  • Small PR (<100 lines): 10-15 minutes
  • Medium PR (100-400 lines): 20-40 minutes
  • Large PR (>400 lines): Request to split, or 60+ minutes

Review Depth Levels

Level 1: Skim Review (5 minutes)

  • Check PR description and linked issues
  • Verify CI/CD status
  • Look at file changes overview
  • Identify if deeper review needed

Level 2: Standard Review (20-30 minutes)

  • Full code walkthrough
  • Logic verification
  • Test coverage check
  • Security scan

Level 3: Deep Review (60+ minutes)

  • Architecture evaluation
  • Performance analysis
  • Security audit
  • Edge case exploration

Communication Guidelines

Tone and Language

Use collaborative language:

  • "What do you think about..." instead of "You should..."
  • "Could we consider..." instead of "This is wrong"
  • "I'm curious about..." instead of "Why didn't you..."

Be specific and actionable:

  • Include code examples when suggesting changes
  • Link to documentation or past discussions
  • Explain the "why" behind suggestions

Handling Disagreements

  1. Seek to understand: Ask clarifying questions
  2. Acknowledge valid points: Show you've considered their perspective
  3. Provide data: Use benchmarks, docs, or examples
  4. Escalate if needed: Involve senior dev or architect
  5. Know when to let go: Not every hill is worth dying on

Review Prioritization

Must Fix (Blocking)

  • Security vulnerabilities
  • Data corruption risks
  • Breaking changes without migration
  • Critical performance issues
  • Missing error handling for user-facing features

Should Fix (Important)

  • Test coverage gaps
  • Moderate performance concerns
  • Code duplication
  • Unclear naming or structure
  • Missing documentation for complex logic

Nice to Have (Non-blocking)

  • Style preferences beyond linting
  • Minor optimizations
  • Additional test cases
  • Documentation improvements

Anti-Patterns to Avoid

Reviewer Anti-Patterns

  • Rubber stamping: Approving without actually reviewing
  • Bike shedding: Debating trivial details extensively
  • Scope creep: "While you're at it, can you also..."
  • Ghosting: Requesting changes then disappearing
  • Perfectionism: Blocking for minor style preferences

Author Anti-Patterns

  • Mega PRs: Submitting 1000+ line changes
  • No context: Missing PR description or linked issues
  • Defensive responses: Arguing every suggestion
  • Silent updates: Making changes without responding to comments

Metrics and Improvement

Track These Metrics

  • Time to first review
  • Review cycle time
  • Number of review rounds
  • Defect escape rate
  • Review coverage percentage

Continuous Improvement

  • Hold retrospectives on review process
  • Share learnings from escaped bugs
  • Update checklists based on common issues
  • Celebrate good reviews and catches