Code Reviewer

Code Reviewer

Conduct thorough, systematic code reviews following industry best practices.

Purpose

This skill guides you through performing comprehensive code reviews that improve code quality, catch bugs, maintain standards, and facilitate knowledge sharing.

Instructions

Phase 1: Initial Assessment (2-3 minutes)

Before diving into code:

1. Read the PR/MR description

   • Understand the goal and context
   • Note any special considerations
   • Check linked issues or tickets

2. Review the change overview

   • Check files changed count (>20 files may need splitting)
   • Look at additions/deletions ratio
   • Identify areas of concern

3. Verify CI/Build Status

   • Ensure all tests pass
   • Check build succeeds
   • Review any warnings
   • Note failing checks to address

Phase 2: Code Quality Review (10-15 minutes)

Examine code for quality issues:

1. Readability

   • Clear variable and function names
   • Appropriate comments for complex logic
   • Consistent formatting
   • Reasonable function/method length

2. Code Structure

   • Single Responsibility Principle
   • DRY (Don't Repeat Yourself)
   • Appropriate abstraction levels
   • Logical organization

3. Naming Conventions

   • Follows project standards
   • Descriptive and meaningful
   • Consistent across codebase
   • No abbreviations unless standard

4. Code Smells

   • Magic numbers or strings
   • Excessive complexity
   • Large classes or methods
   • Tight coupling

Phase 3: Logic and Correctness (10-15 minutes)

Verify the implementation:

1. Logic Review

   • Implementation matches requirements
   • Edge cases handled
   • Error conditions addressed
   • Boundary conditions checked

2. Potential Bugs

   • Off-by-one errors
   • Null/undefined handling
   • Resource leaks
   • Race conditions
   • Integer overflow

3. Algorithm Efficiency

   • Reasonable time complexity
   • Appropriate data structures
   • No unnecessary iterations
   • Optimizable sections noted

Phase 4: Testing (5-10 minutes)

Evaluate test coverage:

1. Test Presence

   • New code has tests
   • Modified code has updated tests
   • Tests are meaningful

2. Test Quality

   • Tests actually verify behavior
   • Edge cases covered
   • Error cases tested
   • Tests are maintainable

3. Test Organization

   • Clear test names
   • Arrange-Act-Assert structure
   • No test interdependencies

Phase 5: Security Review (5-10 minutes)

Check for security issues:

1. Input Validation

   • All user input validated
   • Sanitization applied
   • Type checking performed

2. Common Vulnerabilities

   • SQL injection risks
   • XSS vulnerabilities
   • CSRF protection
   • Path traversal issues

3. Authentication & Authorization

   • Proper access controls
   • Session management
   • Secure credential handling

4. Data Protection

   • Sensitive data encrypted
   • No secrets in code
   • PII handled properly

Phase 6: Documentation (3-5 minutes)

Verify documentation:

1. Code Documentation

   • Public APIs documented
   • Complex logic explained
   • Non-obvious decisions noted

2. README Updates

   • New features documented
   • Breaking changes noted
   • Examples provided

3. API Documentation

   • Parameters documented
   • Return values described
   • Error cases listed

Providing Feedback

Structure your review comments effectively:

Severity Levels

Use these severity indicators:

• 🔴 CRITICAL: Must fix before merge (blocks merge)
• 🟠 MAJOR: Should fix before merge (strong suggestion)
• 🟡 MINOR: Could improve (optional)
• 💡 SUGGESTION: Alternative approach (consider)
• ❓ QUESTION: Need clarification
• ✅ PRAISE: Good work (acknowledge)

Comment Template

[SEVERITY] [Category]: Brief title
Line XX: Specific issue
Reason: Why this matters
Suggestion: How to fix

Example:
[code example if helpful]

Examples

Example 1: Security Issue

🔴 CRITICAL: SQL Injection Risk
Line 45: `query = "SELECT * FROM users WHERE id = " + userId`

Reason: Direct string concatenation allows SQL injection attacks
Suggestion: Use parameterized queries

Example:
query = "SELECT * FROM users WHERE id = @userId"
command.Parameters.AddWithValue("@userId", userId)

Example 2: Logic Bug

🟠 MAJOR: Off-by-One Error
Line 67: `for (int i = 0; i <= array.Length; i++)`

Reason: Loop will throw IndexOutOfRangeException on last iteration
Suggestion: Change to `i < array.Length`

Example 3: Code Quality

🟡 MINOR: Magic Number
Line 89: `if (count > 100) { ... }`

Reason: Unclear what 100 represents
Suggestion: Extract to named constant

Example:
private const int MaxItemsPerPage = 100;
if (count > MaxItemsPerPage) { ... }

Example 4: Positive Feedback

✅ PRAISE: Error Handling
Lines 120-135: Excellent error handling with specific messages

This makes debugging much easier. Great work!

Best Practices

1. Be constructive - Focus on the code, not the person
2. Explain reasoning - Help others learn
3. Suggest solutions - Don't just point out problems
4. Acknowledge good work - Positive feedback matters
5. Ask questions - Understand intent before criticizing
6. Be timely - Review promptly to unblock others
7. Be thorough - But know when to stop
8. Focus on important issues - Don't nitpick style if tests fail

Common Issues Checklist

• All tests pass
• No console.log / debug statements left
• No commented-out code
• No TODO comments without tickets
• Error messages are user-friendly
• Logging is appropriate (not too much/little)
• No hardcoded secrets or credentials
• Database migrations included if schema changed
• Dependencies updated in package files
• Breaking changes documented

When to Request Changes

Request changes when:

• Critical security vulnerabilities exist
• Tests fail or are missing for new features
• Code violates established team standards
• Logic bugs will cause incorrect behavior
• Performance issues will impact users

When to Approve

Approve when:

• All critical issues resolved
• Tests pass and provide adequate coverage
• Code follows team standards
• Documentation is sufficient
• No security concerns remain
• Minor issues can be addressed in follow-up

Timeouts

If a review is taking too long:

• Large PRs: Request splitting into smaller chunks
• Complex logic: Schedule pairing session
• Many comments: Discuss in real-time (call/meeting)
• Disagreement: Escalate to team lead or architect

Notes

• Customize this workflow for your team's standards
• Focus on teaching, not gatekeeping
• Balance thoroughness with timely feedback
• Remember: code review is a conversation, not a judgment