Code Smells

Code Smells

Whole-repo design review. Third member of the analysis family:

The premise: hotspots sees graph shape, audit-deep sees runtime behavior — neither sees misplaced responsibility. A function that lives in the wrong module, a concept smeared across four files, an abstraction with one user: these compile, pass tests, and show up in no profiler. They show up as change cost. This skill finds them and names both the smell and the cure.

Every finding is a named smell mapped to a named refactoring: "Feature Envy in report.py:88 parse_totals() → Move Function to invoice.py" is a diagnosis plus a prescription. "This could be cleaner" is neither — never emit that sentence.

A definitional anchor (it shapes the whole method): a smell is "a surface indication that usually corresponds to a deeper problem" (Fowler), and per Kent Beck "a suggestion that something may be wrong, not evidence that there is already a problem." Smells are heuristics, not rules — the field's own literature warns that mechanical detectors drown in false positives because "code metric values, when inspected out of context, mean nothing." That is why every mechanical lead below must be confirmed by reading, and why the catalog's "not a smell when" clauses carry as much weight as the definitions.

When to run

• /smells [path] [--fix]
• "review this repo for code smells" / "design review" / "why is this codebase hard to change" / "what refactorings does this need"
• After architectural-hotspots flagged files and the user wants to know what kind of wrong they are, not just that they are big.

Do not use when the user wants:

• Bug / perf / durability findings in a named file → code-audit-deep.
• File rankings, dependency structure, cycles → architectural-hotspots.
• A diff/PR review → code-review / simplify.
• Style and lint issues → the project's linter already owns those.

A scoped review still has to name where the cure lives. When the user points at four files, those four bound where you look, not where the fix lands — and for the smells that matter most (Feature Envy, Shotgun Surgery, Data Clumps) the destination is somewhere else by definition, because the whole finding is that behaviour sits in the wrong place. Follow the fix out of scope far enough to name its destination concretely, then stop. "Move parse_totals into invoice.py" is the deliverable; "move this somewhere better" is not a prescription. Say plainly that the target is outside the reviewed set so the user knows the blast radius before approving.

Findings that belong to a sibling get one line under "Hand-off" in the report, never a full section here. Raw file size is hotspots' god-module table; a slow loop is audit-deep perf; an ignored error is audit-deep correctness. This skill takes over when the finding is about the shape of responsibility — who owns which data, which module knows too much about which, what is forced to change together.

Method

Scope is the whole repo, but attention is not uniform — depth where change happens, breadth everywhere else.

0. Read the project's record of rejected findings first. Look for DECISIONS.md, ADRs, a "rejection ledger", a "verified negatives" section, *findings*.md; ask the user if the repo looks mature and nothing turns up. A team that wrote down why it declined a change has already paid for that analysis, and re-raising the item tells them you did not read it — which discredits the findings that are new. When you drop a candidate on this basis, name the entry, so the user can see their record being used rather than assume you found nothing there.

   This is not the same as the report's own consistency. Keep a short ledger of what you rejected this session and why, so a later pass does not resurface it and so the "Not found" reasoning stays stable across the report.

1. Recon (cheap): git ls-files, language mix, rough size, test presence. Then churn to find where the team actually works:

   git log --since="12 months ago" --format= --name-only -- <path> \
     | sort | uniq -c | sort -rn | head -30
   

   Smells in high-churn code are paid for on every change; smells in stable code are mostly rent-free. Churn ranks everything downstream. One empirical correction to folk wisdom (Tufano et al., ICSE 2015): smells are usually born with the file, not accumulated — most instances manifest in the very commit that creates the artifact, introduced during feature work near deadlines. So churn is the payoff ranking, not a suspicion ranking: young, recently-added files under active development deserve the same scrutiny as old veterans, and "this file is new, it must be clean" is exactly backwards.

2. Candidate sweep (cheap, whole repo): run the grep heuristics from references/smell-catalog.md — message chains, boolean-flag parameters, long parameter lists, repeated type-switch discriminators, single-implementation interfaces, duplicated fragments. Each hit is a lead, never a finding. Collect leads per file.

3. Pick the deep-read set: files ranked by churn × lead density, plus anything the user named, plus entry points. Usually 10–20 files. Read those in full — smells like Feature Envy, Divergent Change, and Temporal Coupling are invisible to grep and only appear when you read function bodies and ask "does this code belong here?".

4. Confirm every lead by reading. A grep hit becomes a finding only when you have read the site plus enough of its neighbors to tell the cost story: what concrete change gets more expensive because of this? If you cannot name the change that hurts, drop the lead — a smell with no cost story is dogma, not a finding.

5. Cross-check change-coupling for the change-preventer smells. Shotgun Surgery and Divergent Change are historical claims, so back them with history when git is available:

   git log --since="12 months ago" --format='%h' --name-only
   

   Files that repeatedly appear in the same commits, or one file whose commits alternate between unrelated concerns, are the evidence. (This is the literature's "history-based detection" — the only detector class that can see these two smells at all; no amount of reading a single snapshot proves them.)

   File-level co-change is blind when one file owns a whole vocabulary. If a concept lives inside a single file — a command table, an enum and its handlers, a registry — adding a member touches few files and looks tame, while the real cost is the number of edit sites inside one file. Trace the concept, not the file:

   git log -S'<constant or variant name>' --oneline -- <path>
   

   Adding one command word showed three files changed, which reads as fine; the same concept had five separate edit sites in one file. That is Shotgun Surgery with the blast radius hidden inside a file boundary, and --name-only cannot see it.

6. Report in the fixed format below. If --fix was passed, continue into Apply mode after the user approves.

The catalog in references/smell-catalog.md is the authority for what each smell is, its detection heuristics, its refactoring mapping, and — critically — its "not a smell when" exceptions. Read it before the sweep; do not detect from memory, because half the discipline is in the exceptions.

Report format

Group by smell. Use this exact shape; omit smells with zero findings.

**<Smell Name> (N)**
- `file:line` `symbol` — evidence in one line (counts, sites, churn).
  → <Refactoring Name>: one-line sketch. Effort: S/M/L.

**Hand-off**
- one line per out-of-scope observation → owning skill.

**Top refactorings**
1–5 highest-leverage items: smell, site, refactoring, why first.

Example of a good finding:

Feature Envy (2)

• orders/report.py:88 parse_totals() — 11 reads of Invoice internals vs 1 of its own module; invoice churned 14× this year. → Move Function into invoice.py. Effort: S.

Example of a bad finding (never emit):

report.py has some coupling issues and could be more object-oriented. Consider refactoring.

Rules that keep the report worth reading:

• Evidence is counts and sites, not adjectives. "9 accesses vs 1", "same 4 params in 6 signatures", "12 files touched by each of the last 3 renames" — numbers the user can verify.
• Effort is honest: S = one function moves, M = one module reshapes, L = cross-cutting (name the blast radius).
• Top refactorings are ordered by leverage — cost removed per effort spent, with churn as the multiplier. A Medium refactor on a file the team touches weekly beats a Small one on frozen code.

Apply mode (--fix)

--fix never means "silently rewrite the repo". The flow is report → approve → apply:

1. Produce the full report exactly as above, findings numbered.
2. Ask the user which findings to apply — all, a subset, or top-N. Do not start editing before this answer.
3. Order the approved work safest-first: Dispensables (dead code, inline lazy module, collapse speculative generality) → local reshapes (Extract Function, Introduce Parameter Object) → moves across modules (Move Function, Extract Class) → anything touching public API last.
4. Each refactoring is behavior-preserving and separately committed: apply one finding, run the test suite, commit with the smell and refactoring named in the message, then take the next. A red test stops the line — fix or revert that step before proceeding, never pile a second refactor on top of a broken state.
5. No test suite? Say so before touching anything, and offer the choice: restrict to statically-safe refactors (dead-code deletion, mechanical moves the compiler/imports verify), or write characterization tests around the target first. Never do behavior-risky reshapes on an untested codebase without the user opting in.

Refactorings change structure, not behavior. If mid-apply you discover an actual bug, do not fix it in the same commit — report it (that finding belongs to code-audit-deep anyway) and keep the refactor pure.

After the last approved fix, re-scan the touched areas before declaring done. This is not paranoia: Tufano et al. found hundreds of cases where the refactoring commit itself introduced a new smell (the classic: Extract Class leaving behind a one-method husk — instant Lazy Element, or a mover that turns the destination into a Large Class). The re-scan is cheap; shipping a fix that traded one smell for another is not.

Calibration

A meaningful repo review usually lands 8–20 confirmed findings across 4–8 smell types. Fewer than ~5 on a mature codebase means the sweep was shallow (or the codebase is genuinely disciplined — a real outcome; say so). More than ~25 means the bar dropped: keep the ones with the strongest cost stories and cut the rest.

Every finding must survive two tests:

• The cost test — you can name the concrete change this smell makes expensive. "Adding a currency means editing 6 switch sites" passes. "Not idiomatic" fails.
• The action test — the mapped refactoring is something the user could start today. If the honest prescription is "rewrite the module", the finding belongs in hotspots' territory, not here.

Anti-patterns

• Dogma detection. Flagging every switch, every primitive, every long function because the catalog lists them. The catalog's "not a smell when" clauses exist because most instances are fine. A parser's dispatch switch, a DTO's data clump, a hot path's duplication may all be correct engineering — the finding is the cost, not the pattern.
• Taking a justifying comment at face value. Many "not a smell when" exceptions turn on a claim the code makes about itself — "this duplication is deliberate, the two will diverge", "the clone is required here", "these would only agree by accident". That is a hypothesis, and dropping a real finding because prose asserted it was fine leaves no trace in the report for anyone to catch. Where the claim is checkable, check it before granting the exception.
• Grep-only findings. Reporting leads without reading them. The false-positive rate of raw heuristics is high by design; reading is the filter.
• Sibling poaching. Perf, correctness, durability findings dressed up in smell vocabulary. An N+1 query is not "Insider Trading"; it is audit-deep material. Hand it off.
• Prescription without diagnosis or diagnosis without prescription. Every finding carries both a smell name and a refactoring name. A smell you cannot map to a refactoring is not finished thinking.
• Uniform attention. Spending equal effort on frozen code and weekly-churn code. Churn is the multiplier on every payoff; let it steer.
• Fix-mode scope creep. "While I'm here" improvements bundled into a refactor commit. One finding, one commit, behavior identical.