Reviewer Checklist

calendarDec 12, 2024 · 4 min read · codereview-knowhow reviewer code-quality best-practices  ·
Share on: linkedincopy

Comprehensive checklist for code reviewers to ensure thorough and constructive reviews.

Functionality

1□ Code does what it's supposed to do
2□ Requirements are met
3□ Edge cases are handled
4□ Error handling is appropriate
5□ No obvious bugs
6□ Logic is sound

Code Quality

1□ Code is readable and maintainable
2□ Naming is clear and consistent
3□ Functions are small and focused
4□ No code duplication
5□ Follows SOLID principles
6□ Design patterns are used appropriately
7□ No over-engineering
8□ No premature optimization

Architecture

1□ Changes fit the existing architecture
2□ No architectural violations
3□ Separation of concerns is maintained
4□ Dependencies are appropriate
5□ Interfaces are well-designed
6□ No tight coupling

Tests

1□ Tests are comprehensive
2□ Tests are readable
3□ Tests actually test what they claim to test
4□ No test duplication
5□ Mocks are used appropriately
6□ Test data is realistic

Security

1□ No security vulnerabilities
2□ Input is validated
3□ Output is sanitized
4□ Authentication/authorization is correct
5□ No sensitive data exposure
6□ Dependencies are up-to-date

Performance

1□ No obvious performance issues
2□ Algorithms are efficient
3□ Database queries are optimized
4□ Caching is used where appropriate
5□ No memory leaks

Documentation

1□ Code is self-documenting
2□ Complex logic is explained
3□ Public APIs are documented
4□ README is updated
5□ Comments are helpful, not redundant

Review Process

When Reviewing - Step by Step

  1. 🔍 Understand the context - Read PR description and linked issues
  2. 🔍 Check functionality first - Does it work as intended?
  3. 🔍 Look for security issues - Vulnerabilities, data exposure
  4. 🔍 Verify tests are adequate - Coverage and quality
  5. 🔍 Review code quality - Readability, maintainability
  6. 🔍 Check architecture - Fits existing patterns?
  7. 🔍 Be constructive and kind - Helpful feedback, not criticism

Reviewer Best Practices

Be Constructive

1❌ "This code is terrible"
2✅ "Consider extracting this into a separate function for better readability"
3
4❌ "You don't know what you're doing"
5✅ "This approach might cause issues with X. Have you considered Y?"
6
7❌ "This is wrong"
8✅ "I think there might be an issue here. What happens if X is null?"

Ask Questions

1✅ "Can you explain why you chose this approach?"
2✅ "Have you considered the case where X happens?"
3✅ "What happens if this API call fails?"
4✅ "Could this be simplified by using pattern Y?"

Prioritize Issues

1🔴 Critical: Security vulnerabilities, data loss, crashes
2🟡 Important: Performance issues, maintainability problems
3🟢 Nice-to-have: Style preferences, minor optimizations

Use Prefixes

1[BLOCKER] Must be fixed before merge
2[IMPORTANT] Should be fixed before merge
3[SUGGESTION] Nice to have, not required
4[QUESTION] Need clarification
5[NITPICK] Style/formatting preference
6[LEARNING] Educational comment, no action needed

Review Timing

Quick Review (<100 lines)

  • Time: 10-15 minutes
  • Focus: Functionality, obvious bugs, security

Standard Review (100-400 lines)

  • Time: 30-60 minutes
  • Focus: All aspects, thorough check

Large Review (>400 lines)

  • Time: 1-2 hours or split into multiple sessions
  • Focus: Request PR split if possible

What to Look For

Red Flags (Immediate Rejection)

  • 🚩 No tests for new functionality
  • 🚩 Security vulnerabilities (SQL injection, XSS, etc.)
  • 🚩 Hardcoded secrets (API keys, passwords)
  • 🚩 No error handling in critical paths
  • 🚩 Massive PR (>1000 lines) without justification
  • 🚩 Breaking changes without migration plan
  • 🚩 Fails CI/CD checks

Yellow Flags (Discuss Before Merging)

  • ⚠️ Unclear naming or poor code organization
  • ⚠️ Missing documentation for complex logic
  • ⚠️ Performance concerns (N+1 queries, inefficient algorithms)
  • ⚠️ Architectural violations or tight coupling
  • ⚠️ Insufficient test coverage (<80%)
  • ⚠️ Code duplication that could be refactored

Green Flags (Good to Go)

  • ✅ Clear, self-documenting code
  • ✅ Comprehensive tests with good coverage
  • ✅ Well-structured and follows patterns
  • ✅ Proper error handling
  • ✅ Good documentation
  • ✅ Small, focused changes

Communication Tips

Positive Feedback

Don't just point out problems - acknowledge good work:

1✅ "Nice use of the factory pattern here!"
2✅ "Great test coverage on this feature"
3✅ "I like how you handled this edge case"
4✅ "This is much cleaner than the previous approach"

Constructive Criticism

Frame suggestions positively:

1✅ "What do you think about extracting this to a helper function?"
2✅ "Have you considered using X pattern here? It might simplify this"
3✅ "This works, but we might run into issues when Y happens"

When to Approve vs Request Changes

Approve:

  • All critical issues resolved
  • Minor nitpicks can be addressed later
  • Code meets quality standards
  • You would be comfortable maintaining this code

Request Changes:

  • Security vulnerabilities present
  • Functionality doesn't work as intended
  • Critical bugs or logic errors
  • Architectural violations
  • No tests for new code

Comment (No Approval):

  • Have questions that need answers
  • Want to see how feedback is addressed
  • Waiting for other reviewers

Common Reviewer Mistakes

  • ❌ Being too nitpicky about style (use linters instead)
  • ❌ Reviewing code you don't understand (ask questions!)
  • ❌ Approving without actually reviewing
  • ❌ Being harsh or condescending
  • ❌ Focusing only on what's wrong, ignoring what's right
  • ❌ Not explaining why something should change
  • ❌ Blocking on personal preferences vs standards
  • ❌ Taking too long to review (>24 hours)

Related Snippets