Code Reviewer Checklist
Critical Thinking
Before checking items, apply critical thinking:
- Question assumptions: Why was this approach chosen? Is the premise valid?
- Challenge decisions: Is this the best solution, or just the first one that worked?
- Consider alternatives: What trade-offs were made? What was sacrificed?
- Look for hidden issues: What could go wrong that isn't obvious?
- Think long-term: How will this affect maintainability in 6 months?
- Verify claims: If code comments say "this is faster" - is it actually?
- Check edge cases: What happens with null, empty, max values, concurrent access?
- Consider context: Does this make sense given the broader system design?
Focus Areas
1. Architecture & Design
- Does it follow Clean Architecture/DDD?
- Is there proper separation of concerns?
- Is the logic in the right layer?
- Does it introduce unnecessary coupling?
- Is the abstraction level appropriate?
2. Code Quality
- Readability and naming conventions
- DRY (Don't Repeat Yourself) principle
- Function size (less than 50 lines)
- File size (less than 200 lines)
- Complexity (cyclomatic complexity, nesting depth)
3. Complexity Analysis (KISS Principle)
Always verify: "Is this code overcomplicated?"
Signs of Excessive Complexity
- Unnecessary abstraction: Interfaces/abstract classes used only once
- Design pattern overuse: Factory, Strategy, Decorator for simple problems
- Deep inheritance hierarchy: More than 3 levels of inheritance chain
- Excessive generics: Complex type parameters that are hard to read
- Premature Optimization: Complex optimizations applied without actual bottlenecks
- Over-engineering: Excessively extensible design beyond current requirements
Complexity Check Questions
- "Can a junior developer understand this in 30 minutes?"
- "Can the same functionality be implemented with half the code?"
- "Is this abstraction actually reused, or was it created 'just in case'?"
- "What problem occurs if we remove the design pattern? Can you explain specifically?"
- "Was Strategy pattern used when if-else would suffice?"
- "Is this the simplest solution for current requirements?"
Complexity Metrics
| Metric | Recommended | Warning | Dangerous | |--------|-------------|---------|-----------| | Cyclomatic Complexity | ≤10 | 11-20 | >20 | | Nesting Depth | ≤3 | 4 | ≥5 | | Function Parameters | ≤4 | 5-6 | ≥7 | | Class Dependencies | ≤5 | 6-8 | ≥9 | | Inheritance Depth | ≤2 | 3 | ≥4 |
Simplification Suggestions
- Factory → Simple constructor or static method
- Strategy → if-else or switch
- Abstract class (single implementation) → Concrete class
- Multiple wrapper classes → Direct invocation
- Complex generics → Concrete types
4. Correctness & Safety
- Error handling (try-catch, boundary checks)
- Security vulnerabilities (SQLi, XSS, etc.)
- Memory leaks or performance bottlenecks
- Race conditions and thread safety
- Input validation
5. Testing
- Are there sufficient tests?
- Do tests cover edge cases?
- Are tests actually testing behavior, not implementation?
- Can tests fail for the right reasons?
Karpathy Guidelines (변경 범위 검증)
외과적 변경 원칙
리뷰 시 변경이 요청 범위를 벗어나지 않았는지 확인:
- [ ] 범위 한정: 모든 변경 라인이 요청 사항과 직접 연결되는가?
- [ ] 인접 코드 보존: 요청과 무관한 코드/주석/포맷팅을 "개선"하지 않았는가?
- [ ] 기존 스타일 유지: 다르게 하고 싶어도 기존 컨벤션을 따랐는가?
- [ ] 데드코드 처리: 기존 데드코드는 언급만, 이번 변경으로 생긴 고아 코드만 정리했는가?
목표 검증 가능성
- [ ] 성공 기준이 명확하고 검증 가능한가?
- [ ] "버그 수정" → 재현 테스트가 있는가?
- [ ] "기능 추가" → 해당 기능 테스트가 있는가?
가정 명시 여부
- [ ] 암묵적 가정이 코드나 주석에 명시되어 있는가?
- [ ] 불명확한 부분에 대해 질문했거나 문서화했는가?
Feedback Style
- Constructive: Explain the reasoning behind suggestions
- Specific: Point to exact lines and provide examples
- Prioritized: Distinguish between critical fixes and minor suggestions
- Actionable: Provide clear steps to fix, not just problems
Review Questions Template
### Critical Questions
1. What problem does this solve? Is it the right problem?
2. What are the assumptions? Are they valid?
3. What could break this? Under what conditions?
4. What's the worst-case scenario if this fails?
5. **Is there a simpler way to achieve the same result?**
6. **Is this complexity justified? Is there a concrete reason?**
7. **Is there code added "because it might be needed in the future"? (YAGNI violation)**
8. **How much simpler would it be if implemented directly without this abstraction?**
Documentation Requirement
IMPORTANT: After completing a code review, you MUST save the review results as a markdown document.
File Location
docs/reviews/YYYY-MM-DD-<target>-review.md
Document Template
# Code Review: <Target Name>
## Overview
- **Date**: YYYY-MM-DD
- **Reviewer**: Claude
- **Target**: <file path or component name>
- **Verdict**: APPROVE / REQUEST CHANGES / REJECT
## Summary
<Brief summary of what was reviewed>
## Findings
### Critical Issues
- <Issue 1>
- <Issue 2>
### Complexity Issues (Over-engineering)
- <Unnecessary abstraction, pattern overuse, etc.>
### Suggestions
- <Suggestion 1>
- <Suggestion 2>
### Positive Points
- <What was done well>
## Recommendations
<Action items for the developer>
## Files Reviewed
- `path/to/file1.py`
- `path/to/file2.py`
Rules
- Always create the review document after completing the review
- Create
docs/reviews/directory if it doesn't exist - Use descriptive target names (e.g.,
cell-normalizer,auth-module,api-endpoints) - Include specific line numbers when referencing issues