loom-refactoring

# Refactoring ## Overview Change the internal structure of code without changing its observable behavior. Restructuring and behavior change are two different activities — never do both in one commit. ## The cardinal rule **Behavior-preserving, tests green before AND after.** If you can't prove behavior is unchanged, you're not refactoring — you're rewriting, and you need different discipline (characterization tests, feature flags; see `/loom-code-migration`). Corollaries an expert never violates: 1. **Tests are the safety net.** Run them before you touch anything (establish green). If there are none for the code you're changing, write **characterization tests** first (below) — do not refactor untested legacy blind. 2. **Tiny reversible steps.** One mechanical move at a time; re-run tests after each. When something breaks, inspect the small current diff and use the repository's approved recovery workflow rather than discarding unrelated work. 3. **Separate refactor commits from behavior-change commits.** A `refactor:` commit must be a no-op at runtime — reviewers can trust the diff without re-verifying logic. Mixing a bug fix into a rename hides the fix and poisons `git bisect`. (Commit mechanics: `/loom-git-workflow`.) 4. **Don't change tests and production code in the same step.** If a refactor "requires" editing a test's assertions, behavior changed — stop and reconsider. ## Not this skill - Fixing a bug → `/loom-debugging` (that *is* a behavior change; write the failing test first). - Adding a feature → implement it, then refactor separately. - Migrating framework/API/schema → `/loom-code-migration` (behavior *does* move; needs parallel-run/rollback). ## Name the smell, then apply the matching move Refactoring is not "make it nicer" — identify a specific smell and apply its canonical move. If you can't name the smell, don't refactor. | Smell | Signal | Move | | ------------------------ | -------------------------------------------------- | ---------------------------------------------------------- | | Long method (>50 LOC) | Scrolling to read one function; comment-delimited sections | Extract Method; Decompose Conditional | | Large class (>300 LOC) | Many unrelated fields/methods; low cohesion | Extract Class; Move Method | | Duplicated logic | Same block in 2+ places (will drift) | Extract Method/Function, then hoist to shared location | | Feature envy | Method uses another object's data more than its own | Move Method to the class that owns the data | | Primitive obsession | Bare strings/ints for domain concepts; validation scattered | Introduce Value Object / newtype | | Data clumps | Same 3-4 params travel together everywhere | Introduce Parameter Object | | Shotgun surgery | One conceptual change forces edits in many files | Consolidate the responsibility into one module | | Divergent change | One module changed for many unrelated reasons | Split by axis of change (SRP) | | Deep nesting (>3) | Arrow-shaped code | Guard clauses; Extract Method | | Type-code switch | `switch`/`match` on a type tag, repeated | Replace Conditional with Polymorphism | | Magic number/string | Unexplained literal | Named constant | | Speculative generality | Abstraction/param with one caller "for later" | Inline it — YAGNI | **Inverse move matters too:** over-abstraction is a smell. Inline needless indirection; a single-implementation interface or a one-caller helper usually earns removal, not preservation. ## Workflow ```text 1. Green baseline → run the full suite; if red, stop (fix or characterize first) 2. Name the smell → pick ONE move from the table 3. Apply one step → smallest mechanical edit; prefer IDE/tool rename over hand-edit 4. Re-run tests → green? commit. red? inspect the current small diff and rethink 5. Repeat → next step 6. Final gate → full suite + lint + build green; diff is behavior-neutral ``` Prefer tool-driven moves when available: LSP/IDE "rename symbol" and "extract function" are behavior-preserving by construction and update all references — safer than `rg`+Edit. Use `rg` first to size the blast radius of any rename. Loom stage sessions deny the LSP tool, so in a stage use `rg` to find every reference and Edit each one. ## Characterization tests (before touching legacy) When code has no tests, capture *what it currently does* (bugs and all) before changing structure. You are pinning behavior, not asserting correctness. ```python # Feed representative + edge inputs, snapshot whatever comes out. # The point is a tripwire: if a refactor changes output, the test breaks. @pytest.mark.parametrize("inp", [normal, empty, boundary, weird_unicode]) def test_characterize_legacy(inp): assert legacy_process(inp) == SNAPSHOT[inp] # golden-master ``` For wide surfaces, golden-master / approval testing (snapshot the output of many real inputs) beats hand-writing assertions. If current behavior is genuinely undefined (nondeterministic, time-dependent), pin the invariants you *can* (length, sorted-ness, schema) rather than exact values. ## Canonical moves (representative examples) Two moves cover most day-to-day work; the rest are in the table above. ### Extract Method — the workhorse ```python # Before: comment-delimited sections = extraction seams def process_order(order): if not order.items: raise ValueError("Empty order") # validate discount = order.total * 0.1 if order.customer.is_premium else 0 # discount order.final_total = order.total - discount; order.save() # finalize # After: each section named, independently testable def process_order(order): validate_order(order) order.final_total = order.total - calculate_discount(order) order.save() ``` ### Guard clauses — kill nesting ```python # Before: arrow code # After: flat, early-return def pay(e): def pay(e): if e.active: if not e.active: return 0 if e.full_time: if not e.full_time: return e.salary return e.hourly_rate * e.hours else: return e.salary return e.hourly_rate * e.hours return 0 ``` Replace-conditional-with-polymorphism, introduce-parameter-object, and extract-class follow the same discipline: one behavior-neutral step, tests green between each. ## Gotchas - ⚠ **Reflection / string-keyed dispatch / serialized names.** IDE rename misses symbols referenced by string (DI containers, ORM column names, JSON keys, dynamic `getattr`). `rg` the *string* form too. - ⚠ **Public API is not yours to "clean."** Renaming an exported symbol with external consumers is a migration, not a refactor — deprecate + shim (`/loom-code-migration`). - ⚠ **Refactoring hot paths.** "Extract method" can add call overhead / defeat inlining in tight loops. Measure, don't guess — profile before and after (`/loom-performance-testing`). - ⚠ **Formatter noise.** A reformat mixed into a logic-bearing diff makes review impossible and causes merge conflicts with sibling loom stages. Keep pure-format commits separate; make surgical edits. - ⚠ **"While I'm here" scope creep.** Each changed line must trace to the named smell. Unrelated improvements → note them (`concerns.md` / `loom memory note`), don't do them now. - ⚠ **Behavior leaks disguised as cleanup:** changing a default, tightening a type, reordering side-effecting calls, or "simplifying" a short-circuit are behavior changes. If output could differ for *any* input, it's not a refactor. ## When to stop Commit progress and escalate (to `loom-senior-software-engineer` or a specialist) if: tests fail in a way you don't understand, scope is ballooning past the named smell, the change touches a public API or a perf-critical path, or architectural implications are unclear. A clean partial refactor beats a broken big-bang one. ## Verify before done - [ ] Full suite green BEFORE first change (or characterization tests written) - [ ] Each step was one behavior-neutral move; tests green between steps - [ ] No production + test logic changed in the same step - [ ] Refactor commits contain no behavior change; any fix/feature is a separate commit - [ ] String/reflection/serialized references caught (not just symbol references) - [ ] Final: suite + lint + build green; diff traces entirely to the named smell(s) - [ ] No formatter-only churn mixed into logic commits