--- name: smells description: Use this skill when reviewing code or specs for code smells. Covers dead code, unused bindings, duplication, magic numbers, long parameter lists, god namespaces, requiring-resolve as a cycle-breaker, sleeping in tests, exception ping-pong, and testing smells (bare assertions, table-driven examples, nested describe). --- # Code Smells ## When This Skill Applies Use this skill whenever you are reviewing code, refactoring, or evaluating spec/feature quality. Smells aren't bugs — they're signals that something deserves a closer look. ## Dead code Dead code is code with no meaningful runtime use. It should generally be removed. ### Untested dead code Untested dead code can usually be removed immediately. Snip. If nothing exercises it, there is little evidence that anyone depends on it. Keeping it around adds noise, increases maintenance cost, and makes the real execution path harder to see. ### Tested dead code Tested dead code is trickier. Sometimes it remains because it is part of a public API. Sometimes it is scaffolding for a partially built feature. In either case, the code should be accompanied by a comment explaining why it still exists and who or what depends on it. That comment is not a pardon. Even tested dead code should eventually be removed. ## Unused bindings Unused binding symbols should be prefixed with an underscore to show they are intentionally unused. Use `_foo`, not `foo`, when the value is bound but ignored. A single underscore is enough when the name doesn't matter. `(catch Exception _)` ## Duplication Duplication is repeated knowledge. A copied line is not always a problem; repeated intent usually is. Small duplication can be tolerated while code is in motion. Once the shape settles, extract the shared idea so changes happen in one place. Be careful not to extract too early. Two similar lines do not automatically justify a helper. The helper should make the code clearer, not merely shorter. For example, repeated `(BufferedReader. (StringReader. ...))` setup in a spec can justify a tiny helper like `reader-for` once the intent is clear. That keeps the setup in one place without inventing a grand abstraction. ## Magic numbers A magic number is a literal whose meaning depends on hidden domain knowledge. If a number is part of a protocol, file format, or business rule, name it once and use the named constant everywhere. This is especially important when the same literal appears in both production code and tests. That is repeated knowledge with poor affordance. For example, JSON-RPC error code `-32700` should be represented by a named constant such as `PARSE_ERROR`, not repeated inline across namespaces. ## Long parameter lists When a function takes more than three or four arguments, the interface is telling you something about missing structure. A bag of positional args is hard to read, easy to misorder, and painful to extend. An opts map is better than positional args, but a map that threads through five layers unchanged is its own smell — it suggests a context or configuration object wants to exist. ## God namespace A namespace that everything depends on and that keeps growing. Common symptoms: 500+ lines, frequent merge conflicts, new features always need to touch it. The fix is usually extract class/namespace (see the [refactor](https://github.com/slagyr/agent-lib/blob/main/skills/refactor/SKILL.md) skill). Identify cohesive responsibilities and give each one a home. ## `requiring-resolve` as a cycle-breaker `(requiring-resolve 'other.ns/fn)` (or a bare runtime `require`) used **to hide a circular dependency** is an anti-pattern. It conceals the real graph from humans and from dependency tools, delays failure until call time, and usually means a function or concern is in the wrong namespace. Fix the structure: move the function, invert the dependency, or extract a pure leaf that both sides can require. **Acceptable** (few and far between): intentional **lazy load** of an optional heavy backend; **reload/bootstrap** seams that re-resolve after source change; carefully justified test-harness isolation. Prefer a normal ns `:require` whenever the dependency is on the happy path. ## Sleeping in tests A `Thread/sleep` in a test is a smell. It means synchronization is missing. Sleeps make tests slow and flaky. Replace with blocking primitives (queues, promises, latches) or inject timing so tests can run with zero delay. If you must poll, poll at 1ms, not 10ms or 50ms. The only acceptable sleep is in a test that explicitly tests timeout behavior — and even then, keep it short. ## Exception ping-pong Exceptions bouncing between layers as control flow: throw, catch, re-wrap, throw again. The fix is a single error channel — return error values instead of throwing them. Exceptions should be reserved for genuinely unexpected failures — not for protocol-level request validation or domain-level business rule violations. ## Testing smells Tests should fail in ways that explain what went wrong. A bare `(should false)` is a smell because it throws away the reason for the failure. It signals "something bad happened" without preserving the expected behavior or the missing exception. Prefer an explicit failing assertion with a message, for example `(should-fail "Expected malformed JSON to throw ExceptionInfo")`, or better yet assert directly on the exception or result you expect. ### Table-driven examples Table-driven examples are almost always a test smell. They compress multiple behaviors into one generic harness, which makes the spec harder to read, harder to name well, and harder to debug when it fails. The loop or data table becomes the focus instead of the behavior. In practice they often duplicate implementation structure rather than clarify intent. They encourage "same assertion shape" thinking, which is tidy but usually not what the reader needs. A spec should read like a set of examples of behavior, not like a miniature test framework. Prefer a small number of explicit examples with precise names. If several examples feel repetitive, first ask whether the production API is too repetitive or whether the spec is asserting too literally on structure. Extracting helper data or tiny setup helpers can be fine; generating examples from a table usually is not. ### Nested describe Nested `describe` blocks are a test smell. A spec should usually have one top-level `describe` for the subject and then use `context` blocks to express conditions or situations. Nesting `describe` inside `describe` weakens the narrative shape of the file and makes the structure read like implementation taxonomy instead of behavior. It also tends to produce awkward example names and noisy failure output because each level competes to describe the subject again. Prefer one `describe`, then nested `context` and `it` forms that read as a sentence.