ÔØî 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."

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

Review Process

Phase 1: Context Gathering (2-3 minutes)

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)

1. Architecture & Design

- Does the solution fit the problem?

- Are there simpler approaches?

- Is it consistent with existing patterns?

- Will it scale?

2. File Organization

- Are new files in the right places?

- Is code grouped logically?

- Are there duplicate files?

3. Testing Strategy

- Are there tests?

- Do tests cover edge cases?

- Are tests readable?

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

For each file:

1. Logic & Correctness

- Edge cases handled?

- Off-by-one errors?

- Null/undefined checks?

- Race conditions?

2. Security

- Input validation?

- SQL injection risks?

- XSS vulnerabilities?

- Sensitive data exposure?

3. Performance

- N+1 queries?

- Unnecessary loops?

- Memory leaks?

- Blocking operations?

4. Maintainability

- Clear variable names?

- Functions doing one thing?

- Complex code commented?

- Magic numbers extracted?

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

1. Summarize key concerns

2. Highlight what you liked

3. Make clear decision:

-  Approve

- ­ƒÆ¼ Comment (minor suggestions)

- ­ƒöä Request Changes (must address)

4. Offer to pair if complex

Review Techniques

Technique 1: The Checklist Method

Security Checklist

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

Performance Checklist

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

Testing Checklist

• [ ] 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:

ÔØî "This will fail if the list is empty."

ÔØî "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

Use Collaborative Language

ÔØî "You must change this to use async/await"

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

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

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

"­ƒÄë [praise] Excellent test coverage! This will catch edge cases."

Language-Specific Patterns

Python Code Review

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

// 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) {

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

}

//  Handle errors properly

async function fetchUser(id: string): Promise<User> {

try {

if (!response.ok) {

}

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 <div>{user.name}</div>;

}

//  Don't mutate props

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

useEffect(() => {

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

}, [user.id]);

return <div>{user.name}</div>;

}

Advanced Review Patterns

Pattern 1: Architectural Review

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

2. Review in Stages

- First PR: Core abstractions and interfaces

- Second PR: Implementation

- Third PR: Integration and tests

- Easier to review, faster to iterate

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

// ÔØî Poor test: Implementation detail testing

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

const component = render(<Counter />);

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(<Counter />);

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

Security Review Checklist

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?

Giving Difficult Feedback

Pattern: The Sandwich Method (Modified)

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

When author disagrees with your feedback:

1. Seek to Understand

"Help me understand your approach. What led you to

choose this pattern?"

2. Acknowledge Valid Points

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

3. Provide Data

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

to validate the approach?"

4. Escalate if Needed

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

5. Know When to Let Go

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

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

Summary

[Brief overview of what was reviewed]

Strengths

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

Required Changes

­ƒö┤ [Blocking issue 1]

­ƒö┤ [Blocking issue 2]

Suggestions

­ƒÆí [Improvement 1]

­ƒÆí [Improvement 2]

Questions

ÔØô [Clarification needed on X]

ÔØô [Alternative approach consideration]

Verdict

 Approve after addressing required changes