---
name: clean-code
description: "Readability checks for code in any language: names, function focus, control flow, side effects, errors, comments, duplication. Use when writing, refactoring or reviewing code for quality and naming."
metadata:
  risk: safe
  source: multi-agent-pipeline
  date_added: "2026-09-24"
---

# Clean Code

Code is read far more often than it is written, and every later change starts
with someone working out what the current code does. This skill is a set of
questions to ask of a piece of code, in the order that finds the expensive
problems first, plus the move that fixes each one.

The repository's own conventions win. A linter rule, a module guide or a coding
standard registry that says otherwise overrides anything here; this skill fills
the gaps they leave.

## Order of questions

Work from the outside in. A clear name on a muddled body is cheaper to fix than
a tidy body behind a misleading name, because callers only ever see the name.

1. Does each name tell a caller what they get, without opening the body?
2. Does each unit do one job at one level of detail?
3. Can the control flow be followed top to bottom without holding state in mind?
4. Is every change to state visible at the call site?
5. Does every failure reach someone who can act on it?
6. Does each comment say something the code cannot?
7. Is each rule written down in exactly one place?
8. Would a test fail if the behaviour broke?

## 1. Names

- Name by role in this domain, not by type or storage: `overdueInvoices`, not
  `invoiceList` or `data2`.
- A boolean reads as a claim: `isExpired`, `hasShippingAddress`, `canRetry`.
- A function that returns something is named for what it returns
  (`remainingBalance()`); one that acts is named for the action (`cancelOrder()`).
- Units belong in the name when the type does not carry them: `timeoutSeconds`,
  `distanceKm`.
- One word per concept across the codebase. If `fetch`, `load` and `get` all mean
  "read from the network", pick one.
- Length follows scope: `i` is fine in a three-line loop; a module-level value
  needs a full phrase.

```python
# Before
def calc(d, f):
    return d * (1 - f)

# After
def discounted_price(list_price: Decimal, discount_rate: Decimal) -> Decimal:
    return list_price * (1 - discount_rate)
```

## 2. Units of work

- One job per function. The test: describe it in one sentence without "and" or
  "then". If you cannot, split at the conjunction.
- One level of detail per function. A function that validates an order should
  not also format a currency string; call a function that does.
- Few parameters. Three or more, or any boolean flag that switches behaviour,
  usually means a missing type or two functions sharing a name.
- A body longer than a screen is a prompt to look for the seams, not a rule.

```swift
// Before: the flag hides two behaviours
func export(_ report: Report, asPDF: Bool) -> Data

// After: two names, no flag
func exportPDF(_ report: Report) -> Data
func exportCSV(_ report: Report) -> Data
```

## 3. Control flow

- Handle the exits first. Guard clauses at the top keep the main path at one
  indentation level.
- Replace a chain of `if`/`else if` on the same value with a `switch`/`match`
  that the compiler can check for completeness.
- Name a compound condition: `let isEligible = age >= 18 && hasConsent` reads
  better than the raw expression inside an `if`.
- Nesting past two levels almost always hides a function waiting to be extracted.

```typescript
// Before
function shippingCost(order: Order): number {
  if (order.items.length > 0) {
    if (!order.isDigital) {
      return order.weightKg * RATE_PER_KG;
    }
  }
  return 0;
}

// After
function shippingCost(order: Order): number {
  if (order.items.length === 0 || order.isDigital) return 0;
  return order.weightKg * RATE_PER_KG;
}
```

## 4. State and side effects

- A function either answers a question or changes something. When one does both,
  a caller who only wanted the answer triggers the change.
- Pass what a function needs; do not let it reach into globals, singletons or
  the clock. Inject the dependency so a test can replace it.
- Prefer values that cannot change once built. Mutation belongs in a small,
  named place.
- Keep a variable's scope as small as its use.

## 5. Errors and edge values

- Fail where the problem is detected, with a message that names the input and
  the expectation.
- Do not swallow an error to keep going. Catch it only where something useful
  can be done: retry, fall back, or report to the user.
- Represent "no value" in the type (`Optional`, `Result`, a sealed type) instead
  of returning a sentinel such as `-1`, an empty string or `null` by convention.
- Check the boundaries every time: empty input, one element, the maximum, and
  values that are present but invalid.

```kotlin
// Before: the caller cannot tell "not found" from "zero"
fun itemCount(orderId: String): Int = repository.find(orderId)?.items ?: 0

// After
fun itemCount(orderId: String): Int? = repository.find(orderId)?.items
```

## 6. Comments

- Write down why, never what. The code already says what it does; a comment that
  repeats it goes stale the first time the code changes.
- A comment that explains a confusing block is a request to rename or extract
  until the comment is unnecessary.
- Keep: the reason behind a non-obvious choice, a link to the spec or bug a
  workaround exists for, the legal header a file must carry, and public API docs.
- Delete: commented-out code (version control keeps it), change logs inside the
  file, and section banners that restate the structure.

## 7. Duplication and abstraction

- A business rule written in two places will eventually disagree with itself.
  Give it one home and call it.
- Two blocks that look alike but change for different reasons are not
  duplication; merging them couples unrelated features.
- Wait for the third occurrence before extracting a general helper, unless the
  repeated thing is a rule rather than a shape.
- An abstraction earns its place when it removes decisions from callers. One that
  only forwards arguments adds a hop and a name to learn.

## 8. Tests as the specification

- Each test states one behaviour in its name: `refundIsRejectedAfterShipment`.
- Arrange, act, assert, with the act step a single call.
- A test that cannot fail when the behaviour breaks is worse than none, because
  it reports coverage that does not exist.
- Fast, isolated and repeatable: no network, no shared state between tests, no
  dependence on the clock or execution order.

## Problem to move

| Problem you see | Move |
|---|---|
| A name needs a comment to explain it | Rename |
| A block has a comment heading it | Extract a function named after the comment |
| A boolean parameter | Split into two functions, or pass an enum |
| Three or more related parameters | Introduce a parameter type |
| Deep nesting | Guard clauses, then extract the inner block |
| A `switch` on type repeated in several places | Move the behaviour onto the types |
| A literal with meaning (`86400`, `0.2`) | Named constant or configuration |
| The same rule in two places | Extract once, call from both |
| A function that reads and writes | Separate the query from the command |
| A hard-coded dependency | Inject it |

Refactor in small steps with the tests green between each, and keep a
behaviour change out of a refactoring commit.

## Reporting in a review

For each finding give the location, what the problem is, why it costs the next
reader or maintainer, and the concrete change. Rank by cost: a misleading name
or a swallowed error outranks a long function, which outranks a style nit.
Leave formatting to the formatter and say nothing about it.

## Out of scope

- Security weaknesses: use `security-review`.
- Speed and memory: profile first, then use the stack's performance skill.
- Module boundaries and system design: use `architecture`.
- Language style rules a linter enforces: follow the linter.