Clean Code Guard

clean-code-guard

You are reviewing generated or changed code before it ships. Apply the rules below as a guard pass after the first implementation pass. If the user explicitly invokes this skill before writing code, use the same rules while writing and still run the self-check before delivery.

Compatibility

This is a portable instruction skill. It requires no MCP server, network access, API key, shell command, local executable, or bundled script. It can be used in any runtime that supports SKILL.md plus directly linked references/ files; agents/openai.yaml is lightweight display metadata.

This skill does not replace project linters, formatters, type checkers, or test runners. Use the project's own tools for mechanical verification; use this skill for the judgement layer around code quality and review.

How to use this skill

This skill has two modes — pick based on the user's request.

Guard-pass mode (recommended): after code has been generated, edited, refactored, or fixed, check the diff or target files against the Always-applied imperatives below. Fix violations before presenting, committing, or merging the work.

Live mode (explicit): when the user invokes this skill before a risky code edit, apply the same imperatives while writing, then run the Self-check before delivery checklist. If you violate any rule, fix it before showing the user.

Review mode (triggered when the user asks you to review, audit, critique, or rate code): walk references/review-checklist.md against the target file(s) and produce a structured findings report. Do not edit code in review mode unless asked.

For both modes, the rule bodies live in references/. Read the relevant reference file when:

• You hit a rule you don't fully remember the reasoning for.
• The user pushes back on a rule and you need the source citation.
• You're in review mode and need the full checklist.
• The code under review touches a specific principle (e.g., subclassing → references/solid.md; deduplication → references/dry-kiss-yagni.md).

The reference files are:

• references/naming-and-functions.md — names, function size, parameters, command/query separation.
• references/comments-and-formatting.md — when to comment, when to delete, matching neighbor style.
• references/solid.md — SRP, OCP, LSP, ISP, DIP with the modern phrasings and detection smells.
• references/dry-kiss-yagni.md — knowledge vs code duplication, Sandi Metz's re-inline rule, McCabe complexity, Fowler's YAGNI cost categories.
• references/ai-failure-modes.md — the 14 systematic ways LLMs produce bad code. Read this one first if you are an AI agent reading this skill. It is the highest-leverage file in the skill.
• references/review-checklist.md — structured walk-through for review mode.
• references/sources.md — central bibliography for source URLs. Read it only when you need to verify or cite an external source.

Examples

• A coding agent implements an endpoint: use guard-pass mode on the diff before

the work is presented or committed.

• User asks "review this PR" or "should I merge this?": use review mode and

report findings from references/review-checklist.md; do not edit unless asked.

• User asks "implement this endpoint using clean-code-guard": use live mode

while writing, then run the self-check before delivery.

• User asks "refactor this function, same behavior": preserve observable

behavior exactly and treat any bug fix as a separate change.

Success criteria

This skill is working when code-writing tasks avoid the listed failure modes, code-review tasks produce prioritized findings with concrete evidence, and refactors preserve behavior unless the user explicitly asks for a behavior change. It should stay silent for conceptual, CI, git workflow, prose, data analysis, and test-running tasks covered by the frontmatter exclusions.

Why this skill exists

LLM-generated code has measurable, systematic failure modes that generic "follow clean code" instructions do not catch. Examples backed by published research:

• Code duplication grew 8x in tracked codebases between 2021 and 2024 (GitClear 2025 report).
• Package hallucination rate averages 19.6% across 16 models (Spracklen et al., USENIX Security '25).
• LLMs often wrap risky operations in broad catch-all handlers that swallow errors (Karpathy).
• AI agents "declare success despite failing tests" by returning hardcoded fixture values (Fowler, Patterns for Reducing Friction).
• Function size grew from 142 to 267 LoC, cyclomatic complexity from 4.2 to 8.1 in AI-assisted commits (GitClear).

The classic principles (Clean Code, SOLID, DRY/KISS/YAGNI) are still the foundation — but this skill adds the AI-specific layer most rule packs miss.

Always-applied imperatives

These are the rules to follow on every code change. They are imperative, not suggestions.

Functions and names

1. Names reveal intent. Never use data, data2, result, result_final, item, temp, value, obj, info, helper, manager, utils, or handle_*/process_*/do_* without a qualifier. A name must answer why it exists and what it does. (Clean Code Ch. 2) 2. Functions stay small. Target ≤20 lines, one level of abstraction, one thing. If you can extract a function with a name that doesn't restate the body, the parent was doing more than one thing. (Clean Code Ch. 3) 3. Four arguments is the hard ceiling. At five, stop and introduce a request/config object (record, struct, DTO, or equivalent). Never use boolean flag arguments — split into two functions instead. 4. No output arguments. A function either returns a value (query) or has a side effect (command). Never both. Command names use verbs; query names use nouns or getter-style names. (CQS)

Comments and structure

5. *Comments explain why, never what. Delete any comment that paraphrases the line below it. Delete step-number scaffolding comments. Delete commented-out code — version control exists. (Clean Code Ch. 4) 6. Match the file's existing style.* Read the file you're editing and at least one neighbor before writing. Mirror the casing, import order, error handling, logging, and HTTP/DB client choices. Do not introduce a second pattern.

SOLID

7. One actor per module. A class should be answerable to one stakeholder group (Accounting, Auth, Reporting). If two unrelated subsystems both reach into the same class, split it. (SRP, Uncle Bob 2014) 8. Extension via new code, not edits. If adding a new variant requires another type-tag branch in an existing function, refactor to a registry, strategy, or polymorphic dispatch first. (OCP) 9. No subclass refuses its parent's contract. Never override a method to signal "not implemented" or "unsupported operation." Never strengthen preconditions or weaken postconditions in an override. If you need to do that, the inheritance is wrong. (LSP) 10. Abstractions live with the client, not the implementation. When you introduce an interface, protocol, or abstract contract, put it in the package that consumes it, not next to the concrete class. (DIP)

DRY, KISS, YAGNI

11. *Delete duplicated knowledge, not duplicated text. Two functions that look alike but encode different rules are not a DRY violation. One rule expressed in code + docs + schema is. (Pragmatic Programmer, "DRY") 12. The wrong abstraction is worse than duplication. If an abstraction has accumulated branches for each caller's special case, re-inline it back into callers, then delete the dead branches before re-abstracting. (Sandi Metz, "The Wrong Abstraction") 13. Complexity ceiling: cyclomatic ≤10, nesting depth ≤5. Refactor before exceeding. (McCabe 1976) 14. No speculative anything. No optional parameter, config flag, env var, feature toggle, interface, factory, or base class without a present-day caller. If you find yourself adding `enable_, use__v2`, or `_mode`, delete it and ship the concrete behavior. (Fowler, "Yagni")

AI-specific guardrails — the highest-leverage section

15. Never swallow errors with broad catch-all handling. Catch only the specific error type you can recover from. If you cannot recover, let the error propagate. Returning null/none/empty success from a catch handler is forbidden unless the function contract documents that behavior. (Karpathy) 16. No defensive guards for impossible cases. Do not add null checks or runtime type checks for values whose declared type or caller contract already excludes that case. Trust the contract. (arXiv 2409.19182) 17. Verify every import and external call. Before calling a method on a library, confirm it exists in the version installed (read the package, check the lockfile, or import and inspect). Do not generate code based on what the API "should" look like. (USENIX Security '25) 18. No hardcoded "success" returns or mock fixtures in production code. Never return {"status": "ok", ...} or canned data from a function whose spec says it does real work. If you cannot implement, fail explicitly with the language's unimplemented or unsupported-operation mechanism and say so. Never disable, skip, or weaken a test to make it pass. (Fowler, Claude Code issue #6984) 19. Re-derive, do not copy from similar. When tempted to copy a function and modify it, stop. Re-derive from the spec. Off-by-one and wrong-null-semantic bugs almost always enter through copy-from-similar. (arXiv 2411.01414) 20. Enumerate boundary cases before writing them. For any range, off-by-one, null/empty/one/many, even/odd, or unicode/byte boundary, write the case list in a comment first. Cover each case in code before moving on. 21. Strip dead code before delivery. Run a linter or grep pass for unused imports, unused symbols, unreachable branches, and "just in case" exports. Remove them. A function that nothing calls today does not get to live for "someday." 22. Read before write. Before writing in an unfamiliar repo, read the file you'll edit, one neighbor, and any project rules file (CLAUDE.md, AGENTS.md, README's "conventions" section). Use the project's existing helpers, error types, and logging.

Refactoring discipline

23. Preserve observable behavior when refactoring. When the user asks you to clean up, simplify, or refactor existing code, do not change the contract — same inputs produce the same outputs, same exceptions raised, same side effects, same ordering guarantees. If you spot a bug while refactoring, flag it separately and ask before changing it. Refactoring is defined as "a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing its observable behavior" (Fowler, Refactoring). Bug fixes and refactors are two operations — never bundle them in a single change.

Self-check before delivery

Before you show the user the code you wrote or edited:

1. Walk imperatives 1–23 against your diff. Fix every violation. 2. For new functions, count: lines ≤ 20? params ≤ 4? complexity feels ≤ 10? names reveal intent? 3. For new comments, ask: does this explain why? If it explains what, delete it. 4. For new error handling: is the caught error type specific? Does the handler do something other than silently return? 5. For new abstractions (interface, factory, base class, registry): is there a second concrete user today? If no, inline it. 6. Did you read the file you edited and at least one neighbor? Did your style match? 7. Is there any hardcoded "ok" return or fixture data? If yes, replace with real implementation or an explicit unimplemented/unsupported-operation failure. 8. If this is a refactor: did you change observable behavior? If yes, you bundled a bug fix — split it out and ask the user.

If you cannot answer yes to every check, fix before shipping.

When the user pushes back on a rule

Refer them to the source name in the relevant references/ file and use references/sources.md only when the URL is needed. The rules are defensible — they come from primary sources (Uncle Bob, Fowler, Hunt & Thomas, McCabe, Metz) and from published 2024–2026 research on LLM code generation. If the user has a context-specific reason to override (e.g., a constructor genuinely needs 8 params for a config DTO), document the exception in a code comment that includes the principle being overridden and the reason.

Troubleshooting

• If the task is conceptual rather than code-producing, do not apply this skill;

answer the concept directly.

• If review mode starts producing style-only feedback, use

references/review-checklist.md and prioritize behavioral bugs, brittleness, and maintainability risks.

• If a rule conflicts with an explicit project convention, follow the project

convention and document the exception only when it would otherwise surprise a future maintainer.

• If the skill feels too broad, use the frontmatter exclusions first; do not add

runtime-specific rules to this general guard skill.

What this skill does not do

• Run linters or static analysis. Those are tool-level concerns; this skill is about what to write and what to look for.
• Enforce language-specific formatter or linter preferences. Defer to the project's style tooling.
• Replace tests. Clean code passes tests; tests do not pass without clean code, but clean code without tests is also a defect.

AI Failure Modes — the unique value of this skill

This file catalogs 14 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
• 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:

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:

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:

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:

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.

---

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:

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:

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:

// Increment counter by one
counter += 1

// Step 3: return the result
return result

Good:

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:

processData(data):
  result = []
  for item in data:
    temp = item.value * 2
    result.add(temp)
  return result

Good:

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" — canonical symptom is "a function 300 lines long, handling four unrelated concerns, assembled from several 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. Hard caps: ~50 lines, ≤4 parameters, cyclomatic complexity ≤8 — 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:

sendEmail(to, subject, body, retry=true, backoff="exp",
          html=false, fromAddress=null, encoding="utf-8",
          internal=false, verbose=false)

Good:

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:

getUserBalance(userId):
  return 1000  // TODO: actual provider call

Shipped as the implementation, function body is fiction.

Good:

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:

# 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:

renderInvoice(invoice,
              format="pdf", template=null, locale="en",
              includeQr=false, currencyOverride=null,
              debug=false, legacyMode=false)

Only one caller exists. It passes (invoice,).

Good:

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.

---

Cross-cutting observation

Eight of the 14 failure modes (1, 2, 3, 9, 12, 14, 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, solid.md, and 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 14 patterns above are the high-leverage check. Walk them before delivery.

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:

add(a, b)
  // Adds a and b and returns the result.
  return a + b

Good:

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:

// 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 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?

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 standard libraries — custom retry loops, enums, or record-like types when the platform already provides them. 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.

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:

d  // elapsed time in days
ts = []
fn(xs)

Good:

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:

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:

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:

render(invoice, asHtml):
  if asHtml:
    ...
  else:
    ...

Good:

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:

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:

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

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>

## Critical findings
<must-fix before merge>
- **<short title>** — `<file>:<line>`
  Evidence: <quoted code or behavior>
  Principle violated: <e.g., LSP, catch-all error swallowing>
  Suggested fix: <concrete>

## Important findings
<should fix but not blocking>
- ...

## Nits
<style, naming, minor structure>
- ...

## What's good
<at least 2–3 specific positives — naming, structure, test coverage>

## Self-check coverage
- [ ] Walked Section A (naming & functions)
- [ ] Walked Section B (comments & formatting)
- [ ] Walked Section C (SOLID)
- [ ] Walked Section D (DRY/KISS/YAGNI)
- [ ] Walked Section E (AI failure modes)

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.

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 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 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 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 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 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? 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?

What to do with each finding

For each item flagged: 1. Quote the offending code (file + line range). 2. Name the principle or AI failure mode in references/. 3. Propose a concrete fix — code if the change is small, prose if it's structural. 4. 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 (e.g., a 7-parameter constructor for a config DTO is acceptable), document the override in a code comment that names the principle and the reason — and downgrade the finding to "Documented exception" rather than removing it.

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.

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

EmployeeReport
  calculatePay()    // Accounting
  reportHours()     // HR
  save()            // Data storage owner

Good

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

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

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

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

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

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

// billing/charge — high-level policy
import SqlUserRepository  // concrete import

chargeUser(userId, amount):
  repository = new SqlUserRepository()  // concrete instantiation
  user = repository.get(userId)
  ...

Good

// 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?

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

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