Code Review Excellence

Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.

When to Use This Skill

• Reviewing pull requests and code changes
• Establishing code review standards for teams
• Mentoring junior developers through reviews
• Conducting architecture reviews
• Creating review checklists and guidelines
• Improving team collaboration
• Reducing code review cycle time
• Maintaining code quality standards

Core Principles

1. The Review Mindset

Goals of Code Review:

• Catch bugs and edge cases
• Ensure code maintainability
• Share knowledge across team
• Enforce coding standards
• Improve design and architecture
• Build team culture

Not the Goals:

• Show off knowledge
• Nitpick formatting (use linters)
• Block progress unnecessarily
• Rewrite to your preference

2. Effective Feedback

Good Feedback is:

• Specific and actionable
• Educational, not judgmental
• Focused on the code, not the person
• Balanced (praise good work too)
• Prioritized (critical vs nice-to-have)

markdown
1❌ Bad: "This is wrong."
2✅ Good: "This could cause a race condition when multiple users
3access simultaneously. Consider using a mutex here."
4
5❌ Bad: "Why didn't you use X pattern?"
6✅ Good: "Have you considered the Repository pattern? It would
7make this easier to test. Here's an example: [link]"
8
9❌ Bad: "Rename this variable."
10✅ Good: "[nit] Consider `userCount` instead of `uc` for
11clarity. Not blocking if you prefer to keep it."

3. Review Scope

What to Review:

• Logic correctness and edge cases
• Security vulnerabilities
• Performance implications
• Test coverage and quality
• Error handling
• Documentation and comments
• API design and naming
• Architectural fit

What Not to Review Manually:

• Code formatting (use Prettier, Black, etc.)
• Import organization
• Linting violations
• Simple typos

Review Process

Phase 1: Context Gathering (2-3 minutes)

markdown
1Before diving into code, understand:
2
31. Read PR description and linked issue
42. Check PR size (>400 lines? Ask to split)
53. Review CI/CD status (tests passing?)
64. Understand the business requirement
75. Note any relevant architectural decisions

Phase 2: High-Level Review (5-10 minutes)

markdown
11. **Architecture & Design**
2   - Does the solution fit the problem?
3   - Are there simpler approaches?
4   - Is it consistent with existing patterns?
5   - Will it scale?
6
72. **File Organization**
8   - Are new files in the right places?
9   - Is code grouped logically?
10   - Are there duplicate files?
11
123. **Testing Strategy**
13   - Are there tests?
14   - Do tests cover edge cases?
15   - Are tests readable?

Phase 3: Line-by-Line Review (10-20 minutes)

markdown
1For each file:
2
31. **Logic & Correctness**
4   - Edge cases handled?
5   - Off-by-one errors?
6   - Null/undefined checks?
7   - Race conditions?
8
92. **Security**
10   - Input validation?
11   - SQL injection risks?
12   - XSS vulnerabilities?
13   - Sensitive data exposure?
14
153. **Performance**
16   - N+1 queries?
17   - Unnecessary loops?
18   - Memory leaks?
19   - Blocking operations?
20
214. **Maintainability**
22   - Clear variable names?
23   - Functions doing one thing?
24   - Complex code commented?
25   - Magic numbers extracted?

Phase 4: Summary & Decision (2-3 minutes)

markdown
11. Summarize key concerns
22. Highlight what you liked
33. Make clear decision:
4   - ✅ Approve
5   - 💬 Comment (minor suggestions)
6   - 🔄 Request Changes (must address)
74. Offer to pair if complex

Review Techniques

Technique 1: The Checklist Method

markdown
1## Security Checklist
2
3- [ ] User input validated and sanitized
4- [ ] SQL queries use parameterization
5- [ ] Authentication/authorization checked
6- [ ] Secrets not hardcoded
7- [ ] Error messages don't leak info
8
9## Performance Checklist
10
11- [ ] No N+1 queries
12- [ ] Database queries indexed
13- [ ] Large lists paginated
14- [ ] Expensive operations cached
15- [ ] No blocking I/O in hot paths
16
17## Testing Checklist
18
19- [ ] Happy path tested
20- [ ] Edge cases covered
21- [ ] Error cases tested
22- [ ] Test names are descriptive
23- [ ] Tests are deterministic

Technique 2: The Question Approach

Instead of stating problems, ask questions to encourage thinking:

markdown
1❌ "This will fail if the list is empty."
2✅ "What happens if `items` is an empty array?"
3
4❌ "You need error handling here."
5✅ "How should this behave if the API call fails?"
6
7❌ "This is inefficient."
8✅ "I see this loops through all users. Have we considered
9the performance impact with 100k users?"

Technique 3: Suggest, Don't Command

markdown
1## Use Collaborative Language
2
3❌ "You must change this to use async/await"
4✅ "Suggestion: async/await might make this more readable:
5`typescript
6    async function fetchUser(id: string) {
7        const user = await db.query('SELECT * FROM users WHERE id = ?', id);
8        return user;
9    }
10    `
11What do you think?"
12
13❌ "Extract this into a function"
14✅ "This logic appears in 3 places. Would it make sense to
15extract it into a shared utility function?"

Technique 4: Differentiate Severity

markdown
1Use labels to indicate priority:
2
3🔴 [blocking] - Must fix before merge
4🟡 [important] - Should fix, discuss if disagree
5🟢 [nit] - Nice to have, not blocking
6💡 [suggestion] - Alternative approach to consider
7📚 [learning] - Educational comment, no action needed
8🎉 [praise] - Good work, keep it up!
9
10Example:
11"🔴 [blocking] This SQL query is vulnerable to injection.
12Please use parameterized queries."
13
14"🟢 [nit] Consider renaming `data` to `userData` for clarity."
15
16"🎉 [praise] Excellent test coverage! This will catch edge cases."

Language-Specific Patterns

Python Code Review

python
1# Check for Python-specific issues
2
3# ❌ Mutable default arguments
4def add_item(item, items=[]):  # Bug! Shared across calls
5    items.append(item)
6    return items
7
8# ✅ Use None as default
9def add_item(item, items=None):
10    if items is None:
11        items = []
12    items.append(item)
13    return items
14
15# ❌ Catching too broad
16try:
17    result = risky_operation()
18except:  # Catches everything, even KeyboardInterrupt!
19    pass
20
21# ✅ Catch specific exceptions
22try:
23    result = risky_operation()
24except ValueError as e:
25    logger.error(f"Invalid value: {e}")
26    raise
27
28# ❌ Using mutable class attributes
29class User:
30    permissions = []  # Shared across all instances!
31
32# ✅ Initialize in __init__
33class User:
34    def __init__(self):
35        self.permissions = []

TypeScript/JavaScript Code Review

typescript
1// Check for TypeScript-specific issues
2
3// ❌ Using any defeats type safety
4function processData(data: any) {  // Avoid any
5    return data.value;
6}
7
8// ✅ Use proper types
9interface DataPayload {
10    value: string;
11}
12function processData(data: DataPayload) {
13    return data.value;
14}
15
16// ❌ Not handling async errors
17async function fetchUser(id: string) {
18    const response = await fetch(`/api/users/${id}`);
19    return response.json();  // What if network fails?
20}
21
22// ✅ Handle errors properly
23async function fetchUser(id: string): Promise<User> {
24    try {
25        const response = await fetch(`/api/users/${id}`);
26        if (!response.ok) {
27            throw new Error(`HTTP ${response.status}`);
28        }
29        return await response.json();
30    } catch (error) {
31        console.error('Failed to fetch user:', error);
32        throw error;
33    }
34}
35
36// ❌ Mutation of props
37function UserProfile({ user }: Props) {
38    user.lastViewed = new Date();  // Mutating prop!
39    return <div>{user.name}</div>;
40}
41
42// ✅ Don't mutate props
43function UserProfile({ user, onView }: Props) {
44    useEffect(() => {
45        onView(user.id);  // Notify parent to update
46    }, [user.id]);
47    return <div>{user.name}</div>;
48}

Advanced Review Patterns

Pattern 1: Architectural Review

markdown
1When reviewing significant changes:
2
31. **Design Document First**
4   - For large features, request design doc before code
5   - Review design with team before implementation
6   - Agree on approach to avoid rework
7
82. **Review in Stages**
9   - First PR: Core abstractions and interfaces
10   - Second PR: Implementation
11   - Third PR: Integration and tests
12   - Easier to review, faster to iterate
13
143. **Consider Alternatives**
15   - "Have we considered using [pattern/library]?"
16   - "What's the tradeoff vs. the simpler approach?"
17   - "How will this evolve as requirements change?"

Pattern 2: Test Quality Review

typescript
1// ❌ Poor test: Implementation detail testing
2test('increments counter variable', () => {
3    const component = render(<Counter />);
4    const button = component.getByRole('button');
5    fireEvent.click(button);
6    expect(component.state.counter).toBe(1);  // Testing internal state
7});
8
9// ✅ Good test: Behavior testing
10test('displays incremented count when clicked', () => {
11    render(<Counter />);
12    const button = screen.getByRole('button', { name: /increment/i });
13    fireEvent.click(button);
14    expect(screen.getByText('Count: 1')).toBeInTheDocument();
15});
16
17// Review questions for tests:
18// - Do tests describe behavior, not implementation?
19// - Are test names clear and descriptive?
20// - Do tests cover edge cases?
21// - Are tests independent (no shared state)?
22// - Can tests run in any order?

Pattern 3: Security Review

markdown
1## Security Review Checklist
2
3### Authentication & Authorization
4
5- [ ] Is authentication required where needed?
6- [ ] Are authorization checks before every action?
7- [ ] Is JWT validation proper (signature, expiry)?
8- [ ] Are API keys/secrets properly secured?
9
10### Input Validation
11
12- [ ] All user inputs validated?
13- [ ] File uploads restricted (size, type)?
14- [ ] SQL queries parameterized?
15- [ ] XSS protection (escape output)?
16
17### Data Protection
18
19- [ ] Passwords hashed (bcrypt/argon2)?
20- [ ] Sensitive data encrypted at rest?
21- [ ] HTTPS enforced for sensitive data?
22- [ ] PII handled according to regulations?
23
24### Common Vulnerabilities
25
26- [ ] No eval() or similar dynamic execution?
27- [ ] No hardcoded secrets?
28- [ ] CSRF protection for state-changing operations?
29- [ ] Rate limiting on public endpoints?

Giving Difficult Feedback

Pattern: The Sandwich Method (Modified)

markdown
1Traditional: Praise + Criticism + Praise (feels fake)
2
3Better: Context + Specific Issue + Helpful Solution
4
5Example:
6"I noticed the payment processing logic is inline in the
7controller. This makes it harder to test and reuse.
8
9[Specific Issue]
10The calculateTotal() function mixes tax calculation,
11discount logic, and database queries, making it difficult
12to unit test and reason about.
13
14[Helpful Solution]
15Could we extract this into a PaymentService class? That
16would make it testable and reusable. I can pair with you
17on this if helpful."

Handling Disagreements

markdown
1When author disagrees with your feedback:
2
31. **Seek to Understand**
4   "Help me understand your approach. What led you to
5   choose this pattern?"
6
72. **Acknowledge Valid Points**
8   "That's a good point about X. I hadn't considered that."
9
103. **Provide Data**
11   "I'm concerned about performance. Can we add a benchmark
12   to validate the approach?"
13
144. **Escalate if Needed**
15   "Let's get [architect/senior dev] to weigh in on this."
16
175. **Know When to Let Go**
18   If it's working and not a critical issue, approve it.
19   Perfection is the enemy of progress.

Best Practices

1. Review Promptly: Within 24 hours, ideally same day
2. Limit PR Size: 200-400 lines max for effective review
3. Review in Time Blocks: 60 minutes max, take breaks
4. Use Review Tools: GitHub, GitLab, or dedicated tools
5. Automate What You Can: Linters, formatters, security scans
6. Build Rapport: Emoji, praise, and empathy matter
7. Be Available: Offer to pair on complex issues
8. Learn from Others: Review others' review comments

Common Pitfalls

• Perfectionism: Blocking PRs for minor style preferences
• Scope Creep: "While you're at it, can you also..."
• Inconsistency: Different standards for different people
• Delayed Reviews: Letting PRs sit for days
• Ghosting: Requesting changes then disappearing
• Rubber Stamping: Approving without actually reviewing
• Bike Shedding: Debating trivial details extensively

Templates

PR Review Comment Template

markdown
1## Summary
2
3[Brief overview of what was reviewed]
4
5## Strengths
6
7- [What was done well]
8- [Good patterns or approaches]
9
10## Required Changes
11
12🔴 [Blocking issue 1]
13🔴 [Blocking issue 2]
14
15## Suggestions
16
17💡 [Improvement 1]
18💡 [Improvement 2]
19
20## Questions
21
22❓ [Clarification needed on X]
23❓ [Alternative approach consideration]
24
25## Verdict
26
27✅ Approve after addressing required changes

Resources

• references/code-review-best-practices.md: Comprehensive review guidelines
• references/common-bugs-checklist.md: Language-specific bugs to watch for
• references/security-review-guide.md: Security-focused review checklist
• assets/pr-review-template.md: Standard review comment template
• assets/review-checklist.md: Quick reference checklist
• scripts/pr-analyzer.py: Analyze PR complexity and suggest reviewers