| name | refactor |
| description | Safe refactoring protocol for TDD — green bar rule, two-hats discipline, preparatory refactoring, and smell catalogue |
| version | 2.0 |
| author | software-engineer |
| audience | software-engineer |
| workflow | feature-lifecycle |
Refactor
Load this skill when entering the REFACTOR phase of a TDD cycle, or before starting RED on a new @id when preparatory refactoring is needed.
Sources: Fowler Refactoring 2nd ed. (2018); Beck Canon TDD (2023); Beck Tidy First? (2023); Martin SOLID (2000); Bay Object Calisthenics (2005); Shvets Refactoring.Guru (2014–present). See docs/research/oop-design.md entries 33–36 and docs/research/refactoring-empirical.md.
The Definition
A refactoring is a behavior-preserving transformation of internal structure. If the transformation changes observable behavior, it is not a refactoring — it is a feature change, and requires its own RED-GREEN-REFACTOR cycle.
The Green Bar Rule (absolute)
Refactoring is only permitted while all existing tests pass.
Every individual refactoring step must leave test-fast green. There are no exceptions.
The Two-Hats Rule
Wear one hat at a time:
| Hat | Activity | Allowed during this hat |
|---|
| Feature hat | RED → GREEN | Write failing test, write minimum code to pass |
| Refactoring hat | REFACTOR | Restructure passing code; never add new behavior |
Never mix hats in the same step. If you discover a refactoring is needed while making a test pass (GREEN), note it — finish GREEN first, then switch hats.
When to Use
1. REFACTOR phase (opportunistic)
After GREEN: test-fast passes for the current @id. Now restructure.
2. Preparatory refactoring (before RED)
When the current structure would make the next @id awkward to implement:
- Put on the refactoring hat first
- Refactor until the feature is easy to add
- Commit the preparatory refactoring separately (see Commit Discipline)
- Then put on the feature hat and run RED-GREEN-REFACTOR normally
Beck: "For each desired change, make the change easy (warning: this may be hard), then make the easy change."
Step-by-Step
Step 1 — Identify the smell
Run the smell checklist from your Self-Declaration or from the Architecture Smell Check.
Smell categories from Shvets Refactoring.Guru (2014–present); each smell links to its Fowler catalogue entry.
Bloaters — structures grown too large
| Smell | Signal | Likely catalogue entry |
|---|
| Long Method | Method body needs a comment to understand any section | Extract Function, Decompose Conditional |
| Large Class | Class has too many responsibilities or instance variables | Extract Class, Extract Subclass |
| Primitive Obsession | Domain concept represented as a raw primitive | Replace Primitive with Object, Introduce Parameter Object |
| Long Parameter List | Function takes 3+ parameters, or parameter group repeats across signatures | Introduce Parameter Object, Replace Parameter with Query |
| Data Clumps | Same 2–3 data items always appear together across signatures or fields | Introduce Parameter Object, Extract Class |
OO Abusers — misapplied OOP
| Smell | Signal | Likely catalogue entry |
|---|
| Switch Statements | Repeated if/elif or match on a type flag across callers | Replace Conditional with Polymorphism, Strategy, State |
| Temporary Field | Instance variable set only in some code paths; None in others | Extract Class, Introduce Null Object |
| Refused Bequest | Subclass inherits methods/data it does not use or overrides to do nothing | Push Down Method/Field, Replace Inheritance with Delegation |
| Alternative Classes with Different Interfaces | Two classes do the same thing under different names/signatures | Rename Method, Extract Superclass, unify via Protocol |
Change Preventers — changes ripple unexpectedly
| Smell | Signal | Likely catalogue entry |
|---|
| Divergent Change | One class must change for multiple unrelated reasons | Extract Class (split by axis of change) |
| Shotgun Surgery | One concept change touches many classes | Move Function/Field, Inline Class, combine scattered behavior |
| Parallel Inheritance Hierarchies | Adding a subclass to one hierarchy forces a new subclass in another | Move Function/Field to flatten or unify hierarchies |
Dispensables — dead weight
| Smell | Signal | Likely catalogue entry |
|---|
| Comments | Comment explains what or why when the code could be self-explanatory | Extract Function, Rename Variable/Function |
| Duplicate Code | Same logic copied in 2+ places | Extract Function, Pull Up Method, Form Template Method |
| Lazy Class | Class does too little to justify its existence | Inline Class, Collapse Hierarchy |
| Data Class | Class holds only fields with getters/setters; no behavior | Move Function into class, Encapsulate Field |
| Dead Code | Unreachable code, unused variable, never-called function | Delete it |
| Speculative Generality | Abstractions added "for future use" with no current caller | Inline Class/Function, Remove unused parameters |
Couplers — excessive inter-object dependency
| Smell | Signal | Likely catalogue entry |
|---|
| Feature Envy | Method uses another class's data more than its own | Move Function, Extract Function |
| Inappropriate Intimacy | Class accesses another's private fields or implementation details | Move Function/Field, Extract Class, Replace Inheritance with Delegation |
| Message Chains | a.b().c().d() — navigating a chain of objects | Hide Delegate, Extract Function to encapsulate the chain |
| Middle Man | Class delegates most of its methods to another class | Inline Class, Remove Middle Man |
| Incomplete Library Class | External class lacks a needed method | Introduce Foreign Method, Introduce Extension Object |
If pattern smell detected: load skill apply-patterns for pattern selection guidance.
Step 2 — Apply one catalogue entry at a time
Apply a single catalogue entry, then run test-fast before moving to the next.
Never batch multiple catalogue entries into one step — you lose the ability to pinpoint which step broke something.
Step 3 — Run after each step
uv run task test-fast
All tests green → proceed to next catalogue entry.
Any test red → see "When a Refactoring Breaks a Test" below.
Step 4 — Commit when smell-free
Once no smells remain and test-fast is green:
uv run task test-fast
Commit (see Commit Discipline below).
Key Catalogue Entries
Extract Function
Pull a cohesive fragment into a named function.
Trigger: a fragment needs a comment to explain what it does.
Outcome: the extracted function's name makes the comment unnecessary; the caller reads as a sequence of named steps.
Extract Class
Split a class that is doing two jobs.
Trigger: a data cluster (2–3 fields that always travel together) with related behaviour that could be named independently.
Outcome: each class has one reason to change; the new class becomes a value object or a collaborator.
Introduce Parameter Object
Replace a recurring parameter group with a dedicated object.
Trigger: the same 2+ parameters appear together across multiple function signatures.
Outcome: a named type captures the concept; callers are simplified; the object can later carry behaviour.
Replace Primitive with Object
Elevate a domain concept represented as a raw primitive to its own type.
Trigger: a primitive has validation rules, formatting logic, or operations that are repeated at every call site.
Outcome: behaviour moves into the type; callers are protected from invalid states; the type can be named and tested independently.
Decompose Conditional / Guard Clauses
Flatten nested conditional logic to ≤2 levels.
Trigger: OC-1 violation (nesting beyond one indent level per method), or multi-level nested if chains.
Outcome: each exit condition is expressed as an early return (guard clause); the happy path is at the left margin; no else after return.
When a Refactoring Breaks a Test
A refactoring that breaks a test is not a refactoring. Stop. Diagnose:
Diagnosis flow
Test fails after a structural change
│
▼
Is the test testing internal structure
(private methods, specific call chains,
concrete types) rather than observable behavior?
│
YES │ NO
│ └──→ The "refactoring" changed observable behavior.
│ This is a FEATURE CHANGE.
│ Revert the step.
│ Put on the feature hat.
│ Run RED-GREEN-REFACTOR for it explicitly.
▼
Rewrite the test to use the public interface.
Re-apply the refactoring step.
Run test-fast — must be green.
Never delete a failing test without diagnosing it first.
Commit Discipline
Refactoring commits are always separate from feature commits.
| Commit type | Message format | When |
|---|
| Preparatory refactoring | refactor(<feature-stem>): <what> | Before RED, to make the feature easier |
| REFACTOR phase | refactor(<feature-stem>): <what> | After GREEN, cleaning up the green code |
| Feature addition | feat(<feature-stem>): <what> | After GREEN (never mixed with refactor) |
Never mix a structural cleanup with a behavior addition in one commit. This keeps history bisectable and CI green at every commit.
Self-Declaration Check (before exiting REFACTOR)
Before marking the @id complete, verify all of the following. Each failed item is a smell — apply the catalogue entry, run test-fast, then re-check.
Green Bar
Object Calisthenics (Bay 2005)
| Rule | Constraint | Violation signal |
|---|
| OC-1 | One indent level per method | for inside if inside a method body |
| OC-2 | No else after return | if cond: return x then else: return y |
| OC-3 | Wrap primitives with domain meaning | def process(user_id: int) instead of UserId |
| OC-4 | Wrap collections with domain meaning | list[Order] passed around instead of OrderCollection |
| OC-5 | One dot per line | obj.repo.find(id).name |
| OC-6 | No abbreviations | usr, mgr, cfg, val, tmp |
| OC-7 | Classes ≤ 50 lines, methods ≤ 20 lines | Any method requiring scrolling |
| OC-8 | ≤ 2 instance variables per class (behavioural classes only; dataclasses, Pydantic models, value objects, and TypedDicts are exempt) | __init__ with 3+ self.x = assignments in a behavioural class |
| OC-9 | No getters/setters | def get_name(self) / def set_name(self, v) |
SOLID (Martin 2000)
| Principle | Check | Violation signal |
|---|
| S — Single Responsibility | Does this class have exactly one reason to change? | Class handles data + formatting, or business logic + persistence |
| O — Open/Closed | Can new behavior be added without editing this class? | Adding a case requires editing an if/elif chain inside the class |
| L — Liskov Substitution | Do all subtypes honor the full contract of their base type? | Subclass raises on an inherited method, or narrows a precondition |
| I — Interface Segregation | Does every implementor use every method in the interface? | Implementors stub out methods they don't need |
| D — Dependency Inversion | Does domain code depend only on abstractions, not concrete I/O? | Domain class directly imports a database, file, or network class |
Law of Demeter / Tell, Don't Ask / CQS
Law of Demeter — a method should only call methods on: self, its parameters, objects it creates, and its direct components.
- Violation signal: chaining through two or more intermediaries (
a.b().c()). Ask a to do the thing instead of navigating through it.
Tell, Don't Ask — tell objects what to do; don't query their state and decide externally.
- Violation signal: querying an object's status field, then setting it based on that query from outside the object. Move the decision into the object itself.
Command-Query Separation — a method either changes state (command) or returns a value (query), never both.
- Apply to domain objects. Standard library collections are a known exception (e.g., pop-style methods).
Design Clarity Signals
| Principle | Signal |
|---|
| Explicit over implicit | Dependencies stated at construction; no hidden side effects or magic initialization |
| Simple over complex | One function, one job; prefer a plain function over a class when no state is needed |
| Flat over nested | OC-1 — one indent level per method; early returns over deep nesting |
| Readability | OC-6 — no abbreviations; public items documented |
| Errors surface explicitly | Raise on invalid input; never silently swallow errors or return a default that hides failure |
| No ambiguous defaults | Invalid input raises; callers are never surprised by silent fallbacks |
Type and documentation hygiene
