1---
2name: code-review-excellence
3description: Master effective code review practices to provide constructive feedback, catch bugs early, and foster knowledge sharing while maintaining team morale. Use when reviewing pull requests, establishing review standards, or mentoring developers.
4---
5 
6# Code Review Excellence
7 
8Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.
9 
10## When to Use This Skill
11 
12- Reviewing pull requests and code changes
13- Establishing code review standards for teams
14- Mentoring junior developers through reviews
15- Conducting architecture reviews
16- Creating review checklists and guidelines
17- Improving team collaboration
18- Reducing code review cycle time
19- Maintaining code quality standards
20 
21## Core Principles
22 
23### 1. The Review Mindset
24 
25**Goals of Code Review:**
26 
27- Catch bugs and edge cases
28- Ensure code maintainability
29- Share knowledge across team
30- Enforce coding standards
31- Improve design and architecture
32- Build team culture
33 
34**Not the Goals:**
35 
36- Show off knowledge
37- Nitpick formatting (use linters)
38- Block progress unnecessarily
39- Rewrite to your preference
40 
41### 2. Effective Feedback
42 
43**Good Feedback is:**
44 
45- Specific and actionable
46- Educational, not judgmental
47- Focused on the code, not the person
48- Balanced (praise good work too)
49- Prioritized (critical vs nice-to-have)
50 
51```markdown
52❌ Bad: "This is wrong."
53✅ Good: "This could cause a race condition when multiple users
54access simultaneously. Consider using a mutex here."
55 
56❌ Bad: "Why didn't you use X pattern?"
57✅ Good: "Have you considered the Repository pattern? It would
58make this easier to test. Here's an example: [link]"
59 
60❌ Bad: "Rename this variable."
61✅ Good: "[nit] Consider `userCount` instead of `uc` for
62clarity. Not blocking if you prefer to keep it."
63```
64 
65### 3. Review Scope
66 
67**What to Review:**
68 
69- Logic correctness and edge cases
70- Security vulnerabilities
71- Performance implications
72- Test coverage and quality
73- Error handling
74- Documentation and comments
75- API design and naming
76- Architectural fit
77 
78**What Not to Review Manually:**
79 
80- Code formatting (use Prettier, Black, etc.)
81- Import organization
82- Linting violations
83- Simple typos
84 
85## Review Process
86 
87### Phase 1: Context Gathering (2-3 minutes)
88 
89```markdown
90Before diving into code, understand:
91 
921. Read PR description and linked issue
932. Check PR size (>400 lines? Ask to split)
943. Review CI/CD status (tests passing?)
954. Understand the business requirement
965. Note any relevant architectural decisions
97```
98 
99### Phase 2: High-Level Review (5-10 minutes)
100 
101```markdown
1021. **Architecture & Design**
103   - Does the solution fit the problem?
104   - Are there simpler approaches?
105   - Is it consistent with existing patterns?
106   - Will it scale?
107 
1082. **File Organization**
109   - Are new files in the right places?
110   - Is code grouped logically?
111   - Are there duplicate files?
112 
1133. **Testing Strategy**
114   - Are there tests?
115   - Do tests cover edge cases?
116   - Are tests readable?
117```
118 
119### Phase 3: Line-by-Line Review (10-20 minutes)
120 
121```markdown
122For each file:
123 
1241. **Logic & Correctness**
125   - Edge cases handled?
126   - Off-by-one errors?
127   - Null/undefined checks?
128   - Race conditions?
129 
1302. **Security**
131   - Input validation?
132   - SQL injection risks?
133   - XSS vulnerabilities?
134   - Sensitive data exposure?
135 
1363. **Performance**
137   - N+1 queries?
138   - Unnecessary loops?
139   - Memory leaks?
140   - Blocking operations?
141 
1424. **Maintainability**
143   - Clear variable names?
144   - Functions doing one thing?
145   - Complex code commented?
146   - Magic numbers extracted?
147```
148 
149### Phase 4: Summary & Decision (2-3 minutes)
150 
151```markdown
1521. Summarize key concerns
1532. Highlight what you liked
1543. Make clear decision:
155   - ✅ Approve
156   - 💬 Comment (minor suggestions)
157   - 🔄 Request Changes (must address)
1584. Offer to pair if complex
159```
160 
161## Review Techniques
162 
163### Technique 1: The Checklist Method
164 
165```markdown
166## Security Checklist
167 
168- [ ] User input validated and sanitized
169- [ ] SQL queries use parameterization
170- [ ] Authentication/authorization checked
171- [ ] Secrets not hardcoded
172- [ ] Error messages don't leak info
173 
174## Performance Checklist
175 
176- [ ] No N+1 queries
177- [ ] Database queries indexed
178- [ ] Large lists paginated
179- [ ] Expensive operations cached
180- [ ] No blocking I/O in hot paths
181 
182## Testing Checklist
183 
184- [ ] Happy path tested
185- [ ] Edge cases covered
186- [ ] Error cases tested
187- [ ] Test names are descriptive
188- [ ] Tests are deterministic
189```
190 
191### Technique 2: The Question Approach
192 
193Instead of stating problems, ask questions to encourage thinking:
194 
195```markdown
196❌ "This will fail if the list is empty."
197✅ "What happens if `items` is an empty array?"
198 
199❌ "You need error handling here."
200✅ "How should this behave if the API call fails?"
201 
202❌ "This is inefficient."
203✅ "I see this loops through all users. Have we considered
204the performance impact with 100k users?"
205```
206 
207### Technique 3: Suggest, Don't Command
208 
209````markdown
210## Use Collaborative Language
211 
212❌ "You must change this to use async/await"
213✅ "Suggestion: async/await might make this more readable:
214`typescript
215    async function fetchUser(id: string) {
216        const user = await db.query('SELECT * FROM users WHERE id = ?', id);
217        return user;
218    }
219    `
220What do you think?"
221 
222❌ "Extract this into a function"
223✅ "This logic appears in 3 places. Would it make sense to
224extract it into a shared utility function?"
225````
226 
227### Technique 4: Differentiate Severity
228 
229```markdown
230Use labels to indicate priority:
231 
232🔴 [blocking] - Must fix before merge
233🟡 [important] - Should fix, discuss if disagree
234🟢 [nit] - Nice to have, not blocking
235💡 [suggestion] - Alternative approach to consider
236📚 [learning] - Educational comment, no action needed
237🎉 [praise] - Good work, keep it up!
238 
239Example:
240"🔴 [blocking] This SQL query is vulnerable to injection.
241Please use parameterized queries."
242 
243"🟢 [nit] Consider renaming `data` to `userData` for clarity."
244 
245"🎉 [praise] Excellent test coverage! This will catch edge cases."
246```
247 
248## Language-Specific Patterns
249 
250### Python Code Review
251 
252```python
253# Check for Python-specific issues
254 
255# ❌ Mutable default arguments
256def add_item(item, items=[]):  # Bug! Shared across calls
257    items.append(item)
258    return items
259 
260# ✅ Use None as default
261def add_item(item, items=None):
262    if items is None:
263        items = []
264    items.append(item)
265    return items
266 
267# ❌ Catching too broad
268try:
269    result = risky_operation()
270except:  # Catches everything, even KeyboardInterrupt!
271    pass
272 
273# ✅ Catch specific exceptions
274try:
275    result = risky_operation()
276except ValueError as e:
277    logger.error(f"Invalid value: {e}")
278    raise
279 
280# ❌ Using mutable class attributes
281class User:
282    permissions = []  # Shared across all instances!
283 
284# ✅ Initialize in __init__
285class User:
286    def __init__(self):
287        self.permissions = []
288```
289 
290### TypeScript/JavaScript Code Review
291 
292```typescript
293// Check for TypeScript-specific issues
294 
295// ❌ Using any defeats type safety
296function processData(data: any) {  // Avoid any
297    return data.value;
298}
299 
300// ✅ Use proper types
301interface DataPayload {
302    value: string;
303}
304function processData(data: DataPayload) {
305    return data.value;
306}
307 
308// ❌ Not handling async errors
309async function fetchUser(id: string) {
310    const response = await fetch(`/api/users/${id}`);
311    return response.json();  // What if network fails?
312}
313 
314// ✅ Handle errors properly
315async function fetchUser(id: string): Promise<User> {
316    try {
317        const response = await fetch(`/api/users/${id}`);
318        if (!response.ok) {
319            throw new Error(`HTTP ${response.status}`);
320        }
321        return await response.json();
322    } catch (error) {
323        console.error('Failed to fetch user:', error);
324        throw error;
325    }
326}
327 
328// ❌ Mutation of props
329function UserProfile({ user }: Props) {
330    user.lastViewed = new Date();  // Mutating prop!
331    return <div>{user.name}</div>;
332}
333 
334// ✅ Don't mutate props
335function UserProfile({ user, onView }: Props) {
336    useEffect(() => {
337        onView(user.id);  // Notify parent to update
338    }, [user.id]);
339    return <div>{user.name}</div>;
340}
341```
342 
343## Advanced Review Patterns
344 
345### Pattern 1: Architectural Review
346 
347```markdown
348When reviewing significant changes:
349 
3501. **Design Document First**
351   - For large features, request design doc before code
352   - Review design with team before implementation
353   - Agree on approach to avoid rework
354 
3552. **Review in Stages**
356   - First PR: Core abstractions and interfaces
357   - Second PR: Implementation
358   - Third PR: Integration and tests
359   - Easier to review, faster to iterate
360 
3613. **Consider Alternatives**
362   - "Have we considered using [pattern/library]?"
363   - "What's the tradeoff vs. the simpler approach?"
364   - "How will this evolve as requirements change?"
365```
366 
367### Pattern 2: Test Quality Review
368 
369```typescript
370// ❌ Poor test: Implementation detail testing
371test('increments counter variable', () => {
372    const component = render(<Counter />);
373    const button = component.getByRole('button');
374    fireEvent.click(button);
375    expect(component.state.counter).toBe(1);  // Testing internal state
376});
377 
378// ✅ Good test: Behavior testing
379test('displays incremented count when clicked', () => {
380    render(<Counter />);
381    const button = screen.getByRole('button', { name: /increment/i });
382    fireEvent.click(button);
383    expect(screen.getByText('Count: 1')).toBeInTheDocument();
384});
385 
386// Review questions for tests:
387// - Do tests describe behavior, not implementation?
388// - Are test names clear and descriptive?
389// - Do tests cover edge cases?
390// - Are tests independent (no shared state)?
391// - Can tests run in any order?
392```
393 
394### Pattern 3: Security Review
395 
396```markdown
397## Security Review Checklist
398 
399### Authentication & Authorization
400 
401- [ ] Is authentication required where needed?
402- [ ] Are authorization checks before every action?
403- [ ] Is JWT validation proper (signature, expiry)?
404- [ ] Are API keys/secrets properly secured?
405 
406### Input Validation
407 
408- [ ] All user inputs validated?
409- [ ] File uploads restricted (size, type)?
410- [ ] SQL queries parameterized?
411- [ ] XSS protection (escape output)?
412 
413### Data Protection
414 
415- [ ] Passwords hashed (bcrypt/argon2)?
416- [ ] Sensitive data encrypted at rest?
417- [ ] HTTPS enforced for sensitive data?
418- [ ] PII handled according to regulations?
419 
420### Common Vulnerabilities
421 
422- [ ] No eval() or similar dynamic execution?
423- [ ] No hardcoded secrets?
424- [ ] CSRF protection for state-changing operations?
425- [ ] Rate limiting on public endpoints?
426```
427 
428## Giving Difficult Feedback
429 
430### Pattern: The Sandwich Method (Modified)
431 
432```markdown
433Traditional: Praise + Criticism + Praise (feels fake)
434 
435Better: Context + Specific Issue + Helpful Solution
436 
437Example:
438"I noticed the payment processing logic is inline in the
439controller. This makes it harder to test and reuse.
440 
441[Specific Issue]
442The calculateTotal() function mixes tax calculation,
443discount logic, and database queries, making it difficult
444to unit test and reason about.
445 
446[Helpful Solution]
447Could we extract this into a PaymentService class? That
448would make it testable and reusable. I can pair with you
449on this if helpful."
450```
451 
452### Handling Disagreements
453 
454```markdown
455When author disagrees with your feedback:
456 
4571. **Seek to Understand**
458   "Help me understand your approach. What led you to
459   choose this pattern?"
460 
4612. **Acknowledge Valid Points**
462   "That's a good point about X. I hadn't considered that."
463 
4643. **Provide Data**
465   "I'm concerned about performance. Can we add a benchmark
466   to validate the approach?"
467 
4684. **Escalate if Needed**
469   "Let's get [architect/senior dev] to weigh in on this."
470 
4715. **Know When to Let Go**
472   If it's working and not a critical issue, approve it.
473   Perfection is the enemy of progress.
474```
475 
476## Best Practices
477 
4781. **Review Promptly**: Within 24 hours, ideally same day
4792. **Limit PR Size**: 200-400 lines max for effective review
4803. **Review in Time Blocks**: 60 minutes max, take breaks
4814. **Use Review Tools**: GitHub, GitLab, or dedicated tools
4825. **Automate What You Can**: Linters, formatters, security scans
4836. **Build Rapport**: Emoji, praise, and empathy matter
4847. **Be Available**: Offer to pair on complex issues
4858. **Learn from Others**: Review others' review comments
486 
487## Common Pitfalls
488 
489- **Perfectionism**: Blocking PRs for minor style preferences
490- **Scope Creep**: "While you're at it, can you also..."
491- **Inconsistency**: Different standards for different people
492- **Delayed Reviews**: Letting PRs sit for days
493- **Ghosting**: Requesting changes then disappearing
494- **Rubber Stamping**: Approving without actually reviewing
495- **Bike Shedding**: Debating trivial details extensively
496 
497## Templates
498 
499### PR Review Comment Template
500 
501```markdown
502## Summary
503 
504[Brief overview of what was reviewed]
505 
506## Strengths
507 
508- [What was done well]
509- [Good patterns or approaches]
510 
511## Required Changes
512 
513🔴 [Blocking issue 1]
514🔴 [Blocking issue 2]
515 
516## Suggestions
517 
518💡 [Improvement 1]
519💡 [Improvement 2]
520 
521## Questions
522 
523❓ [Clarification needed on X]
524❓ [Alternative approach consideration]
525 
526## Verdict
527 
528✅ Approve after addressing required changes
529```
530