code-quality-review
Systematic code review patterns, quality dimensions, anti-pattern detection, and constructive feedback techniques. Use when reviewing code changes, assessing codebase quality, identifying technical debt, or mentoring through reviews. Covers correctness, design, security, performance, and maintainability.
$ 安裝
git clone https://github.com/rsmdt/the-startup /tmp/the-startup && cp -r /tmp/the-startup/plugins/team/skills/quality/code-quality-review ~/.claude/skills/the-startup// tip: Run this command in your terminal to install the skill
name: code-quality-review description: Systematic code review patterns, quality dimensions, anti-pattern detection, and constructive feedback techniques. Use when reviewing code changes, assessing codebase quality, identifying technical debt, or mentoring through reviews. Covers correctness, design, security, performance, and maintainability.
Code Quality Review Methodology
Systematic patterns for reviewing code and providing constructive, actionable feedback that improves both code quality and developer skills.
When to Activate
- Reviewing pull requests or merge requests
- Assessing overall codebase quality
- Identifying and prioritizing technical debt
- Mentoring developers through code review
- Establishing code review standards for teams
- Auditing code for security or compliance
Review Dimensions
Every code review should evaluate these six dimensions:
1. Correctness
Does the code work as intended?
| Check | Questions |
|---|---|
| Functionality | Does it solve the stated problem? |
| Edge Cases | Are boundary conditions handled? |
| Error Handling | Are failures gracefully managed? |
| Data Validation | Are inputs validated at boundaries? |
| Null Safety | Are null/undefined cases covered? |
2. Design
Is the code well-structured?
| Check | Questions |
|---|---|
| Single Responsibility | Does each function/class do one thing? |
| Abstraction Level | Is complexity hidden appropriately? |
| Coupling | Are dependencies minimized? |
| Cohesion | Do related things stay together? |
| Extensibility | Can it be modified without major changes? |
3. Readability
Can others understand this code?
| Check | Questions |
|---|---|
| Naming | Do names reveal intent? |
| Comments | Is the "why" explained, not the "what"? |
| Formatting | Is style consistent? |
| Complexity | Is cyclomatic complexity reasonable (<10)? |
| Flow | Is control flow straightforward? |
4. Security
Is the code secure?
| Check | Questions |
|---|---|
| Input Validation | Are all inputs sanitized? |
| Authentication | Are auth checks present where needed? |
| Authorization | Are permissions verified? |
| Data Exposure | Is sensitive data protected? |
| Dependencies | Are there known vulnerabilities? |
5. Performance
Is the code efficient?
| Check | Questions |
|---|---|
| Algorithmic | Is time complexity appropriate? |
| Memory | Are allocations reasonable? |
| I/O | Are database/network calls optimized? |
| Caching | Is caching used where beneficial? |
| Concurrency | Are race conditions avoided? |
6. Testability
Can this code be tested?
| Check | Questions |
|---|---|
| Test Coverage | Are critical paths tested? |
| Test Quality | Do tests verify behavior, not implementation? |
| Mocking | Are external dependencies mockable? |
| Determinism | Are tests reliable and repeatable? |
| Edge Cases | Are boundary conditions tested? |
Anti-Pattern Catalog
Common code smells and their remediation:
Method-Level Anti-Patterns
| Anti-Pattern | Detection Signs | Remediation |
|---|---|---|
| Long Method | >20 lines, multiple responsibilities | Extract Method |
| Long Parameter List | >3-4 parameters | Introduce Parameter Object |
| Duplicate Code | Copy-paste patterns | Extract Method, Template Method |
| Complex Conditionals | Nested if/else, switch statements | Decompose Conditional, Strategy Pattern |
| Magic Numbers | Hardcoded values without context | Extract Constant |
| Dead Code | Unreachable or unused code | Delete it |
Class-Level Anti-Patterns
| Anti-Pattern | Detection Signs | Remediation |
|---|---|---|
| God Object | >500 lines, many responsibilities | Extract Class |
| Data Class | Only getters/setters, no behavior | Move behavior to class |
| Feature Envy | Method uses another class's data extensively | Move Method |
| Inappropriate Intimacy | Classes know too much about each other | Move Method, Extract Class |
| Refused Bequest | Subclass doesn't use inherited behavior | Replace Inheritance with Delegation |
| Lazy Class | Does too little to justify existence | Inline Class |
Architecture-Level Anti-Patterns
| Anti-Pattern | Detection Signs | Remediation |
|---|---|---|
| Circular Dependencies | A depends on B depends on A | Dependency Inversion |
| Shotgun Surgery | One change requires many file edits | Move Method, Extract Class |
| Leaky Abstraction | Implementation details exposed | Encapsulate |
| Premature Optimization | Complex code for unproven performance | Simplify, measure first |
| Over-Engineering | Abstractions for hypothetical requirements | YAGNI - simplify |
Review Prioritization
Focus review effort where it matters most:
Priority 1: Critical (Must Fix)
- Security vulnerabilities (injection, auth bypass)
- Data loss or corruption risks
- Breaking changes to public APIs
- Production stability risks
Priority 2: High (Should Fix)
- Logic errors affecting functionality
- Performance issues in hot paths
- Missing error handling for likely failures
- Violation of architectural principles
Priority 3: Medium (Consider Fixing)
- Code duplication
- Missing tests for new code
- Naming that reduces clarity
- Overly complex conditionals
Priority 4: Low (Nice to Have)
- Style inconsistencies
- Minor optimization opportunities
- Documentation improvements
- Refactoring suggestions
Constructive Feedback Patterns
The Feedback Formula
[Observation] + [Why it matters] + [Suggestion] + [Example if helpful]
Good Feedback Examples
# Instead of:
"This is wrong"
# Say:
"This query runs inside a loop (line 45), which could cause N+1
performance issues as the dataset grows. Consider using a batch
query before the loop:
```python
users = User.query.filter(User.id.in_(user_ids)).all()
user_map = {u.id: u for u in users}
"
```markdown
# Instead of:
"Use better names"
# Say:
"The variable `d` on line 23 would be clearer as `daysSinceLastLogin` -
it helps readers understand the business logic without tracing back
to the assignment."
Feedback Tone Guide
| Avoid | Prefer |
|---|---|
| "You should..." | "Consider..." or "What about..." |
| "This is wrong" | "This might cause issues because..." |
| "Why didn't you..." | "Have you considered..." |
| "Obviously..." | "One approach is..." |
| "Always/Never do X" | "In this context, X would help because..." |
Positive Observations
Include what's done well:
"Nice use of the Strategy pattern here - it makes adding new
payment methods straightforward."
"Good error handling - the retry logic with exponential backoff
is exactly what we need for this flaky API."
"Clean separation of concerns between the validation and persistence logic."
Review Checklists
Quick Review Checklist (< 100 lines)
- Code compiles and tests pass
- Logic appears correct for stated purpose
- No obvious security issues
- Naming is clear
- No magic numbers or strings
Standard Review Checklist (100-500 lines)
All of the above, plus:
- Design follows project patterns
- Error handling is appropriate
- Tests cover new functionality
- No significant duplication
- Performance is reasonable
Deep Review Checklist (> 500 lines or critical)
All of the above, plus:
- Architecture aligns with system design
- Security implications considered
- Backward compatibility maintained
- Documentation updated
- Migration/rollback plan if needed
Review Workflow
Before Reviewing
- Understand the context (ticket, discussion, requirements)
- Check if CI passes (don't review failing code)
- Estimate review complexity and allocate time
During Review
- First pass: Understand the overall change
- Second pass: Check correctness and design
- Third pass: Look for edge cases and security
- Document findings as you go
After Review
- Summarize overall impression
- Clearly indicate approval status
- Distinguish blocking vs non-blocking feedback
- Offer to discuss complex suggestions
Review Metrics
Track review effectiveness:
| Metric | Target | What It Indicates |
|---|---|---|
| Review Turnaround | < 24 hours | Team velocity |
| Comments per Review | 3-10 | Engagement level |
| Defects Found | Decreasing trend | Quality improvement |
| Review Time | < 60 min for typical PR | Right-sized changes |
| Approval Rate | 70-90% first submission | Clear standards |
Anti-Patterns in Reviewing
Avoid these review behaviors:
| Anti-Pattern | Description | Better Approach |
|---|---|---|
| Nitpicking | Focusing on style over substance | Use linters for style |
| Drive-by Review | Quick approval without depth | Allocate proper time |
| Gatekeeping | Blocking for personal preferences | Focus on objective criteria |
| Ghost Review | Approval without comments | Add at least one observation |
| Review Bombing | Overwhelming with comments | Prioritize and limit to top issues |
| Delayed Review | Letting PRs sit for days | Commit to turnaround time |
References
- Review Dimension Details - Expanded criteria for each dimension
- Anti-Pattern Examples - Code examples of each anti-pattern
Repository
