Code Review Diagnostic
Systematic code review catches 60-90% of defects before production, reduces maintenance costs by 40%, and serves as effective knowledge transfer. This skill provides structured review guidance for both human reviewers and AI agents.
When to Use This Skill
Use this skill when:
- Reviewing code before merge
- Assessing code quality
- Preparing code for PR submission
- Self-reviewing before requesting review
Do NOT use this skill when:
- Writing new code (use implementation skills)
- Designing architecture (use system-design)
- Working on requirements (use requirements-analysis)
Core Principle
Review effectiveness degrades sharply with PR size. Under 400 lines: highest defect detection. 400-800 lines: 50% less effective. 800+ lines: 90% less effective.
Quick Reference: Review Effectiveness
| Factor | Optimal | Degraded |
|---|---|---|
| PR size | < 400 lines | > 800 lines |
| Review time | < 60 minutes | > 90 minutes |
| Review speed | 200-400 LOC/hour | > 500 LOC/hour |
| Reviewers | 2 | 4+ (diminishing returns) |
Quality Pyramid
| Level | Checks | Catches | Frequency |
|---|---|---|---|
| 1. Automated | Lint, types, unit tests, security scan | 60% | Every commit |
| 2. Integration | Integration tests, contracts, performance | 25% | Every PR |
| 3. Human Review | Design, logic, maintainability, context | 15% | Significant changes |
Review Focus Areas
1. Correctness
Questions:
- Does it solve the stated problem?
- Are edge cases handled?
- Is error handling complete?
- Are assumptions valid?
Validation: Test coverage, business logic, data integrity, concurrency handling
2. Maintainability
Questions:
- Is the code self-documenting?
- Can it be easily modified?
- Are abstractions appropriate?
- Is complexity justified?
Indicators: Clear naming, single responsibility, minimal coupling, high cohesion
3. Performance
Questions:
- Are there obvious bottlenecks?
- Is caching appropriate?
- Are queries optimized?
- Is memory managed?
Red Flags: N+1 queries, unbounded loops, synchronous I/O in async context, memory leaks
4. Security
Questions:
- Is input validated?
- Are secrets protected?
- Is authentication checked?
- Are permissions verified?
Critical Checks: No hardcoded secrets, SQL parameterized, XSS prevention, CSRF tokens
Code Smells Checklist
Method Level
| Smell | Threshold | Action |
|---|---|---|
| Long method | > 50 lines | Extract method |
| Long parameter list | > 5 params | Parameter object |
| Duplicate code | > 10 similar lines | Extract common |
| Dead code | Never called | Remove |
Class Level
| Smell | Symptoms | Action |
|---|---|---|
| God class | > 1000 lines, > 20 methods | Split class |
| Feature envy | Uses other class data excessively | Move method |
| Data clumps | Same parameter groups | Extract class |
Architecture Level
| Smell | Detection | Action |
|---|---|---|
| Circular dependencies | Dependency cycles | Introduce interface |
| Unstable dependencies | Depends on volatile modules | Dependency inversion |
Comment Guidelines
Comment Types
[BLOCKING] - Must fix before merge
- Security vulnerabilities, data corruption risks, breaking API changes
[MAJOR] - Should fix before merge
- Missing tests, performance issues, code duplication
[MINOR] - Can fix in follow-up
- Style inconsistencies, documentation typos, naming improvements
[QUESTION] - Seeking clarification
- Design decisions, business logic, external dependencies
Effective Comment Pattern
Observation + Impact + Suggestion
Example:
"This method is 200 lines long [observation].
This makes it hard to understand and test [impact].
Consider extracting helper methods [suggestion]."
Avoid
- Vague: "This could be better"
- Personal: "I don't like this"
- Nitpicky: "Missing period in comment"
- Overwhelming: 50+ minor style issues
Review Readiness Checklist
Before Requesting Review
- Feature fully implemented
- All tests written and passing
- Self-review performed
- No commented code or debug statements
- Coverage threshold met
- Linting clean
- Build succeeds
- Documentation updated
- PR description explains problem and solution
PR Description Should Include
- Problem statement (why this change?)
- Solution approach (how does it solve it?)
- Testing strategy (how verified?)
- Breaking changes (if any)
- Review focus areas (where to look closely?)
Complexity Thresholds
Cyclomatic Complexity
| Range | Classification | Action |
|---|---|---|
| 1-10 | Simple | OK |
| 11-20 | Moderate | Consider refactoring |
| 21-50 | Complex | Refactor required |
| > 50 | Untestable | Must decompose |
Cognitive Complexity
| Range | Classification |
|---|---|
| < 7 | Clear |
| 7-15 | Acceptable |
| > 15 | Confusing - refactor needed |
Anti-Patterns
Rubber Stamp
Approving without thorough review. "LGTM" in < 1 minute. Fix: Minimum review time, required comments, random audits.
Nitpicking
50+ style comments, missing real issues. Fix: Automate style checks, focus on logic/design, limit minor comments.
Big Bang Review
2000+ line PRs that overwhelm. Fix: Stack small PRs, feature flags, review drafts early.
Security Scanning Categories
Severity Classification
| Level | Definition | SLA |
|---|---|---|
| Critical | Remote code execution possible | Fix immediately |
| High | Data breach possible | Fix within 24 hours |
| Medium | Limited impact | Fix within sprint |
| Low | Minimal risk | Fix when convenient |
Review Metrics
Efficiency
| Metric | Target |
|---|---|
| First review turnaround | < 4 hours |
| Review cycles | < 3 |
| PR to merge time | < 24 hours |
Quality
| Metric | Target |
|---|---|
| Defect detection rate | > 80% |
| Post-merge defects | < 0.5 per PR |
| Review coverage | 100% |
Related Skills
- github-agile - PR workflow and GitHub integration
- task-decomposition - If PR too large, break it down
- requirements-analysis - For unclear requirements