code-review-excellence

• 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

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

❌ Bad: "This is wrong."

✅ Good: "This could cause a race condition when multiple users

access simultaneously. Consider using a mutex here."

❌ Bad: "Why didn't you use X pattern?"

✅ Good: "Have you considered the Repository pattern? It would

make this easier to test. Here's an example: [link]"

❌ Bad: "Rename this variable."

✅ Good: "[nit] Consider userCount instead of uc for

clarity. 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

Phase 1: Context Gathering (2-3 minutes)

```markdown

Before diving into code, understand:

1. Read PR description and linked issue
2. Check PR size (>400 lines? Ask to split)
3. Review CI/CD status (tests passing?)
4. Understand the business requirement
5. Note any relevant architectural decisions

```

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

```markdown

1. Architecture & Design

- Does the solution fit the problem?

- Are there simpler approaches?

- Is it consistent with existing patterns?

- Will it scale?

1. File Organization

- Are new files in the right places?

- Is code grouped logically?

- Are there duplicate files?

1. Testing Strategy

- Are there tests?

- Do tests cover edge cases?

- Are tests readable?

```

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

```markdown

For each file:

1. Logic & Correctness

- Edge cases handled?

- Off-by-one errors?

- Null/undefined checks?

- Race conditions?

1. Security

- Input validation?

- SQL injection risks?

- XSS vulnerabilities?

- Sensitive data exposure?

1. Performance

- N+1 queries?

- Unnecessary loops?

- Memory leaks?

- Blocking operations?

1. Maintainability

- Clear variable names?

- Functions doing one thing?

- Complex code commented?

- Magic numbers extracted?

```

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

```markdown

1. Summarize key concerns
2. Highlight what you liked
3. Make clear decision:

- ✅ Approve

- 💬 Comment (minor suggestions)

- 🔄 Request Changes (must address)

1. Offer to pair if complex

```

Technique 1: The Checklist Method

```markdown

• [ ] User input validated and sanitized
• [ ] SQL queries use parameterization
• [ ] Authentication/authorization checked
• [ ] Secrets not hardcoded
• [ ] Error messages don't leak info

• [ ] No N+1 queries
• [ ] Database queries indexed
• [ ] Large lists paginated
• [ ] Expensive operations cached
• [ ] No blocking I/O in hot paths

• [ ] Happy path tested
• [ ] Edge cases covered
• [ ] Error cases tested
• [ ] Test names are descriptive
• [ ] Tests are deterministic

```

Technique 2: The Question Approach

Instead of stating problems, ask questions to encourage thinking:

```markdown

❌ "This will fail if the list is empty."

✅ "What happens if items is an empty array?"

❌ "You need error handling here."

✅ "How should this behave if the API call fails?"

❌ "This is inefficient."

✅ "I see this loops through all users. Have we considered

the performance impact with 100k users?"

```

Technique 3: Suggest, Don't Command

```markdown

❌ "You must change this to use async/await"

✅ "Suggestion: async/await might make this more readable:

```typescript

async function fetchUser(id: string) {

const user = await db.query('SELECT * FROM users WHERE id = ?', id);

return user;

}

```

What do you think?"

❌ "Extract this into a function"

✅ "This logic appears in 3 places. Would it make sense to

extract it into a shared utility function?"

```

Technique 4: Differentiate Severity

```markdown

Use labels to indicate priority:

🔴 [blocking] - Must fix before merge

🟡 [important] - Should fix, discuss if disagree

🟢 [nit] - Nice to have, not blocking

💡 [suggestion] - Alternative approach to consider

📚 [learning] - Educational comment, no action needed

🎉 [praise] - Good work, keep it up!

Example:

"🔴 [blocking] This SQL query is vulnerable to injection.

Please use parameterized queries."

"🟢 [nit] Consider renaming data to userData for clarity."

"🎉 [praise] Excellent test coverage! This will catch edge cases."

```

Python Code Review

```python

# Check for Python-specific issues

# ❌ Mutable default arguments

def add_item(item, items=[]): # Bug! Shared across calls

items.append(item)

return items

# ✅ Use None as default

def add_item(item, items=None):

if items is None:

items = []

items.append(item)

return items

# ❌ Catching too broad

try:

result = risky_operation()

except: # Catches everything, even KeyboardInterrupt!

pass

# ✅ Catch specific exceptions

try:

result = risky_operation()

except ValueError as e:

logger.error(f"Invalid value: {e}")

raise

# ❌ Using mutable class attributes

class User:

permissions = [] # Shared across all instances!

# ✅ Initialize in __init__

class User:

def __init__(self):

self.permissions = []

```

TypeScript/JavaScript Code Review

```typescript

// Check for TypeScript-specific issues

// ❌ Using any defeats type safety

function processData(data: any) { // Avoid any

return data.value;

}

// ✅ Use proper types

interface DataPayload {

value: string;

}

function processData(data: DataPayload) {

return data.value;

}

// ❌ Not handling async errors

async function fetchUser(id: string) {

const response = await fetch(/api/users/${id});

return response.json(); // What if network fails?

}

// ✅ Handle errors properly

async function fetchUser(id: string): Promise {

try {

const response = await fetch(/api/users/${id});

if (!response.ok) {

throw new Error(HTTP ${response.status});

}

return await response.json();

} catch (error) {

console.error('Failed to fetch user:', error);

throw error;

}

}

// ❌ Mutation of props

function UserProfile({ user }: Props) {

user.lastViewed = new Date(); // Mutating prop!

return

}

// ✅ Don't mutate props

function UserProfile({ user, onView }: Props) {

useEffect(() => {

onView(user.id); // Notify parent to update

}, [user.id]);

return

}

```

Pattern 1: Architectural Review

```markdown

When reviewing significant changes:

1. Design Document First

- For large features, request design doc before code

- Review design with team before implementation

- Agree on approach to avoid rework

1. Review in Stages

- First PR: Core abstractions and interfaces

- Second PR: Implementation

- Third PR: Integration and tests

- Easier to review, faster to iterate

1. Consider Alternatives

- "Have we considered using [pattern/library]?"

- "What's the tradeoff vs. the simpler approach?"

- "How will this evolve as requirements change?"

```

Pattern 2: Test Quality Review

```typescript

// ❌ Poor test: Implementation detail testing

test('increments counter variable', () => {

const component = render();

const button = component.getByRole('button');

fireEvent.click(button);

expect(component.state.counter).toBe(1); // Testing internal state

});

// ✅ Good test: Behavior testing

test('displays incremented count when clicked', () => {

render();

const button = screen.getByRole('button', { name: /increment/i });

fireEvent.click(button);

expect(screen.getByText('Count: 1')).toBeInTheDocument();

});

// Review questions for tests:

// - Do tests describe behavior, not implementation?

// - Are test names clear and descriptive?

// - Do tests cover edge cases?

// - Are tests independent (no shared state)?

// - Can tests run in any order?

```

Pattern 3: Security Review

```markdown

Authentication & Authorization

• [ ] Is authentication required where needed?
• [ ] Are authorization checks before every action?
• [ ] Is JWT validation proper (signature, expiry)?
• [ ] Are API keys/secrets properly secured?

Input Validation

• [ ] All user inputs validated?
• [ ] File uploads restricted (size, type)?
• [ ] SQL queries parameterized?
• [ ] XSS protection (escape output)?

Data Protection

• [ ] Passwords hashed (bcrypt/argon2)?
• [ ] Sensitive data encrypted at rest?
• [ ] HTTPS enforced for sensitive data?
• [ ] PII handled according to regulations?

Common Vulnerabilities

• [ ] No eval() or similar dynamic execution?
• [ ] No hardcoded secrets?
• [ ] CSRF protection for state-changing operations?
• [ ] Rate limiting on public endpoints?

```

Pattern: The Sandwich Method (Modified)

```markdown

Traditional: Praise + Criticism + Praise (feels fake)

Better: Context + Specific Issue + Helpful Solution

Example:

"I noticed the payment processing logic is inline in the

controller. This makes it harder to test and reuse.

[Specific Issue]

The calculateTotal() function mixes tax calculation,

discount logic, and database queries, making it difficult

to unit test and reason about.

[Helpful Solution]

Could we extract this into a PaymentService class? That

would make it testable and reusable. I can pair with you

on this if helpful."

```

Handling Disagreements

```markdown

When author disagrees with your feedback:

1. Seek to Understand

"Help me understand your approach. What led you to

choose this pattern?"

1. Acknowledge Valid Points

"That's a good point about X. I hadn't considered that."

1. Provide Data

"I'm concerned about performance. Can we add a benchmark

to validate the approach?"

1. Escalate if Needed

"Let's get [architect/senior dev] to weigh in on this."

1. Know When to Let Go

If it's working and not a critical issue, approve it.

Perfection is the enemy of progress.

```

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

• 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

PR Review Comment Template

```markdown

[Brief overview of what was reviewed]

• [What was done well]
• [Good patterns or approaches]

🔴 [Blocking issue 1]

🔴 [Blocking issue 2]

💡 [Improvement 1]

💡 [Improvement 2]

❓ [Clarification needed on X]

❓ [Alternative approach consideration]

✅ Approve after addressing required changes

```

• 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