Pragmatic Clean Code Reviewer

Strict code review following Clean Code, Clean Architecture, and The Pragmatic Programmer principles.

Core principle: Let machines handle formatting; humans focus on logic and design.

⚠️ MANDATORY FIRST STEP: Project Positioning

STOP! Before reviewing, determine the strictness level using this questionnaire.

Q1: Who will use this code?

Q2: What standard do you want?

Q3: How critical? (Conditional)

Only ask if: (D2 or D3) AND (R3 or R4)

Quick Lookup Table

For detailed explanations: See positioning.md

Level Definitions

Strictness Matrix & Metric Thresholds

Quick reference:

• Function length: L2(≤80) → L3(≤50) → L4(≤30) → L5(≤20)
• Parameter count: L2(≤7) → L3(≤5) → L4(≤3) → L5(≤2)
• Test coverage: L2(30%) → L3(60%) → L4(80%) → L5(95%)

For complete matrices: See positioning.md

⚠️ Measurement Rules (MUST follow)

1. Count logic lines only — exclude docstrings, comments, blank lines
2. Metrics are conversation starters, not hard gates
3. Do NOT report as issues (function length):
   • Single-responsibility functions that cannot be meaningfully decomposed
   • Pure data builders, large switch/match statements, configuration mappings
   • A clear 60-line function beats three confusing 20-line functions (exemption rationale, not default tolerance)
4. Do NOT report as issues (parameter count): (Pragmatic adjustment—original book has no explicit exemptions)
   • Functions where most parameters have default values (count required params only)
   • Internal/private classes not directly instantiated by users
   • Configuration functions (e.g., configure_logging(level="INFO", ...))
   • Factory/Builder patterns controlled by framework
5. Do NOT report as issues (DRY/duplication):
   • DRY tolerance = max allowed repetitions. Report when occurrences exceed this number:
     • L5: max 1 → report on 2nd occurrence
     • L4: max 2 → report on 3rd occurrence
     • L3: max 3 → report on 4th occurrence
     • L2: max 4 → report on 5th occurrence
     • L1: N/A (no limit)
   • Accidental duplication (all levels): Similar code representing different business knowledge—do NOT report even if exceeds tolerance. Quick test: "If one changes, must the other ALWAYS change?" If no → accidental duplication → keep separate.
   • Same file (L1-L3 only): Duplicates within same file are lower risk
   • See principles-spectrum.md for DRY vs WET guidance

Language-Aware Review

Before reviewing, identify the language paradigm:

For language-specific adjustments: See language-adjustments.md

15-Point Review Checklist

1. Correctness & Functionality

• Logic implements requirements correctly? (PP-75)
• Boundary conditions and error handling complete? (CC-153, PP-36)
• Security vulnerabilities? (PP-72, PP-73)

2. Readability & Maintainability

• Names reveal intent? (CC-4, PP-74)
• Functions small and do one thing? (CC-20, CC-21)
• Comments explain "Why" not "What"? (CC-39, CC-43)

3. Design & Architecture

• Follows SRP? (CA-8, CC-110)
• Avoids duplication (DRY)? (PP-15, CC-37)
• Dependency direction correct? (CA-12, CA-31)

4. Testing

• New code has tests? (PP-91, CC-194)
• Tests readable and independent? (CC-102, CC-106)

5. Advanced Checks (L3+)

• Concurrency safe? (PP-57, CC-137)
• Security validated? (PP-72, PP-73)
• Resources released? (PP-40)
• Algorithm complexity appropriate? (PP-63, PP-64)

Common Code Smells

For full symptom lookup: See quick-lookup.md

Red Flags - Investigate Further

⚠️ Language-aware: Some red flags are paradigm-dependent. Always check language-adjustments.md first.

If you notice any of these, consult the reference files:

• Switch statements (CC-24, CC-173) — OOP only; match/when expressions are idiomatic in TS, Rust, Kotlin, FP languages
• Null returns/passes (CC-92, CC-93)
• Commented-out code (CC-58, CC-144)
• Deep nesting (CC-22, CC-178)
• Global state (PP-47, PP-48)
• Inheritance > 2 levels (PP-51)

DO NOT Review (Machine's Job)

These should be caught by Linter/Formatter:

• Formatting and indentation (CC-64~77)
• Basic naming conventions
• Unused variables/imports (CC-162)
• Basic syntax errors
• Missing semicolons/brackets

Focus on what machines can't: Logic correctness, design decisions, architectural alignment.

Report Format

Before reporting: Apply Measurement Rules exemptions. Do NOT include exempt items (e.g., pure data builders exceeding line limits) in any issue category—omit them entirely.

## 📋 Code Review Report

**Project Positioning:** [Level] (e.g., L3 Team)
**Review Scope:** [files/commits reviewed]

### 🔴 Critical Issues (Must Fix)
- [file:line] Issue description
  - **Rule:** XX-## (Rule Name)
  - **Principle:** Brief explanation of why this matters
  - **Suggestion:** How to fix it

### 🟡 Important Issues (Should Fix)
- [file:line] Issue description
  - **Rule:** XX-## (Rule Name)
  - **Principle:** Brief explanation of why this matters
  - **Suggestion:** How to fix it

### 🔵 Minor Issues (Nice to Have)
- [file:line] Issue description
  - **Rule:** XX-## (Rule Name)

### ✅ Strengths
- What's done well

### 📝 Verdict
[✅ Ready to merge / ⚠️ Needs fixes / 🚫 Major rework needed]

Report Example

## 📋 Code Review Report

**Project Positioning:** L3 Team
**Review Scope:** src/services/user.ts, src/utils/helpers.ts

### 🔴 Critical Issues (Must Fix)
- [user.ts:45] SQL query built with string concatenation
  - **Rule:** PP-72 (Keep It Simple and Minimize Attack Surfaces)
  - **Principle:** String concatenation in SQL queries creates injection vulnerabilities
  - **Suggestion:** Use parameterized queries: `db.query('SELECT * FROM users WHERE id = ?', [userId])`

### 🟡 Important Issues (Should Fix)
- [helpers.ts:120] Function `processUserData` has 8 parameters
  - **Rule:** CC-26 (Function Arguments) + CC-147 (Too Many Arguments)
  - **Principle:** Many parameters increase cognitive load and make testing difficult. L3 threshold is ≤5.
  - **Suggestion:** Group related parameters into a `UserDataOptions` object

- [user.ts:200] Duplicate validation logic (3rd occurrence)
  - **Rule:** PP-15 (DRY) + CC-37 (Don't Repeat Yourself)
  - **Principle:** L3 allows max 3 repetitions. This is the 3rd occurrence—consider extracting.
  - **Suggestion:** Extract to `validateUserInput(input)` function in utils

### 🔵 Minor Issues (Nice to Have)
- [helpers.ts:55] Magic number `86400`
  - **Rule:** CC-175 (Replace Magic Numbers with Named Constants)

### ✅ Strengths
- Clear separation between data access and business logic
- Consistent error handling pattern
- Good test coverage on core functions

### 📝 Verdict
⚠️ Needs fixes — Critical SQL injection issue must be addressed before merge

Rule Reference Codes

Common Principles Quick Reference

Component Principles (for packages/modules)

For full glossary: See principles-glossary.md

For DRY vs WET guidance: See principles-spectrum.md

Common Mistakes to Avoid

The Bottom Line

1. Always calibrate: Project level determines strictness
2. Cite rules: Every issue references a rule code
3. Focus on logic: Let machines handle formatting
4. Be pragmatic: Rules serve the code, not vice versa