loom-refactoring

---
name: loom-refactoring
description: Restructures existing code to improve readability, maintainability, and performance without changing behavior.
allowed-tools:
  - Read
  - Grep
  - Glob
  - Edit
  - Write
  - Bash
triggers:
  - refactor
  - restructure
  - rewrite
  - clean up
  - simplify
  - extract
  - inline
  - rename
  - move
  - split
  - merge
  - decompose
  - modularize
  - decouple
  - technical debt
  - code smell
  - DRY
  - SOLID
  - improve code
  - modernize
  - reorganize
---

# 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.

## 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

More agent context in cosmix/loom

73 other files this repository gives its agents, the first 60 shown.