| name | code-review |
| description | Review code for quality, bugs, and design issues. Use when the user says "review this code", "do a code review", "check this PR", "review this diff", "is this code good", "what's wrong with this code", "code quality check", or pastes code and wants feedback - even if they don't explicitly say "code review".
|
Overview
Based on "Clean Code" by Robert C. Martin. Martin's central argument: code is read far more than it is written. Clean code reads like well-written prose - the reader can understand the intent without decoding it. A code review is not just a bug hunt - it's a check that the code communicates its intent clearly to the next developer who reads it.
Workflow
Step 1: Read for intent first
Before looking for bugs, read the code as a whole. Can you understand what it does without reading comments or documentation? If not, that's the first finding.
Step 2: Check naming
Martin's rule: names should reveal intent. Review every variable, function, and class name.
- Does the name tell you what it does, not how it does it?
- Would a new team member understand this name without context?
- Are booleans named as predicates? (
isValid, hasPermission, not flag, check)
Bad: d = datetime.now() - start
Good: elapsed_time = datetime.now() - start
Step 3: Check function size and responsibility
Martin's single responsibility principle: a function should do one thing and do it well.
- Functions longer than 20 lines are candidates for extraction
- If a function needs a comment to explain what each section does, split it
- If a function has multiple levels of abstraction mixed together, extract
Step 4: Check error handling
- Are errors handled explicitly or silently swallowed?
- Do error messages help the caller understand what went wrong and how to fix it?
- Are exceptions used for exceptional cases only - not flow control?
Step 5: Check for duplication
Martin's DRY principle: duplication is the root of all evil in software.
- Is this logic duplicated elsewhere in the codebase?
- Could this be extracted to a shared utility?
- Are magic numbers duplicated? (extract to named constants)
Step 6: Check tests
- Are there tests for the new or changed behavior?
- Do tests cover the happy path AND failure cases?
- Are test names descriptive? (
test_login_fails_with_invalid_password not test_login_2)
Step 7: Write the review
Structure feedback as:
- Must fix - bugs, security issues, broken contracts
- Should fix - Clean Code violations that will cause maintenance pain
- Consider - style suggestions, optional improvements
- Praise - what was done well (important for team morale and learning)
Anti-Patterns
1. Nitpicking style without flagging real issues
Bad: 10 comments about variable naming, zero comments about a missing null check.
Good: Prioritize by impact. Fix correctness first, then clarity, then style.
2. No positive feedback
Bad: Only listing problems.
Good: Note what was done well. Reviewers who only criticize produce defensive engineers.
3. Vague feedback
Bad: "This function is too complex."
Good: "This function does 3 things: validates input, transforms data, and saves to DB. Extract each into its own function so each can be read and tested independently."
4. Reviewing without running the code
Bad: Approving a PR without checking it builds and tests pass.
Good: CI should enforce this automatically. If CI is absent, run tests locally before approving.
Quality Checklist
