📦 deps(thirdparty): update snapshots
This commit is contained in:
+345
@@ -0,0 +1,345 @@
|
||||
# AI Failure Modes — the unique value of this skill
|
||||
|
||||
This file catalogs 15 systematic ways LLMs produce bad code, each backed by published research or widely-documented engineering observations. Read this first if you are an AI agent applying this skill — these are the patterns most likely to enter your own output.
|
||||
|
||||
For each failure mode you get:
|
||||
- **Pattern:** one-line description.
|
||||
- **Source:** the research or post documenting it as systematic, not incidental.
|
||||
- **Bad / Good:** short before-and-after.
|
||||
- **Rule:** the imperative for your own self-check.
|
||||
|
||||
## Contents
|
||||
|
||||
- 1. Catch-all error handling that swallows failures
|
||||
- 2. Defensive guards for impossible cases
|
||||
- 3. Premature abstraction
|
||||
- 4. Comment pollution
|
||||
- 5. Code duplication instead of reuse
|
||||
- 6. Hallucinated APIs and packages
|
||||
- 7. Generic, intent-less naming
|
||||
- 8. Long functions doing many things
|
||||
- 9. Parameter explosion
|
||||
- 10. Inconsistency with surrounding code
|
||||
- 11. Dead code, unused imports, half-implementations
|
||||
- 12. Declares success with mock fallbacks in production code
|
||||
- 13. Plausible-but-wrong code
|
||||
- 14. YAGNI violations through speculative configurability
|
||||
- 15. New dependency for trivial work
|
||||
- Cross-cutting observation
|
||||
- Where this skill differs from generic clean-code rules
|
||||
|
||||
---
|
||||
|
||||
## 1. Catch-all error handling that swallows failures
|
||||
|
||||
**Pattern.** Wrapping operations in broad catch-all handlers or returning null/empty success on any caught error, hiding real bugs.
|
||||
|
||||
**Source.** Karpathy directly observed that LLMs are unusually afraid of exceptions. Reinforced by field reports on LLM error suppression. Root cause is the reward signal during training — propagating exceptions penalizes the model, so the model learns to suppress them.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
getEmail(userId):
|
||||
attempt:
|
||||
user = userStore.get(userId)
|
||||
return user.email
|
||||
catch anyError:
|
||||
return null
|
||||
```
|
||||
Looks safe. In practice, a database outage is now indistinguishable from "user has no email."
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
getEmail(userId):
|
||||
user = userStore.get(userId) // storage errors propagate
|
||||
return user.email // null only means the user has no email
|
||||
```
|
||||
|
||||
**Rule.** Catch only the specific error type you can recover from. Never use broad catch-all handling without a documented recovery path. Returning null/empty success from a handler is forbidden unless the function's contract says so.
|
||||
|
||||
---
|
||||
|
||||
## 2. Defensive guards for impossible cases
|
||||
|
||||
**Pattern.** Adding null checks, runtime type checks, or truthiness checks for conditions the type system or call graph already prevents.
|
||||
|
||||
**Source.** arXiv 2409.19182, "AI-Generated Code Considered Harmful"; HN discussion of defensive code overuse. Same reward-shaping mechanism as #1.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
total(orderItems):
|
||||
if orderItems is null: return 0
|
||||
if orderItems is not a collection: return 0
|
||||
return sum(order.amount for each non-null order in orderItems)
|
||||
```
|
||||
The contract says `orderItems` is a collection of orders. None of these guards can fire under normal call paths.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
total(orderItems):
|
||||
return sum(order.amount for each order in orderItems)
|
||||
```
|
||||
|
||||
**Rule.** Do not add null checks, runtime type checks, or truthiness checks for values whose type annotation or caller contract already excludes that case. Trust the contract. This applies *inside* a trust boundary; at the boundary itself — external input, payloads, deserialized or cross-process data — validation is required, not defensive bloat.
|
||||
|
||||
---
|
||||
|
||||
## 3. Premature abstraction
|
||||
|
||||
**Pattern.** Factories, strategy classes, base classes, plugin hooks, dependency-injection scaffolding introduced before a second concrete user exists.
|
||||
|
||||
**Source.** Martin Fowler, "Patterns for Reducing Friction in AI-Assisted Development" — names "overeagerness (adding unrequested features)" as a documented AI pattern. Fowler, "I still care about the code". Fowler, "Conversation: LLMs and Building Abstractions".
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
PaymentProcessor interface
|
||||
charge(amount)
|
||||
|
||||
CardPaymentProcessor implements PaymentProcessor
|
||||
charge(amount):
|
||||
return paymentProvider.createCharge(amount).id
|
||||
|
||||
PaymentProcessorFactory
|
||||
create():
|
||||
return new CardPaymentProcessor()
|
||||
```
|
||||
There is exactly one payment processor. The abstract interface, the factory, and the indirection are pure ceremony.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
charge(amount):
|
||||
return paymentProvider.createCharge(amount).id
|
||||
```
|
||||
|
||||
**Rule.** Do not introduce an interface, abstract class, factory, registry, strategy, or plugin pattern unless two or more concrete implementations exist today or the spec explicitly requires extensibility. One implementation = inline it.
|
||||
|
||||
---
|
||||
|
||||
## 4. Comment pollution
|
||||
|
||||
**Pattern.** Line-by-line comments restating the code in English; step-number scaffolding comments left in; documentation comments that paraphrase the signature.
|
||||
|
||||
**Source.** HN thread #43929768 — *"The most common thing that makes agentic code ugly is the overuse of comments."* arXiv 2402.13013, "Code Needs Comments" and arXiv on multi-intent comment generation — LLM-generated comments answer "what?" rather than "why?", averaging ~5 descriptive words versus 19-word mixed-intent author comments.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
// Increment counter by one
|
||||
counter += 1
|
||||
|
||||
// Step 3: return the result
|
||||
return result
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
counter += 1
|
||||
|
||||
// Reset counter at midnight UTC to align with billing periods.
|
||||
if counter > daily_limit:
|
||||
counter = 0
|
||||
```
|
||||
|
||||
**Rule.** Comments explain *why*, never *what*. Strip restating-code comments and any leftover "Step N" scaffolding before finalizing. Keep comments only where the rationale wouldn't be obvious to a reader of the code.
|
||||
|
||||
---
|
||||
|
||||
## 5. Code duplication instead of reuse
|
||||
|
||||
**Pattern.** Inline copies of logic that already exists in a helper, instead of importing it.
|
||||
|
||||
**Source.** GitClear, AI Copilot Code Quality 2025 — 211M-LoC longitudinal analysis. Copy-pasted 5+ line blocks increased **8x** between 2021 and 2024. Copy/pasted lines rose from 8.3% to 12.3%. Refactoring share dropped from 25% to under 10%. This is the strongest quantitative result on LLM code quality available.
|
||||
|
||||
**Rule.** Before writing a function, search the codebase for a similar existing one. If a block of ≥5 lines matches existing code in the repo, extract or call the existing function.
|
||||
|
||||
---
|
||||
|
||||
## 6. Hallucinated APIs and packages
|
||||
|
||||
**Pattern.** Imports, method names, or signatures that don't exist in the version of the library actually installed.
|
||||
|
||||
**Source.** Spracklen et al., USENIX Security '25, "Package Hallucinations" — 16 models tested; average hallucination rate 19.6% (commercial ~5%, open source ~21%). arXiv 2409.20550 "LLM Hallucinations in Practical Code Generation" gives a taxonomy. arXiv 2407.09726, "Mitigating Code LLM Hallucinations with API Documentation".
|
||||
|
||||
**Rule.** Every import and external API call must be verified against the actual installed version — read the package, check the lockfile, or import and inspect. Do not call a method based on what "should exist."
|
||||
|
||||
---
|
||||
|
||||
## 7. Generic, intent-less naming
|
||||
|
||||
**Pattern.** `data`, `result`, `item`, `temp`, `value`, `obj`, `info`, `helper`, `manager`, `utils`, `process_*`, `handle_*`, `do_*`.
|
||||
|
||||
**Source.** arXiv 2512.01141, "Neural Variable Name Repair" — generic identifiers are an explicit target of name-repair models because LLM code over-produces them. arXiv 2510.03178, "When Names Disappear" — semantic names act as anchors during generation.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
processData(data):
|
||||
result = []
|
||||
for item in data:
|
||||
temp = item.value * 2
|
||||
result.add(temp)
|
||||
return result
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
doublePrices(orders):
|
||||
return orders.map(order -> order.priceCents * 2)
|
||||
```
|
||||
|
||||
**Rule.** Identifiers must reveal intent. Ban `data`, `result`, `item`, `temp`, `value`, `obj`, `info`, `helper`, `manager`, `handle_*`, `process_*`, `do_*` unless qualified (`raw_csv_bytes`, `parsed_invoice`).
|
||||
|
||||
---
|
||||
|
||||
## 8. Long functions doing many things
|
||||
|
||||
**Pattern.** A single function mixing I/O, business logic, formatting, and side effects — often because the prompt asked for several things in one breath.
|
||||
|
||||
**Source.** arXiv 2512.11922, "Vibe Coding in Practice" — documents how AI-assisted "vibe coding" accumulates technical debt through architectural inconsistencies; the canonical symptom is a single function hundreds of lines long handling several unrelated concerns, assembled from multiple AI-generated fragments. GitClear 2025 — file size 142→267 LoC, cyclomatic complexity 4.2→8.1 in AI-assisted commits. arXiv 2304.10778 compares Copilot/CodeWhisperer/ChatGPT quality.
|
||||
|
||||
**Rule.** A function does one thing. If the prompt asks for N things, produce N functions and a small composer. Refactor ceiling: ~50 lines (target ≤20), ≤4 parameters, cyclomatic complexity ≤10 — refactor before exceeding.
|
||||
|
||||
---
|
||||
|
||||
## 9. Parameter explosion
|
||||
|
||||
**Pattern.** Functions taking 6+ positional or keyword args that should have been a typed config object.
|
||||
|
||||
**Source.** arXiv 2304.10778 quality study and Fowler's overeagerness pattern. The triggering behavior is "AI does not pause to extract a config struct."
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
sendEmail(to, subject, body, retry=true, backoff="exp",
|
||||
html=false, fromAddress=null, encoding="utf-8",
|
||||
internal=false, verbose=false)
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
EmailRequest
|
||||
to
|
||||
subject
|
||||
body
|
||||
html = false
|
||||
|
||||
sendEmail(request: EmailRequest)
|
||||
```
|
||||
|
||||
**Rule.** When a function reaches 5 parameters, stop and introduce a typed request/config object: record, struct, DTO, or equivalent. Do not keep adding positional args.
|
||||
|
||||
---
|
||||
|
||||
## 10. Inconsistency with surrounding code
|
||||
|
||||
**Pattern.** Introduces snake_case in a camelCase file, a new HTTP client when the repo has one, a new error type when an existing taxonomy exists, a new logging style.
|
||||
|
||||
**Source.** Pullflow, "The Context Illusion"; Honeycomb, "How I Code With LLMs These Days"; Stripe Minions architecture writeups (anup.io, Stripe blog). The explicit production fix is forcing the agent to read repo-local conventions before writing.
|
||||
|
||||
**Rule.** Before writing in a file, read the file and at least one neighbor. Match casing, import style, error handling pattern, and logging style. Reuse the project's existing HTTP, database, and logging utilities rather than introducing new ones.
|
||||
|
||||
---
|
||||
|
||||
## 11. Dead code, unused imports, half-implementations
|
||||
|
||||
**Pattern.** Imports never referenced, helper functions never called, branches never reachable, "just in case" exports.
|
||||
|
||||
**Source.** arXiv 2411.01414, "A Deep Dive Into LLM Code Generation Mistakes" — 7-category taxonomy of non-syntactic mistakes including specification-deviation patterns that leave half-implemented code. GitClear's "added-code dominates moved/deleted" finding is the quantitative version.
|
||||
|
||||
**Rule.** Before finalizing, run a linter or static check for unused imports, unused symbols, and unreachable branches; remove them. Do not leave a function defined unless something calls it now.
|
||||
|
||||
---
|
||||
|
||||
## 12. "Declares success" — mock fallbacks in production code
|
||||
|
||||
**Pattern.** Returning hardcoded success values, fixture data, or empty defaults instead of doing the actual work, then claiming the task is done.
|
||||
|
||||
**Source.** Fowler, "Patterns for Reducing Friction" — names "declaring success despite failing tests" and "brute-force fixes." claude-code issue #6984 "Systematic Mock Data Generation Bias". Anthropic Claude Code best practices explicitly tell agents: no mock implementations.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
getUserBalance(userId):
|
||||
return 1000 // TODO: actual provider call
|
||||
```
|
||||
Shipped as the implementation, function body is fiction.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
getUserBalance(userId):
|
||||
raise NotImplemented("Wire to billing.getBalance() after auth is available")
|
||||
```
|
||||
|
||||
**Rule.** Never return hardcoded "success" values or fixture data from a function the spec says should perform real work. Never disable, skip, or change a test to make it pass. If you cannot implement, fail explicitly with an unimplemented error and say what is missing.
|
||||
|
||||
---
|
||||
|
||||
## 13. Plausible-but-wrong code
|
||||
|
||||
**Pattern.** Code that compiles and reads correctly but encodes a slightly wrong formula, range, or null semantic — often lifted from a similar-but-different function.
|
||||
|
||||
**Source.** arXiv 2411.01414 — 4 of the 7 mistake categories are non-syntactic semantic mistakes prior work had missed; root cause is "misunderstanding of specification." Katanaquant, "Your LLM Doesn't Write Correct Code. It Writes Plausible Code". Simon Willison: hallucinations in code are *less* dangerous because they fail loudly — the corollary is that the dangerous class is plausible-but-wrong semantic code that runs.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
# Compute median
|
||||
median = (values[values.length / 2] + values[values.length / 2 + 1]) / 2
|
||||
// off-by-one for odd length
|
||||
|
||||
# Iterate items
|
||||
for index from 1 to items.length - 1:
|
||||
... // silently drops items[0]
|
||||
```
|
||||
|
||||
**Rule.** For any boundary, range, off-by-one, or null-semantic question, write the case enumeration in a comment first (`cases: empty / one / even / odd / null`) and verify each case before the code. Never copy a similar function and adapt — re-derive from the spec.
|
||||
|
||||
---
|
||||
|
||||
## 14. YAGNI violations — speculative configurability
|
||||
|
||||
**Pattern.** Config flags, env vars, optional parameters, and feature toggles for use cases that don't exist.
|
||||
|
||||
**Source.** Fowler's overeagerness pattern and the HN defensive-code thread. Anecdotal but widely observed.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
renderInvoice(invoice,
|
||||
format="pdf", template=null, locale="en",
|
||||
includeQr=false, currencyOverride=null,
|
||||
debug=false, legacyMode=false)
|
||||
```
|
||||
Only one caller exists. It passes `(invoice,)`.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
renderInvoice(invoice)
|
||||
```
|
||||
|
||||
**Rule.** No optional parameter, config flag, env var, or feature toggle without a present-day caller. If you find yourself adding `enable_*`, `use_*`, or `*_mode` arguments, delete them and rely on the single concrete behavior.
|
||||
|
||||
---
|
||||
|
||||
## 15. New dependency for trivial work
|
||||
|
||||
**Pattern.** Adding a third-party package to do what the standard library, an already-installed dependency, or a few lines of code already cover — a micro-dependency for a one-line helper, or a heavy library pulled in for a single function.
|
||||
|
||||
**Source.** Field reports on AI over-building (Fowler's overeagerness pattern) and long-standing supply-chain guidance: every dependency is permanent maintenance surface — version churn, transitive vulnerabilities, audit and licensing weight — that a small local function never carries. Same emit-more bias as modes 3 and 14.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
# add a package to sum a column of numbers
|
||||
import stats_helpers
|
||||
total = stats_helpers.sum_column(rows, "amount")
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
total = sum(row.amount for row in rows)
|
||||
```
|
||||
|
||||
**Rule.** Before adding a package, check the stdlib, the already-installed dependencies, and whether a few lines solve it. Add a dependency only when it owns real complexity you should not re-implement (cryptography, parsing, time zones — illustrative, not exhaustive), never to avoid a short function. This is the inverse of mode 5 and of [dry-kiss-yagni.md](dry-kiss-yagni.md) ranked item 8: don't re-implement what the platform gives you — and don't import what a few lines already cover.
|
||||
|
||||
---
|
||||
|
||||
## Cross-cutting observation
|
||||
|
||||
Nine of the 15 failure modes (1, 2, 3, 9, 12, 14, 15, plus pieces of 8 and 11) trace to one root cause: **the model is biased toward emitting more code, more parameters, more guards, more abstractions** — anything but the minimum required by the spec. The cure is restraint, not knowledge. Before writing each line, ask: *does the spec require this, today?* If no, do not write it.
|
||||
|
||||
## Where this skill differs from generic clean-code rules
|
||||
|
||||
Sections in [naming-and-functions.md](naming-and-functions.md), [solid.md](solid.md), and [dry-kiss-yagni.md](dry-kiss-yagni.md) cover the classic principles. They are necessary but not sufficient — an LLM that "knows" SOLID can still produce code that fails for the reasons in this file. The 15 patterns above are the high-leverage check. Walk them before delivery.
|
||||
+115
@@ -0,0 +1,115 @@
|
||||
# Comments and Formatting — Clean Code Chapters 4 and 5
|
||||
|
||||
Source: Robert C. Martin, *Clean Code*. Summaries: Vivek Khatri Ch. 4, Vivek Khatri Ch. 5, LinkedIn summary of Ch. 4.
|
||||
|
||||
## Contents
|
||||
|
||||
- Comments
|
||||
- C1. Acceptable comments
|
||||
- C2. Banned comments
|
||||
- C3. Docstring discipline
|
||||
- Formatting
|
||||
- Fmt1. Vertical openness separates concepts
|
||||
- Fmt2. Vertical density implies association
|
||||
- Fmt3. Vertical distance
|
||||
- Fmt4. Horizontal density
|
||||
- Fmt5. Match the file you're editing
|
||||
- Self-check for comments and formatting
|
||||
|
||||
## Comments
|
||||
|
||||
The foundational rule: **"Don't comment bad code — rewrite it."** Comments are failures to express intent in code. Every comment is a candidate for rename or extract.
|
||||
|
||||
### C1. Acceptable comments
|
||||
|
||||
A short list of comments that earn their keep:
|
||||
|
||||
- **Legal headers** — license boilerplate, copyright.
|
||||
- **Intent** — explaining *why* a decision was made when the choice is non-obvious. Example: `// Use exponential backoff to avoid hammering the rate limiter during retries.`
|
||||
- **Warnings of consequences** — `// This function is called during transaction commit; do not raise.`
|
||||
- **TODOs** — sparingly, with a tracking ticket reference. `// TODO(JIRA-1234): switch to streaming once API supports it.`
|
||||
- **Public API documentation** — docstrings that document *contract* (preconditions, postconditions, raises), not body.
|
||||
- **Amplification** — calling attention to something non-obvious. `# The `+ 1` accounts for the inclusive end of the range; see RFC §3.2.`
|
||||
|
||||
### C2. Banned comments
|
||||
|
||||
Delete on sight:
|
||||
|
||||
- **Restating-code comments.** `// increment counter by one` above `counter += 1`. The comment adds zero signal and creates two things to maintain.
|
||||
- **Noise comments.** `# default constructor`, `# getter`, `# returns the day of month`.
|
||||
- **Banner comments.** `# ====== USER FUNCTIONS ======`. Use a class or module split instead.
|
||||
- **Closing-brace comments.** `} // end of for loop`. If you need this to follow the flow, the function is too long.
|
||||
- **Attributions and journal comments.** `# Updated by Bob on 2023-04-01 to fix bug #42`. Version control records this.
|
||||
- **Commented-out code.** Delete it. If you need it back, git has it. Commented blocks are toxic — readers don't know whether to trust them.
|
||||
- **`Step 1` / `Step 2` scaffolding.** Common LLM artifact. Each step should be a function call with a name; the names provide the structure.
|
||||
|
||||
### C3. Docstring discipline
|
||||
|
||||
A documentation comment that paraphrases the function signature is noise:
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
add(a, b)
|
||||
// Adds a and b and returns the result.
|
||||
return a + b
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
add(a, b)
|
||||
return a + b
|
||||
```
|
||||
|
||||
A documentation comment earns its keep when it documents contract: what may be passed, what may be returned, what errors are raised, and any non-obvious side effects.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
// Charge a payment source.
|
||||
// Returns: charge identifier.
|
||||
// Raises: CardDeclined for decline failures; PaymentProviderError otherwise.
|
||||
// Side effect: writes an audit record on success.
|
||||
charge(paymentSourceId, amountCents)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Formatting
|
||||
|
||||
### Fmt1. Vertical openness separates concepts
|
||||
|
||||
Blank lines between concepts. No blank lines inside a tightly-coupled block. The eye uses blank lines as boundaries.
|
||||
|
||||
### Fmt2. Vertical density implies association
|
||||
|
||||
Code that belongs together should sit together. Variable declared 30 lines from its use is a smell.
|
||||
|
||||
### Fmt3. Vertical distance
|
||||
|
||||
- **Variables declared close to use.** Not at the top of the function "C-style."
|
||||
- **Caller above callee.** Top-down reading: high-level function first, then the helpers it calls. The step-down rule ([naming-and-functions.md](naming-and-functions.md) F4).
|
||||
- **Conceptually related functions adjacent.** If `parse_invoice` and `validate_invoice` are siblings, put them next to each other, not on opposite ends of the file.
|
||||
|
||||
### Fmt4. Horizontal density
|
||||
|
||||
- Spaces around assignment and comparison operators: `x = 1`, `if x == 1`.
|
||||
- No space between function name and parenthesis: `f(x)` not `f (x)`.
|
||||
- Line length: 80 traditional, ≤100–120 acceptable. Beyond 120 is careless.
|
||||
|
||||
### Fmt5. Match the file you're editing
|
||||
|
||||
The most common cross-cutting violation: introducing a new style in a file that already had one. If the file uses snake_case, do not introduce camelCase. If it uses double quotes, do not introduce single quotes. If it sorts imports alphabetically, do not append at the bottom. If the project already has an HTTP client, database wrapper, or logging helper, reuse it instead of introducing another one.
|
||||
|
||||
Team rules override personal preference. Read the file, then write.
|
||||
|
||||
---
|
||||
|
||||
## Self-check for comments and formatting
|
||||
|
||||
Before you ship code:
|
||||
|
||||
1. Walk every comment you added. For each, ask: does it explain *why*? If it explains *what*, delete it.
|
||||
2. Walk every documentation comment you added. Is it paraphrasing the signature? Delete the paraphrase; keep only contract documentation.
|
||||
3. Any commented-out code? Delete it.
|
||||
4. Any `Step 1`, `Step 2`, `First, ...`, or `Then, ...` scaffolding comments? Delete.
|
||||
5. Are variables declared near their use, not at the top?
|
||||
6. Does the casing, quoting, and import order match the file's existing style?
|
||||
+133
@@ -0,0 +1,133 @@
|
||||
# DRY, KISS, YAGNI
|
||||
|
||||
Three short principles. Often confused. Often applied wrong by AI agents (and humans).
|
||||
|
||||
## Contents
|
||||
|
||||
- DRY: do not duplicate knowledge
|
||||
- KISS: keep complexity low and local
|
||||
- YAGNI: avoid speculative configurability
|
||||
- Ranked list: where AI agents over-engineer
|
||||
- Self-check for DRY, KISS, YAGNI
|
||||
|
||||
---
|
||||
|
||||
## DRY — Don't Repeat Yourself
|
||||
|
||||
**Definition (Hunt & Thomas, *The Pragmatic Programmer*, verbatim).** *"Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."*
|
||||
|
||||
Source: pragprog official DRY excerpt PDF; Wikipedia summary; O'Reilly *97 Things Every Programmer Should Know*, Ch. 30.
|
||||
|
||||
### The misreading
|
||||
|
||||
*"Don't have any duplicate code."* No. Hunt and Thomas frame DRY as duplication "of knowledge, of intent... expressing the same thing in two different places, possibly in two totally different ways." Two functions that **look alike but encode different rules** are not a DRY violation. **One rule** expressed in code + database schema + documentation **is**.
|
||||
|
||||
### Smells worth flagging
|
||||
|
||||
These are textual signals that *probably* indicate knowledge duplication, but verify the underlying meaning before refactoring:
|
||||
|
||||
- Identical token sequence of ≥5 non-trivial lines appearing in ≥2 functions.
|
||||
- The same regex, SQL fragment, or URL literal repeated in ≥3 sites.
|
||||
- The same magic number or string repeated ≥3 times outside a constants module.
|
||||
- The same validation branch (for example, "if value is missing, raise") duplicated across siblings of one module.
|
||||
|
||||
### The Rule of 3 — wait for the third occurrence
|
||||
|
||||
Don't extract an abstraction the first time you see duplication. Don't extract on the second. Wait for the third — by then you have enough signal about the *real* shape of the shared knowledge to abstract correctly. The Rule of 3 is folklore from refactoring practice; the underlying principle (don't abstract too early) is in Fowler's *Refactoring* (refactoring.com/catalog) and in the wrong-abstraction work below.
|
||||
|
||||
### The Sandi Metz corollary — wrong abstraction is worse than duplication
|
||||
|
||||
From Sandi Metz, "The Wrong Abstraction" (Jan 2016): *"duplication is far cheaper than the wrong abstraction."*
|
||||
|
||||
If an abstraction has accumulated per-caller branches and special cases, it is the wrong abstraction. The remedy:
|
||||
|
||||
1. Re-inline the abstraction back into each caller.
|
||||
2. Delete the per-caller dead branches.
|
||||
3. Live with honest duplication for a while.
|
||||
4. Re-abstract only when the *real* shared knowledge becomes obvious.
|
||||
|
||||
**Rule.** Do not introduce an abstraction to eliminate three lines of duplication unless you can name the underlying *knowledge* the lines represent. If you can't name it, leave the duplication.
|
||||
|
||||
---
|
||||
|
||||
## KISS — Keep It Simple, Stupid
|
||||
|
||||
**Origin.** Coined by Clarence "Kelly" Johnson at Lockheed's Skunk Works (U-2, SR-71). His designers were handed a basic toolkit; the aircraft had to be repairable by an average mechanic in a combat field with only those tools. The "stupid" refers to the mismatch between break-conditions and repair sophistication — not to the engineer.
|
||||
|
||||
Original phrasing: *"Keep it simple stupid"* (no comma). First documented use by the U.S. Navy in 1960.
|
||||
|
||||
Source: Wikipedia, KISS principle; Braithwaite background story.
|
||||
|
||||
### Operationalizing KISS for code review
|
||||
|
||||
KISS is fuzzy without numbers. Use these as defaults:
|
||||
|
||||
- **Cognitive Complexity ≤10 per function.** SonarSource's Cognitive Complexity metric measures *how hard the code is to understand* rather than the count of independent paths. It's the dominant 2026 metric — adopted by SonarQube, Biome, and ReSharper. Target <7 for greenfield code; <15 is the upper bound before refactor is mandatory.
|
||||
- **Cyclomatic complexity ≤10 per function.** McCabe's original 1976 metric, still useful as a structural floor. 11–20 is moderate risk; 21–50 is high risk; >50 is untestable. Source: McCabe NIST 235r; JetBrains ReSharper threshold guidance. Use Cognitive Complexity for human-readability judgement; use cyclomatic for "is this testable" judgement. When they disagree, prefer Cognitive Complexity.
|
||||
- **Nesting depth ≤5.** Source: Aivosto, Project Metrics: Complexity.
|
||||
- **Function length:** no canonical absolute, but common static-analysis defaults (SonarQube, PMD) flag >50–60 lines. Pair with complexity ceiling rather than relying on LOC alone.
|
||||
|
||||
### Self-check
|
||||
|
||||
When you see a function exceed cyclo 10 or nest depth 5, refactor *before* finishing — not "later." Extract a helper, replace nested `if/else` with early returns or a lookup table.
|
||||
|
||||
---
|
||||
|
||||
## YAGNI — You Aren't Gonna Need It
|
||||
|
||||
**Canonical reference.** Martin Fowler, *bliki: Yagni* (May 2015). *"A mantra from ExtremeProgramming... capabilities we presume our software needs in the future should not be built now because 'you aren't gonna need it.'"*
|
||||
|
||||
### Fowler's four cost categories
|
||||
|
||||
When you build a presumptive feature, you pay:
|
||||
|
||||
1. **Cost of build.** Analysis, coding, testing of a feature that ends up unused.
|
||||
2. **Cost of delay.** Opportunity cost — revenue-generating work you didn't do instead.
|
||||
3. **Cost of carry.** Added complexity makes every future modification and debug slower.
|
||||
4. **Cost of repair.** When the presumed feature turns out to be wrong, you pay to rip it out plus the technical debt accumulated against it.
|
||||
|
||||
Source: martinfowler.com/bliki/Yagni.html; InfoQ summary.
|
||||
|
||||
### AI-specific YAGNI traps
|
||||
|
||||
LLMs over-produce speculative surface area. These are the patterns to spot:
|
||||
|
||||
1. **Config flags / env vars nobody asked for.** `enable_x_v2`, `legacy_mode`, toggles for a single code path that has no second variant.
|
||||
2. **Plugin / strategy systems for 2 known cases.** Registry + base class + 2 subclasses where a direct conditional is shorter and clearer.
|
||||
3. **Generic helpers with one caller.** `normalizeAnything(value, strict=false, mode="default")` invoked from exactly one site.
|
||||
4. **Optional parameters never passed.** `send(value, retries=3, backoff=null, jitter=false, logger=null)` where every call site uses defaults. Delete them until a real caller exists.
|
||||
5. **Speculative async / batching / caching.** Async wrappers, queues, and batch endpoints where current load is single-digit RPS.
|
||||
6. **Premature interfaces/protocols with one implementation.** `FooRepository` paired with one concrete `SqlFooRepository`. Inline until you have a second implementation.
|
||||
|
||||
### Self-check
|
||||
|
||||
For every parameter, class, file, or abstraction you introduce, answer: *who calls this today?* If the answer is "nobody yet," delete it.
|
||||
|
||||
---
|
||||
|
||||
## Ranked list — where AI agents over-engineer
|
||||
|
||||
By frequency observed (engineering-blog-grade, not from a controlled study):
|
||||
|
||||
1. **Premature interfaces/protocols** with one implementation.
|
||||
2. **Factory classes for trivial constructors** — `UserFactory.create(...)` wrapping `User(...)`.
|
||||
3. **DI containers in small apps** — wiring frameworks for 3–5 services where explicit construction in `main()` is shorter and traceable.
|
||||
4. **Try/catch wrappers that change nothing** — adds lines, hides tracebacks.
|
||||
5. **Speculative config surface** — settings objects with 15 fields where 3 are read.
|
||||
6. **Plugin / registry scaffolding for two cases.**
|
||||
7. **`utils.py` / `common.py` modules** — magnets for unrelated functions; violate DRY's "single authoritative representation" by location.
|
||||
8. **Re-implementing what the platform already gives you** — custom retry loops, enums, or record-like types the standard library provides; hand-rolled validation a database/schema constraint, the type system, or a framework's declarative rule would enforce; a native platform feature replaced by hand-written app code. Prefer the cheapest existing solution: stdlib or native feature over new code, a declarative constraint over an imperative check. The flip side is mode 15 in [ai-failure-modes.md](ai-failure-modes.md) — don't reach for a *new* dependency for what a few lines already cover either.
|
||||
9. **Excessive layering** (Controller → Service → Manager → Repository) for CRUD — four files to read one row.
|
||||
10. **Wrapping libraries "to make them swappable"** — thin pass-through adapters around an HTTP, database, or SDK client you will never swap.
|
||||
|
||||
---
|
||||
|
||||
## Self-check for DRY, KISS, YAGNI
|
||||
|
||||
Before you ship code:
|
||||
|
||||
1. (DRY) Did you eliminate duplication of *knowledge*, or just duplication of *text*? Can you name the underlying rule?
|
||||
2. (DRY/Metz) If you introduced an abstraction, are there at least two callers today whose code is structurally identical? Or is the abstraction speculative?
|
||||
3. (KISS) Any function over cyclomatic 10 or nest depth 5?
|
||||
4. (YAGNI) Any optional parameter, config flag, env var, interface, factory, or base class without a caller using it today?
|
||||
5. (YAGNI) Did you wrap a library to "make it swappable"? Delete the wrapper.
|
||||
+187
@@ -0,0 +1,187 @@
|
||||
# Naming and Functions — Clean Code Chapters 2 and 3
|
||||
|
||||
Source: Robert C. Martin, *Clean Code*. Sample chapters online at the Pearson PDF; chapter summaries at Vivek Khatri's Ch. 2 notes and Herberto Graça's Ch. 3 summary.
|
||||
|
||||
## Contents
|
||||
|
||||
- Meaningful names
|
||||
- N1. Intention-revealing
|
||||
- N2. No disinformation, no encodings
|
||||
- N3. Meaningful distinctions
|
||||
- N4. Searchable, pronounceable
|
||||
- N5. Class names are nouns, method names are verbs
|
||||
- N6. Banned generic names
|
||||
- Functions
|
||||
- F1. Small. Then smaller.
|
||||
- F2. Do one thing
|
||||
- F3. One level of abstraction per function
|
||||
- F4. Step-down rule
|
||||
- F5. Few arguments
|
||||
- F6. No flag arguments
|
||||
- F7. No output arguments / Command-Query Separation
|
||||
- F8. No side effects in queries
|
||||
- F9. Prefer exceptions to return codes
|
||||
- F10. Duplication is the root evil
|
||||
- Self-check for naming and functions
|
||||
|
||||
## Meaningful names
|
||||
|
||||
### N1. Intention-revealing
|
||||
|
||||
A name should tell you *why it exists, what it does, and how it's used*. If you need a comment to explain a name, the name is wrong.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
d // elapsed time in days
|
||||
ts = []
|
||||
fn(xs)
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
elapsedDays
|
||||
timestamps
|
||||
filterOverdueInvoices(invoices)
|
||||
```
|
||||
|
||||
### N2. No disinformation, no encodings
|
||||
|
||||
No Hungarian notation (`strName`, `iCount`). No interface-prefix `I` (`IUserService`). No member prefix `m_`. No "List" suffix unless the type is actually a list (`accountList` for a `set` is disinformation).
|
||||
|
||||
**Bad:** `strFirstName`, `IUserRepo`, `m_count`, `userArray` (when it is not an array).
|
||||
|
||||
**Good:** `first_name`, `UserRepo`, `count`, `users_by_id`.
|
||||
|
||||
### N3. Meaningful distinctions
|
||||
|
||||
Do not differentiate names by adding noise words. `ProductInfo`, `ProductData`, `Product` — what's the difference? Same with `getActiveAccount` vs. `getActiveAccountInfo`. If the distinction is real, name the distinction.
|
||||
|
||||
### N4. Searchable, pronounceable
|
||||
|
||||
Single-letter names are acceptable inside short loop scope (`for i in range(...)`). Anywhere else they hurt grep. `MAX_RETRIES` is searchable; `7` is not.
|
||||
|
||||
If you can't read the name aloud in a code review, it's a bad name. `genymdhms` is a real-world example from the book — `generation_timestamp` is the fix.
|
||||
|
||||
### N5. Class names are nouns, method names are verbs
|
||||
|
||||
`User`, `Invoice`, `Account` — classes are things. `saveInvoice`, `computeTotal`, `notifyUser` — methods are actions. A class named `ProcessInvoice` and a method named `Invoice` are both wrong.
|
||||
|
||||
### N6. Banned generic names
|
||||
|
||||
Without a qualifier, these names always violate intention-revealing:
|
||||
|
||||
- `data`, `data2`, `data_final`
|
||||
- `result`, `result_final`
|
||||
- `item`, `value`, `temp`, `obj`, `info`
|
||||
- `helper`, `manager`, `utils`, `common`
|
||||
- `handle_*`, `process_*`, `do_*` (when `*` is also generic)
|
||||
|
||||
Qualified versions are fine: `raw_csv_bytes`, `parsed_invoice`, `dedup_by_email`.
|
||||
|
||||
---
|
||||
|
||||
## Functions
|
||||
|
||||
### F1. Small. Then smaller.
|
||||
|
||||
Target ≤20 lines. Uncle Bob's harder pass says 2–4 lines is the goal. If a function does not fit on a screen, it does too much. Extract.
|
||||
|
||||
### F2. Do one thing
|
||||
|
||||
A function does one thing when you cannot extract another function from it with a name that is not a restatement of its body. If `compute_invoice` contains a 10-line block that you could meaningfully call `apply_discount`, the original was doing more than one thing.
|
||||
|
||||
### F3. One level of abstraction per function
|
||||
|
||||
Mixing levels is the most common subtle defect. Do not put an HTTP call, a SQL query, a regex parse, and a business rule in the same function — those are four levels.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
renderUserReport(userId):
|
||||
connection = openDatabaseConnection()
|
||||
row = queryUserRow(connection, userId)
|
||||
displayName = row.firstName + " " + row.lastName
|
||||
markup = "<h1>" + displayName + "</h1>"
|
||||
return markup
|
||||
```
|
||||
Four levels: connection, query, formatting, presentation.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
renderUserReport(userId):
|
||||
user = userRepository.findById(userId)
|
||||
return userReportView.render(user)
|
||||
```
|
||||
|
||||
### F4. Step-down rule
|
||||
|
||||
Read a file top-to-bottom; each function is followed by functions one level of abstraction below. Callers above callees. Confirmed by Uncle Bob himself on X.
|
||||
|
||||
### F5. Few arguments
|
||||
|
||||
Zero is best. One is fine. Two is OK. Three "should be avoided." Four or more "requires very special justification" — usually means you should pass a config object.
|
||||
|
||||
At five parameters, stop and extract a request/config object: record, struct, DTO, or equivalent.
|
||||
|
||||
### F6. No flag arguments
|
||||
|
||||
A boolean parameter that switches behavior is always wrong. Split into two functions.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
render(invoice, asHtml):
|
||||
if asHtml:
|
||||
...
|
||||
else:
|
||||
...
|
||||
```
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
renderInvoiceHtml(invoice)
|
||||
renderInvoicePdf(invoice)
|
||||
```
|
||||
|
||||
The same applies to `mode="x"` string enums when the mode changes behavior. If `mode` parameterizes data (locale, currency), it's fine. If it parameterizes *which function runs*, split.
|
||||
|
||||
### F7. No output arguments / Command-Query Separation
|
||||
|
||||
A function either returns a value (query) or has a side effect (command). Never both.
|
||||
|
||||
**Bad:**
|
||||
```text
|
||||
save(record) -> boolean
|
||||
// Returns true if saved, false if record was not found.
|
||||
```
|
||||
What does the bool mean? Success? Found-ness? The caller can't tell.
|
||||
|
||||
**Good:**
|
||||
```text
|
||||
save(record)
|
||||
recordExists(recordId) -> boolean
|
||||
```
|
||||
|
||||
### F8. No side effects in queries
|
||||
|
||||
A getter-style, finder-style, or predicate-style function must not mutate state. If it caches, log the cache write at debug level; do not change observable behavior.
|
||||
|
||||
### F9. Prefer exceptions to return codes
|
||||
|
||||
`if save(x):` is a code smell. Either save succeeds (returns nothing) or it raises (`InvoiceSaveError`). Return codes proliferate up the call stack and get forgotten; exceptions can't be ignored silently.
|
||||
|
||||
### F10. Duplication is the root evil
|
||||
|
||||
If two functions share a non-trivial block, extract it. But — see [dry-kiss-yagni.md](dry-kiss-yagni.md) for when this is wrong (Sandi Metz's "wrong abstraction" caveat).
|
||||
|
||||
---
|
||||
|
||||
## Self-check for naming and functions
|
||||
|
||||
Before you ship code:
|
||||
|
||||
1. Do all names answer "what does this represent" without a comment?
|
||||
2. Are functions ≤20 lines?
|
||||
3. Are functions doing one thing? (Can you extract another function with a non-restating name? If yes, you're doing more than one.)
|
||||
4. Are mixed abstraction levels eliminated?
|
||||
5. Are there any functions with >4 parameters? Extract a config object.
|
||||
6. Are there boolean flag arguments? Split.
|
||||
7. Do any functions both return a value *and* mutate state in a way callers depend on? Split.
|
||||
+150
@@ -0,0 +1,150 @@
|
||||
# Review-Mode Checklist
|
||||
|
||||
When the user asks you to **review, audit, critique, or rate code** (rather than write it), follow this structured walk-through. Do not edit the code unless asked. Produce a findings report.
|
||||
|
||||
## Contents
|
||||
|
||||
- Output format
|
||||
- Pre-flight: is this a refactor or a rewrite?
|
||||
- Walk order
|
||||
- Section A: naming and functions
|
||||
- Section B: comments and formatting
|
||||
- Section C: SOLID
|
||||
- Section D: DRY, KISS, YAGNI
|
||||
- Section E: AI failure modes
|
||||
- What to do with each finding
|
||||
- When the review is contested
|
||||
- What this review does not do
|
||||
|
||||
## Output format
|
||||
|
||||
Use this template exactly. The headings make findings easy to triage.
|
||||
|
||||
```
|
||||
# Code review: <file or scope>
|
||||
|
||||
## Summary
|
||||
<2–3 sentence verdict: ship / needs work / rewrite>
|
||||
Counts: <N> critical, <M> important, <K> nits (must equal the findings listed below)
|
||||
|
||||
## Critical findings
|
||||
<must-fix before merge; omit this heading if none>
|
||||
- `<file>:<line>` — <tag>: <what's wrong> [`<quoted code or behavior>`]. Fix: <concrete change>.
|
||||
<continuation line only when the fix is code-sized>
|
||||
|
||||
## Important findings
|
||||
<should fix but not blocking; omit if none>
|
||||
- ...
|
||||
|
||||
## Nits
|
||||
<style, naming, minor structure; max 3, each with a fix; omit if none>
|
||||
- ...
|
||||
|
||||
## What's good
|
||||
<0–3 genuine, specific positives; omit on a clean review — do not manufacture praise>
|
||||
|
||||
## Coverage
|
||||
One line per section: the findings it produced, or `clean` (walked it, found nothing). A blank section is an unbacked claim, not a pass — fill it before delivering.
|
||||
- Section A (naming & functions): <findings, or `clean`>
|
||||
- Section B (comments & formatting): <findings, or `clean`>
|
||||
- Section C (SOLID): <findings, or `clean`>
|
||||
- Section D (DRY/KISS/YAGNI): <findings, or `clean`>
|
||||
- Section E (AI failure modes): <findings, or `clean`>
|
||||
```
|
||||
|
||||
Severity:
|
||||
- **Critical** — security, correctness, data loss, swallowed exceptions, hardcoded "success" returns.
|
||||
- **Important** — design defects with maintenance cost: SOLID violations, premature abstractions, parameter explosion, generic naming.
|
||||
- **Nit** — style, single-letter names outside loops, missing docstring contracts on public APIs.
|
||||
|
||||
Every finding carries its quoted code or observed behavior and a named fix — that is what lets the author contest it; with no quote or no fix it is not a finding, so drop it. Report only counted findings: never an estimated quality score, "X% cleaner," or a maintainability index — no baseline exists, so the number would be invented.
|
||||
|
||||
## Pre-flight: is this a refactor or a rewrite?
|
||||
|
||||
Before walking the sections, classify the review:
|
||||
|
||||
- **Refactor review:** the user wants the code to be cleaner, not different. **Observable behavior must not change** — same inputs, same outputs, same exceptions, same side effects. If you'd suggest a change that alters behavior, mark it as a *separate finding* labelled "Behavior change — confirm with author" and do not bundle it with refactor recommendations. Refactoring is *"a change made to the internal structure of software... without changing its observable behavior"* (Fowler, *Refactoring*).
|
||||
- **Code-review for correctness:** the user wants you to find bugs. Behavior changes are in scope. Flag them at Critical severity if they affect the contract.
|
||||
|
||||
If you can't tell which one the user wants, ask before writing the review.
|
||||
|
||||
## Walk order
|
||||
|
||||
### Section A — naming and functions
|
||||
|
||||
Pull [naming-and-functions.md](naming-and-functions.md) if you need source citations.
|
||||
|
||||
1. Scan all identifiers. Flag generic ones: `data`, `result`, `item`, `temp`, `value`, `obj`, `info`, `helper`, `manager`, `utils`, `handle_*`, `process_*`, `do_*` without qualifier.
|
||||
2. For each function: lines ≤20? params ≤4? one thing? one level of abstraction? Flag violations.
|
||||
3. Flag boolean flag arguments.
|
||||
4. Flag functions that both return value *and* mutate observable state ambiguously (CQS violation).
|
||||
5. Flag getter-style or predicate-style functions that mutate.
|
||||
|
||||
### Section B — comments and formatting
|
||||
|
||||
Pull [comments-and-formatting.md](comments-and-formatting.md) if needed.
|
||||
|
||||
1. Flag every comment that paraphrases the code below it.
|
||||
2. Flag commented-out code blocks.
|
||||
3. Flag step-number, "First...", or "Then..." scaffolding comments.
|
||||
4. Flag docstrings that restate the signature with no contract.
|
||||
5. Flag style inconsistencies with the surrounding file (casing, quoting, import order).
|
||||
|
||||
### Section C — SOLID
|
||||
|
||||
Pull [solid.md](solid.md) if needed.
|
||||
|
||||
1. (SRP) Any class with methods serving two unrelated stakeholder groups?
|
||||
2. (OCP) Conditional or switch dispatch on a type tag that grew with the codebase?
|
||||
3. (LSP) Any subclass with an unimplemented/unsupported-operation failure, strengthened preconditions, or weakened postconditions?
|
||||
4. (ISP) Any interface where the concrete client uses only a subset of methods?
|
||||
5. (DIP) High-level module importing a concrete from a low-level module? Abstractions living in the same package as the concrete?
|
||||
|
||||
### Section D — DRY, KISS, YAGNI
|
||||
|
||||
Pull [dry-kiss-yagni.md](dry-kiss-yagni.md) if needed.
|
||||
|
||||
1. (DRY) ≥5-line duplicated blocks. Confirm it's knowledge duplication before recommending extraction.
|
||||
2. (DRY-Metz) Wrong abstractions: per-caller branches and special-case flags accumulating in a shared function.
|
||||
3. (KISS) Any function with cyclomatic >10 or nesting >5? (Estimate from branches and loops; you don't need exact metrics.)
|
||||
4. (YAGNI) Optional parameters never called, config flags with one path, abstractions with one implementation, wrappers around libraries that "make them swappable."
|
||||
|
||||
### Section E — AI failure modes (highest leverage)
|
||||
|
||||
Pull [ai-failure-modes.md](ai-failure-modes.md) for every check here.
|
||||
|
||||
1. Any catch-all error handler that swallows without recovery? Critical.
|
||||
2. Any defensive guards for types/values the system already excludes — *inside* a trust boundary? (Validation of external or untrusted input at the boundary is not a defensive guard; do not flag it.)
|
||||
3. Any premature abstraction — interface or factory with one implementation?
|
||||
4. Any comment pollution — line-by-line restating, step-number scaffolding, or documentation comments that paraphrase signatures?
|
||||
5. Any duplication of logic that already exists in a helper in the same repo?
|
||||
6. Any imports or library methods you should verify exist in the installed version?
|
||||
7. Any generic naming (cross-check with Section A).
|
||||
8. Any long function mixing concerns (cross-check with Section A).
|
||||
9. Any 5+ parameter functions without a config object (cross-check with Section A).
|
||||
10. Any inconsistency with surrounding file style (cross-check with Section B).
|
||||
11. Any dead code, unused imports, unreachable branches, half-implementations?
|
||||
12. **Any hardcoded "success" returns, mock fixtures, fake values in production code?** Critical.
|
||||
13. Any code that looks copy-pasted from a similar function (off-by-one, wrong null semantic)?
|
||||
14. Any speculative configurability — flags, env vars, optional params without callers?
|
||||
15. **Any "simplification" that deleted boundary validation, or a cleanup path (`finally`/`close`/`defer`/context-manager) the contract relied on?** That's a behavior change, not cleanup. Critical.
|
||||
|
||||
## What to do with each finding
|
||||
|
||||
A finding must name its fix — a code change OR a specific structural action. "Nameable" is the bar, not "codeable." No named fix means it stays vague unease, not a finding — drop it.
|
||||
|
||||
- ❌ "This error handling could perhaps be more specific." (no named replacement — drop it)
|
||||
- ✅ "`L42 except Exception` swallows the DB error → catch `OperationalError`, let the rest propagate."
|
||||
- ✅ "`L88–140 processOrder` mixes validation, pricing, persistence → extract `validate()` and `price()`."
|
||||
|
||||
For each finding: quote the offending code (file + line), name the principle or AI failure mode in `references/`, give the fix (code if small, a named structural action if not), and assign severity (Critical / Important / Nit).
|
||||
|
||||
## When the review is contested
|
||||
|
||||
If the user pushes back on a finding, cite the source from the relevant `references/` file. The rules trace to primary sources (Uncle Bob, Fowler, Hunt & Thomas, McCabe, Metz) and published 2024–2026 research on LLM code generation. If the user has a context-specific reason to override, record it as an inline comment that names **the principle, the reason, and a revisit trigger** — e.g. `// clean-code exception: 4-arg ceiling — config DTO, all fields required at construction; revisit when an optional field appears.` (the prefix is illustrative, not a required tag). On a later pass a well-formed marker downgrades the finding to *Documented exception* — don't re-flag it; a marker with **no revisit trigger is itself a finding**, since an exception with no exit is just deferred debt. Name the principle, not a rule number — a number is meaningless to a future reader.
|
||||
|
||||
## What this review does not do
|
||||
|
||||
- Run linters or formatters. That's tooling.
|
||||
- Execute the code or run tests. Add a finding instead: *"No tests for the new `charge` path — recommend adding."*
|
||||
- Enforce language-specific style (Black, Prettier, PHPCS). Defer to the project's style tooling unless the user explicitly asks.
|
||||
+280
@@ -0,0 +1,280 @@
|
||||
# SOLID — the five principles
|
||||
|
||||
Source: Robert C. Martin. The five principles were collected on Uncle Bob's "Principles of OOD" page on objectmentor.com (mirrored at butunclebob.com) and updated on blog.cleancoder.com. Original papers from *C++ Report* circa 1995–1996.
|
||||
|
||||
## Contents
|
||||
|
||||
- S: Single Responsibility Principle
|
||||
- O: Open/Closed Principle
|
||||
- L: Liskov Substitution Principle
|
||||
- I: Interface Segregation Principle
|
||||
- D: Dependency Inversion Principle
|
||||
- How AI-generated code typically breaks SOLID
|
||||
- Self-check for SOLID
|
||||
|
||||
---
|
||||
|
||||
## S — Single Responsibility Principle
|
||||
|
||||
**Definition (Martin 2014, hardened from the original).** *"A module should be responsible to one, and only one, actor."*
|
||||
|
||||
Older form: "A class should have only one reason to change."
|
||||
|
||||
Source: blog.cleancoder.com — SRP, 2014.
|
||||
|
||||
### Why
|
||||
|
||||
The axis is *people*. Different stakeholders (Accounting, HR, DBA) want different things from the same class. When their needs change, they edit the same file, conflict, and break each other.
|
||||
|
||||
### Smells to flag
|
||||
|
||||
- One class contains methods touching unrelated subsystems (persistence + presentation + business rules).
|
||||
- Methods on the class serve disjoint stakeholder groups.
|
||||
- Git history shows two distinct clusters of co-changing methods inside one class.
|
||||
|
||||
### Common misinterpretation
|
||||
|
||||
*"A class should do one thing."* No. SRP is about **cohesion around an actor**, not method count. A 12-method `InvoiceRepository` answerable only to the data-access layer satisfies SRP. A 3-method class with one HTTP call, one Jinja render, and one DB write does not.
|
||||
|
||||
### Bad
|
||||
|
||||
```text
|
||||
EmployeeReport
|
||||
calculatePay() // Accounting
|
||||
reportHours() // HR
|
||||
save() // Data storage owner
|
||||
```
|
||||
|
||||
### Good
|
||||
|
||||
```text
|
||||
PayCalculator // Accounting
|
||||
HoursReporter // HR
|
||||
EmployeeRepository // Data storage owner
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## O — Open/Closed Principle
|
||||
|
||||
**Definition.** *"Software entities (classes, modules, functions) should be open for extension, but closed for modification."*
|
||||
|
||||
Originally Bertrand Meyer (1988, *Object-Oriented Software Construction*); Martin refocused it on polymorphic abstraction rather than implementation inheritance.
|
||||
|
||||
Source: blog.cleancoder.com — OCP, 2014; Martin's 1996 paper PDF (Duke mirror).
|
||||
|
||||
### Why
|
||||
|
||||
Protect stable high-level policy from churn in low-level variants. New behavior should arrive as new code, not edits to working code.
|
||||
|
||||
### Smells to flag
|
||||
|
||||
- Branch dispatching on a type tag or runtime type check — every new type requires editing the same function.
|
||||
- Adding a feature requires modifying N existing files instead of adding one.
|
||||
- `match`/`enum` switches that cross module boundaries (policy reaching into details).
|
||||
|
||||
### Common misinterpretation
|
||||
|
||||
*"Never modify code."* The principle is that *modules containing high-level policy* should not be modified to accommodate new variants. Leaf code changes freely.
|
||||
|
||||
### Bad
|
||||
|
||||
```text
|
||||
export(record, kind):
|
||||
if kind == "pdf": return toPdf(record)
|
||||
if kind == "csv": return toCsv(record)
|
||||
if kind == "json": return toJson(record)
|
||||
// adding "xml" requires editing this function
|
||||
```
|
||||
|
||||
### Good
|
||||
|
||||
```text
|
||||
exporters = {
|
||||
"pdf": toPdf,
|
||||
"csv": toCsv,
|
||||
"json": toJson,
|
||||
}
|
||||
|
||||
export(record, kind):
|
||||
return exporters[kind](record)
|
||||
// adding "xml" is one line in the table
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## L — Liskov Substitution Principle
|
||||
|
||||
**Definition (Liskov & Wing, 1994).** *"If for each object o1 of type S there is an object o2 of type T such that for all programs P defined in terms of T, the behavior of P is unchanged when o1 is substituted for o2, then S is a subtype of T."*
|
||||
|
||||
Source: Martin's LSP paper PDF (LaBRI mirror).
|
||||
|
||||
### Why
|
||||
|
||||
Substitutability. Callers written against a base type must continue to work when handed a subtype — otherwise polymorphism leaks abstraction.
|
||||
|
||||
### Smells to flag
|
||||
|
||||
- A subclass overrides a method to signal "not implemented" or "unsupported operation."
|
||||
- A subclass **strengthens preconditions** (rejects inputs the parent accepts).
|
||||
- A subclass **weakens postconditions** (returns something the parent guarantees against).
|
||||
- Callers perform runtime subtype checks to decide whether to call a method.
|
||||
|
||||
### Common misinterpretation
|
||||
|
||||
*"Subclasses must have the same methods."* That's signature compatibility, which is just type-checking. LSP is **behavioral**:
|
||||
- Preconditions can only **weaken** in the subtype.
|
||||
- Postconditions and invariants can only **strengthen**.
|
||||
- Parameter types are contravariant; return types covariant.
|
||||
|
||||
### The Rectangle/Square classic
|
||||
|
||||
```text
|
||||
Rectangle
|
||||
setWidth(width)
|
||||
setHeight(height)
|
||||
area()
|
||||
|
||||
Square extends Rectangle
|
||||
setWidth(width):
|
||||
this.width = width
|
||||
this.height = width // invariant: width == height
|
||||
setHeight(height):
|
||||
this.width = height // invariant: width == height
|
||||
this.height = height
|
||||
```
|
||||
A caller holding a `Rectangle` reference does `r.set_width(5); r.set_height(4); assert r.area() == 20`. With a `Square`, the assertion fails — LSP violated.
|
||||
|
||||
The fix is *not* to fix the methods. The fix is that `Square is-not-a Rectangle` in the behavioral sense. Compose, don't inherit.
|
||||
|
||||
---
|
||||
|
||||
## I — Interface Segregation Principle
|
||||
|
||||
**Definition.** *"Clients should not be forced to depend on methods they do not use."* Equivalently: many client-specific interfaces beat one general-purpose interface.
|
||||
|
||||
Source: Martin's 1996 ISP paper, catalogued at butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod.
|
||||
|
||||
### Why
|
||||
|
||||
Fat interfaces create transitive coupling. Clients are dragged into recompiles and test-fixtures for methods they never call.
|
||||
|
||||
### Smells to flag
|
||||
|
||||
- A `Service` / `Manager` / `Repository` interface with 10+ methods, where any given caller uses one or two.
|
||||
- Implementations that stub half the methods with no-op bodies, null/empty placeholders, or unimplemented failures (usually co-occurs with an LSP violation).
|
||||
- One mock object reconfigured differently across tests because the interface is too broad.
|
||||
|
||||
### Common misinterpretation
|
||||
|
||||
*"Make interfaces small."* As a count rule, no. ISP is **client-centric** — segregation is driven by *the set of methods a particular client uses*, not by an arbitrary method-count ceiling. Two clients with identical method needs can share one interface even if it has 20 methods.
|
||||
|
||||
### Bad
|
||||
|
||||
```text
|
||||
UserService
|
||||
create(...)
|
||||
read(...)
|
||||
update(...)
|
||||
delete(...)
|
||||
email(...)
|
||||
notify(...)
|
||||
audit(...)
|
||||
export(...)
|
||||
```
|
||||
The audit logger only needs `audit`. It now depends transitively on the email and export subsystems.
|
||||
|
||||
### Good
|
||||
|
||||
```text
|
||||
UserAuditor
|
||||
audit(...)
|
||||
|
||||
UserNotifier
|
||||
notify(...)
|
||||
```
|
||||
|
||||
Implementations can satisfy multiple protocols. Callers depend only on what they use.
|
||||
|
||||
---
|
||||
|
||||
## D — Dependency Inversion Principle
|
||||
|
||||
**Definition (verbatim, two clauses).**
|
||||
*(a) High-level modules should not depend on low-level modules. Both should depend on abstractions.*
|
||||
*(b) Abstractions should not depend on details. Details should depend on abstractions.*
|
||||
|
||||
Source: Martin's 1996 *C++ Report* paper, archived at Wayback / objectmentor.com.
|
||||
|
||||
### Why
|
||||
|
||||
Control the direction of the import graph. Policy must not transitively `import` mechanism, or policy becomes un-reusable and untestable.
|
||||
|
||||
### Smells to flag
|
||||
|
||||
- A high-level module imports a concrete low-level client inside business logic.
|
||||
- A constructor that `new`/instantiates concrete collaborators instead of accepting them as parameters.
|
||||
- Abstractions defined in the *low-level* package (the interface lives next to its database or service implementation) — ownership reversed. The interface should live in the **client's** package.
|
||||
- Function signatures typed against concrete classes instead of interfaces, protocols, or abstract contracts.
|
||||
|
||||
### Common misinterpretation
|
||||
|
||||
*"DIP means use a DI container."* No. DIP is about **the direction of source-code dependencies**. You can satisfy DIP with plain constructor injection and no framework; you can violate DIP while using Spring.
|
||||
|
||||
### Bad
|
||||
|
||||
```text
|
||||
// billing/charge — high-level policy
|
||||
import SqlUserRepository // concrete import
|
||||
|
||||
chargeUser(userId, amount):
|
||||
repository = new SqlUserRepository() // concrete instantiation
|
||||
user = repository.get(userId)
|
||||
...
|
||||
```
|
||||
|
||||
### Good
|
||||
|
||||
```text
|
||||
// billing/user-repository — abstraction lives WITH the client (billing)
|
||||
UserRepository
|
||||
get(userId) -> User
|
||||
|
||||
// billing/charge
|
||||
chargeUser(userId, amount, repository: UserRepository):
|
||||
user = repository.get(userId)
|
||||
...
|
||||
|
||||
// sql/user-repository — detail depends on the abstraction
|
||||
SqlUserRepository satisfies UserRepository
|
||||
get(userId) -> User
|
||||
```
|
||||
|
||||
The import arrows go: `sql → billing` (detail → abstraction). They do not go `billing → sql`.
|
||||
|
||||
---
|
||||
|
||||
## How AI-generated code typically breaks SOLID
|
||||
|
||||
Mapped to the principle each breaks:
|
||||
|
||||
1. **God-module** from "do everything in one file" prompts — SRP + DIP + usually OCP.
|
||||
2. **Type-tag dispatch chains** (`if kind == "pdf": ...`) — OCP.
|
||||
3. **Unsupported-operation stubs in subclasses** when asked to "implement only the methods we need" — LSP + ISP.
|
||||
4. **Concrete SDK/client imports at module load time** — DIP, hard to test without patching the runtime.
|
||||
5. **Mega-`Service` interfaces** with create/read/update/delete/email/notify/audit/export — ISP, usually SRP too.
|
||||
6. **Silent precondition strengthening on override** — defensive-looking, breaks LSP because callers holding the base type now crash on previously-valid inputs.
|
||||
7. **Invariant-breaking "convenience" subclasses** (e.g., `ReadOnlyList(list)` overriding `append` to no-op) — LSP.
|
||||
8. **Inverted ownership of abstractions** — putting the interface/protocol/abstract contract in the same file as the concrete implementation. Cosmetic DIP fix, real dependency graph unchanged.
|
||||
|
||||
---
|
||||
|
||||
## Self-check for SOLID
|
||||
|
||||
Before you ship code:
|
||||
|
||||
1. (SRP) Does any class in the diff answer to more than one stakeholder group?
|
||||
2. (OCP) Does any change require a type-tag branch added to an existing function? Could it be data-driven (registry/strategy) instead?
|
||||
3. (LSP) Does any new subclass signal "not implemented", tighten preconditions, or weaken postconditions?
|
||||
4. (ISP) Does any interface have a method your concrete client doesn't use?
|
||||
5. (DIP) Does the high-level package import the low-level concrete? Where do new abstractions live — with the client or with the implementation?
|
||||
+47
@@ -0,0 +1,47 @@
|
||||
# Sources
|
||||
|
||||
Central bibliography for `clean-code-guard`. Other reference files use source
|
||||
names instead of inline URLs so rule guidance stays readable.
|
||||
|
||||
## Contents
|
||||
|
||||
- Classic clean code and design references
|
||||
- LLM code-generation research and field reports
|
||||
- Metrics and verification references
|
||||
|
||||
## Classic Clean Code And Design
|
||||
|
||||
- **Clean Code sample chapters**: https://ptgmedia.pearsoncmg.com/images/9780132350884/samplepages/9780132350884.pdf
|
||||
- **Command Query Separation, Fowler**: https://martinfowler.com/bliki/CommandQuerySeparation.html
|
||||
- **Refactoring, Fowler**: https://martinfowler.com/books/refactoring.html
|
||||
- **YAGNI, Fowler**: https://martinfowler.com/bliki/Yagni.html
|
||||
- **DRY, The Pragmatic Programmer excerpt**: https://media.pragprog.com/titles/tpp20/dry.pdf
|
||||
- **The Wrong Abstraction, Sandi Metz**: https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction
|
||||
- **SOLID principles, Uncle Bob archive**: http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod
|
||||
- **SRP, Uncle Bob**: https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html
|
||||
- **OCP, Uncle Bob**: https://blog.cleancoder.com/uncle-bob/2014/05/12/TheOpenClosedPrinciple.html
|
||||
- **LSP, Liskov/Wing paper mirror**: https://www.labri.fr/perso/clement/enseignements/ao/LSP.pdf
|
||||
- **DIP, Object Mentor archive**: https://web.archive.org/web/20150924054349/http://www.objectmentor.com/resources/articles/Principles_and_Patterns.pdf
|
||||
|
||||
## LLM Code-Generation Research And Field Reports
|
||||
|
||||
- **GitClear AI code quality 2025**: https://www.gitclear.com/ai_assistant_code_quality_2025_research
|
||||
- **Package Hallucinations, Spracklen et al., USENIX Security 2025**: https://www.usenix.org/system/files/conference/usenixsecurity25/sec25cycle1-prepub-742-spracklen.pdf
|
||||
- **AI-Generated Code Considered Harmful**: https://arxiv.org/abs/2409.19182
|
||||
- **A Deep Dive Into LLM Code Generation Mistakes**: https://arxiv.org/abs/2411.01414
|
||||
- **Patterns for Reducing Friction in AI-Assisted Development, Fowler**: https://martinfowler.com/articles/reduce-friction-ai/
|
||||
- **Claude Code issue 6984, mock data generation bias**: https://github.com/anthropics/claude-code/issues/6984
|
||||
- **Karpathy on exception suppression**: https://x.com/karpathy/status/1976077806443569355
|
||||
- **Code Needs Comments**: https://arxiv.org/html/2402.13013v1
|
||||
- **Evaluating the Code Quality of AI-Assisted Code Generation Tools (Copilot, CodeWhisperer, ChatGPT)**: https://arxiv.org/abs/2304.10778
|
||||
- **On Mitigating Code LLM Hallucinations with API Documentation**: https://arxiv.org/abs/2407.09726
|
||||
- **LLM Hallucinations in Practical Code Generation (taxonomy)**: https://arxiv.org/abs/2409.20550
|
||||
- **When Names Disappear: Revealing What LLMs Actually Understand About Code**: https://arxiv.org/abs/2510.03178
|
||||
- **Neural Variable Name Repair**: https://arxiv.org/abs/2512.01141
|
||||
- **Vibe Coding in Practice**: https://arxiv.org/abs/2512.11922
|
||||
|
||||
## Metrics And Verification
|
||||
|
||||
- **McCabe NIST 235r**: https://www.mccabe.com/pdf/mccabe-nist235r.pdf
|
||||
- **Cognitive Complexity, SonarSource**: https://www.sonarsource.com/docs/CognitiveComplexity.pdf
|
||||
- **ReSharper cyclomatic complexity threshold guidance**: https://github.com/JetBrains/resharper-cyclomatic-complexity/blob/master/docs/ThresholdGuidance.md
|
||||
Reference in New Issue
Block a user