---
name: clean-code
description: "Enforces clean code and refactoring discipline: naming, function size, arguments, SOLID, comment policy, dead code. Use when writing new code, refactoring, or when code smells appear - long functions, flag parameters, deep nesting, duplicated logic, vague names."
license: MIT
metadata:
  author: lammanhhoang
  version: "1.0.0"
---

# Clean Code

Numbered rules use RFC-2119 keywords. Cite findings by number ("violates rule #3").
For the full smell catalog (21 named smells with detection signals, recipes, severity),
read references/SMELLS.md — load it when auditing an existing codebase or reviewing a PR,
not when writing a small greenfield change.

## When to use

- Writing new functions, classes, or modules in any language.
- Refactoring, code review, or "clean this up" requests.
- You notice a smell mid-task: a 60-line function, a `doThing(x, true, false)` call, 4 levels of nesting.

When NOT to use: mechanical formatting (that is the linter's job), performance tuning
(profile first, clean second), or one-off throwaway scripts the user says will be deleted.

## The Law of Refactoring

**Never refactor and change behavior in the same commit. Tests MUST be green before AND after.**
If tests do not exist for the code you are about to refactor, write characterization tests first
or explicitly tell the user you are refactoring without a safety net. A refactor commit with a
behavior change hidden inside it is the single most expensive kind of diff to review and revert.

## Rules: Functions

1. A function MUST do one thing at one level of abstraction. Test: if you can extract a
   chunk and give it a name that is not a restatement of the parent's name, the parent does
   too much. Mixed abstraction levels (HTTP parsing next to business math) always fail this.
2. Functions SHOULD take at most 2 positional parameters; when you do exceed 2, collapse
   them into a single options object with named fields — call sites become self-documenting
   and argument-order bugs become impossible.
3. Functions MUST NOT take boolean flag parameters. A flag proves the function does two
   things. Split it into two functions, one per branch (see pair A below).
4. Functions MUST NOT mutate their inputs. Return new values; the caller owns its data.
   Exception: methods mutating `this`, and hot paths where the profiler demanded it — then
   the name MUST warn (`sortInPlace`, not `sort`).
5. Prefer guard clauses and early returns. Nesting MUST NOT exceed 2 levels inside a function
   body (see pair B below).
6. Command-query separation: a function MUST either return data (query) or perform an effect
   (command) — never both. `getUser()` that silently creates a missing user is a landmine.
   Exception: intentionally atomic ops (`pop()`, `getOrCreate` named as such, compare-and-swap).

## Rules: Naming

7. Names MUST be searchable and pronounceable. No magic numbers — `MAX_RETRIES = 3`, not a
   bare `3`. Single letters MAY appear only as loop indices or in lambdas of <=3 lines.
8. Names MUST reveal intent: `elapsedDays` not `d`, `isEligibleForDiscount(user)` not
   `check(u)`. If the name needs a comment to explain it, the name failed.
9. One word per concept per codebase. Pick ONE of fetch/get/retrieve, ONE of
   create/make/build, ONE of delete/remove/destroy — and grep before naming to match the
   existing choice. `fetchUser` next to `getOrder` next to `retrieveInvoice` is three names
   for one concept and readers will assume the difference is meaningful.
10. No encodings or abbreviations: no Hungarian (`strName`), no `I` prefix on interfaces, no
    `Impl`/`Mgr`/`Util` suffixes hiding a missing concept, no `tmp2`/`data3`.
11. Name length MUST be proportional to scope. `i` in a 3-line loop: fine. A package-level
    export named `process`: forbidden — it will collide and says nothing.

## Rules: SOLID (smell -> fix)

Default stance: composition over inheritance. Reach for `extends` only for genuine is-a with
full substitutability; otherwise inject a collaborator.

| # | Principle | Smell (detection) | Fix |
|---|-----------|-------------------|-----|
| 12 | SRP | Class whose job description needs "and" (`UserService` that validates AND persists AND emails) | Split along the "and"s; one reason to change each |
| 13 | OCP | The same `switch (type)` repeated in 3+ files; adding a variant touches all of them | Replace conditional with polymorphism or a strategy map `Record<Type, Handler>` |
| 14 | LSP | Subclass throws `NotImplementedError` / no-ops a parent method (`Square extends Rectangle`) | It is not a subtype. Use composition; extract the honest shared interface |
| 15 | ISP | Clients stub methods they never call to satisfy a fat interface | Split into role interfaces (`Readable`, `Writable`); clients depend only on what they use |
| 16 | DIP | Domain module imports `pg`/`axios`/`fs` directly | Define the port (interface) in the domain; implement the adapter at the edge; inject it |

## Rules: Comments

17. Comments MUST explain WHY, never WHAT: business rules, non-obvious constraints,
    workarounds with a ticket link ("Stripe retries webhooks for 72h, so dedupe by event id
    — see PAY-812"). A comment restating the code MUST be deleted and, if the code was
    unclear, the code renamed instead.
18. DELETE commented-out code on sight. Git remembers. No "might need it later", no
    "keep for reference". Zero exceptions.
19. No journal comments (`// 2024-03-01 fixed by dave`), no attribution comments, no
    closing-brace markers (`} // end if`), no filler (`// constructor`). Version control
    and blame already store all of it.

## Rules: Duplication

20. Rule of three: copy once, tolerate it; on the THIRD occurrence, extract. Abstracting on
    the second occurrence means guessing the axis of variation with n=2 data points.
21. The wrong abstraction costs more than duplication. If a shared helper has grown
    per-caller flags or `if (caller === ...)` branches, inline it back into every call site,
    delete it, and let the duplicates diverge until the real shape appears (recipe 3 below).

## Safe refactor recipes

Each recipe is one commit. Tests green before and after (see Law).

**1. Extract function**
1. Identify the fragment doing a nameable sub-thing; note every local it reads/writes.
2. Create the new function; pass reads as parameters, return the writes.
3. Replace the fragment with a call. Compile + run tests. Commit.

**2. Extract module**
1. Cluster the functions/types that change together (they share a reason to change — rule #12).
2. Move them to a new file; export only what outsiders actually call.
3. Fix imports, delete the old copies, run tests. Commit.

**3. Inline a premature abstraction**
1. Copy the helper's body into each call site, specializing per caller (delete dead branches
   the caller never hit).
2. Delete the helper and its flags/config.
3. Run tests. Only re-extract later if a true common shape emerges (rule #20). Commit.

**4. Strangler fig (legacy replacement)**
1. Put a facade (interface/route/proxy) in front of the legacy unit; route 100% through it.
2. Build the replacement behind the facade; port one behavior at a time, switching routes as
   each slice passes its tests (feature flag if risky).
3. When the legacy unit receives zero traffic, delete it. Facade stays as the seam.

**5. Dead-code removal sweep**
1. Find candidates: compiler/linter unused warnings, coverage-never-hit exports,
   `grep -rn "symbolName"` returning only the definition.
2. Delete — do not comment out (rule #18). Includes unused deps, feature flags fully rolled
   out, and config for deleted features.
3. Full build + test suite. One commit titled "remove dead code", nothing else in it.

## Before / after pairs

**A. Flag parameter (rule #3)**
```ts
// BEFORE — call site is unreadable: render(order, true)
function render(order: Order, isAdmin: boolean) {
  if (isAdmin) { /* admin table w/ cost columns */ }
  else { /* customer receipt */ }
}

// AFTER — two functions, each does one thing
function renderAdminOrderTable(order: Order) { /* cost columns */ }
function renderCustomerReceipt(order: Order) { /* receipt */ }
```

**B. Deep nesting -> guard clauses (rule #5)**
```ts
// BEFORE — 4 levels, the happy path is buried
function ship(order: Order) {
  if (order) {
    if (order.isPaid) {
      if (order.items.length > 0) {
        if (!order.shipped) { dispatch(order); }
      }
    }
  }
}

// AFTER — happy path at the bottom, zero nesting
function ship(order: Order) {
  if (!order?.isPaid) return;
  if (order.items.length === 0) return;
  if (order.shipped) return;
  dispatch(order);
}
```

**C. Vague names (rules #7, #8)**
```ts
// BEFORE
function proc(d: any[], t: number) {
  const r = d.filter((x) => x.v > t);
  return r.map((x) => x.n);
}

// AFTER
function namesOfReadingsAbove(readings: Reading[], thresholdCelsius: number) {
  const overheated = readings.filter((r) => r.valueCelsius > thresholdCelsius);
  return overheated.map((r) => r.sensorName);
}
```

**D. Side effect on input (rule #4)**
```ts
// BEFORE — mutates the caller's array AND its elements
function applyDiscount(items: Item[], pct: number) {
  items.sort((a, b) => a.price - b.price);          // reorders caller's data
  items.forEach((i) => { i.price *= 1 - pct; });    // corrupts caller's objects
  return items;
}

// AFTER — pure: caller's data untouched
function withDiscount(items: readonly Item[], pct: number): Item[] {
  return [...items]
    .sort((a, b) => a.price - b.price)
    .map((i) => ({ ...i, price: i.price * (1 - pct) }));
}
```

**E. God function -> one level of abstraction (rule #1)**
```ts
// BEFORE — parsing, validation, math, and IO in one 80-line body
async function handleCheckout(req: Request) {
  // ...12 lines parsing req.body...
  // ...15 lines validating stock...
  // ...20 lines computing totals, tax, shipping...
  // ...persist + email, error handling interleaved...
}

// AFTER — the function reads as its own table of contents
async function handleCheckout(req: Request) {
  const order = parseCheckoutRequest(req);
  await assertItemsInStock(order.items);
  const priced = priceOrder(order);        // totals, tax, shipping
  await persistOrder(priced);
  await sendConfirmationEmail(priced);
}
```

**F. Comment noise vs. a WHY comment (rules #17-#19)**
```ts
// BEFORE
// increment i by 1
i++;
// check if user is active   <- restates code
if (user.status === 1) { ... }
// const oldRate = 0.07;     <- commented-out code
// 2023-11-02 changed by minh <- journal comment

// AFTER — noise deleted, magic number named, one WHY comment survives
const ACTIVE = UserStatus.Active;
if (user.status === ACTIVE) { ... }
// VAT held at pre-2024 rate for contracts signed before Jan 1 (FIN-482)
const vatRate = contract.signedBefore2024 ? 0.07 : 0.085;
```

**G. Parameter list -> options object (rule #2)**
```ts
// BEFORE — call site: createUser("An", "an@x.vn", true, false, "vi", 3)
function createUser(name: string, email: string, verified: boolean,
                    admin: boolean, locale: string, retries: number) {}

// AFTER — call site reads like config; impossible to swap two booleans
interface CreateUserOptions {
  name: string; email: string;
  verified?: boolean; admin?: boolean; locale?: string; retries?: number;
}
function createUser(opts: CreateUserOptions) {}
createUser({ name: "An", email: "an@x.vn", verified: true, locale: "vi" });
```

## Output template

For any refactor bigger than one recipe-commit, fill assets/refactor-plan.md
(scope, smells cited by rule number, commit sequence, test evidence before/after)
and show it to the user before touching code.

## What NOT to do

- Do NOT mix a refactor with a feature or fix in one commit — the Law above.
- Do NOT abstract on the second duplicate (rule #20) or keep feeding flags into a shared
  helper (rule #21) — inline it instead.
- Do NOT add `Util`, `Helper`, `Manager`, or `Common` modules; they are where cohesion goes
  to die. Name the actual concept or leave the code where it is used.
- Do NOT keep commented-out code, TODO graveyards, or `.bak` copies in the tree.
- Do NOT "clean up" formatting in files you are not otherwise touching — it bloats diffs and
  masks the real change.
- Do NOT rename public/exported API in a cleanup pass without flagging it as a breaking
  change; internal names are fair game, contracts are not.