pragmatic-clean-code-reviewer（务实干净的代码审查者）

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

Fallback: If the user skips positioning or says "just review it," default to L3 (Team) and note in the report header: **Project Positioning:** L3 Team (default — user skipped calibration).

Level Definitions

Review Workflow

Follow this sequence for every review:

1. Calibrate — Ask Q1/Q2/Q3 → determine strictness level (or apply L3 fallback)
2. Scope — Confirm what to review: PR diff (changed files + immediate context), module, or specific files. If PR exceeds the level's size limit, flag it as an issue
3. Language check — Identify paradigm; read language-adjustments.md if language is NOT Java/C#
4. Review — Walk through the 15-Point Checklist against the code
5. Classify — Assign severity to each finding (see Severity Classification below)
6. Assess — Add Effort/Benefit to Critical and Important issues
7. Report — Generate report using the template
8. Verdict — Apply verdict criteria to reach conclusion

Review type adjustments: For bug fixes, emphasize correctness and regression tests. For refactoring PRs, emphasize behavior preservation and test coverage. For new features, emphasize design and architecture. For test code, relax DRY tolerance.

Priority order: security > correctness > design > style. Rules serve the code, not vice versa.

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)*

1. 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

1. 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.

Severity Classification

Rules:

• Security issues are always Critical regardless of project level
• Severity is determined by the issue's *nature*, not by the effort to fix it
• Items below Important threshold are not reported — if an issue isn't worth actioning, omit it entirely

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.

Empty sections: If a severity tier has no issues, omit that section entirely from the report. Do not output a section header with "None" or "No issues found."

Allowed sections only: The report must contain exactly these sections: Critical Issues, Important Issues, Verdict. Do not add any other sections. Do not include praise, strengths, or positive observations anywhere in the report.

## 📋 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
  - Effort: [Low/Medium/High]
    - [reason derived from effort questions below]
  - Benefit: [Low/Medium/High]
    - [reason derived from benefit questions below]

### 🟡 Important Issues (Should Fix)
- **[file:line] Issue description**
  - Rule: XX-## (Rule Name)
  - Principle: Brief explanation of why this matters
  - Suggestion: How to fix it
  - Effort: [Low/Medium/High]
    - [reason derived from effort questions below]
  - Benefit: [Low/Medium/High]
    - [reason derived from benefit questions below]

---

- **[file:line] Issue description**
  - Rule: XX-## (Rule Name)
  - Principle: Brief explanation of why this matters
  - Suggestion: How to fix it
  - Effort: [Low/Medium/High]
    - [reason derived from effort questions below]
  - Benefit: [Low/Medium/High]
    - [reason derived from benefit questions below]

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

Verdict Criteria

Apply the first matching condition from top to bottom.

Effort & Benefit (Critical/Important only)

Add Effort and Benefit lines to each Critical and Important issue to help teams prioritize fix order. Each rating must include 1-3 nested bullet reasons derived from the questions below.

• Effort — how hard to fix

- Low: a few lines, < 30 min - Medium: moderate refactor, 30 min - 4 h - High: architectural change or wide-reaching modification, > 4 h

• Benefit — value gained after fixing (trigger frequency × impact scope)

- High: hot path + severe consequence (data loss, security breach, outage) - Medium: common path + moderate impact, or edge case + severe consequence - Low: edge case + minor impact (UI glitch, degraded experience)

Before assigning ratings, reason through these questions:

For Effort:

• How many files need changes? (1 file = likely Low, 3+ files = likely Medium+)
• Does the fix cross module/layer boundaries? (yes = Medium+)
• Does existing test coverage need updating? (significant test changes = add one level)

For Benefit:

• Is this code on a hot path (called frequently)? (yes = High trigger frequency)
• What's the worst-case consequence if this issue triggers? (data loss/security = High impact)
• Can users work around it? (no workaround = higher impact)

If uncertain, default to the more extreme rating (Low or High), not Medium. Medium should be a deliberate choice, not a fallback.

Express your reasoning as nested bullets under each rating line. Simple issues need 1 bullet; complex issues need 2-3 bullets. Reason bullets must derive from the questions above -- do not use generic justifications.

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])`
  - Effort: Low
    - Single file change, no cross-module impact
  - Benefit: High
    - Hot path -- every user query hits this code
    - Data loss/breach risk if exploited

### 🟡 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
  - Effort: Medium
    - Touches callers across 3 files
    - Test updates needed for new signature
  - Benefit: Low
    - Internal utility, not on hot path
    - Users unaffected functionally

---

- **[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
  - Effort: Low
    - Extract to shared function, update 3 call sites in same module
  - Benefit: Medium
    - Common path -- validation runs on every user mutation
    - Drift risk if logic diverges across copies

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

When to Load References

Do NOT load all references at once. Use the rule prefix (PP/CC/CA) to pick the right file.

Rule Reference Codes

Common Principles Quick Reference

Component Principles (REP, CCP, CRP, ADP, SDP, SAP): See principles-glossary.md

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