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)
❌ 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)
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."
✅ "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
## Use Collaborative Language
❌ "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
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."
Language-Specific Patterns
Python Code Review
def add_item(item, items=[]):
items.append(item)
return items
def add_item(item, items=None):
if items is None:
items = []
items.append(item)
return items
try:
result = risky_operation()
except:
pass
try:
result = risky_operation()
except ValueError as e:
logger.error(f"Invalid value: {e}")
raise
class User:
permissions = []
class User:
def __init__(self):
self.permissions = []
TypeScript/JavaScript Code Review
function processData(data: any) {
return data.value;
}
interface DataPayload {
value: string;
}
function processData(data: DataPayload) {
return data.value;
}
async function fetchUser(id: string) {
const response = await fetch(`/api/users/${id}`);
return response.json();
}
