code-review-excellence

# 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 ❌ 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 ## Review Process ### 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? 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) ```markdown 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) ```markdown 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 ``` Before writing review feedback, read `references/review-techniques.md` for checklists, question framing, collaboration, and severity techniques. Before language-specific, architectural, test-quality, or security review, read `references/review-patterns.md` for review patterns and checklists. ## Giving Difficult Feedback ### 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?" 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 ```markdown ## 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 ```