القائمة
Safe refactoring protocol for TDD — green bar rule, two-hats discipline, preparatory refactoring, and smell catalogue
التثبيت باستخدام Codex أو Claude انسخ هذا Prompt والصقه في Codex أو Claude أو مساعد آخر ليراجع صفحة Skill ويثبّتها لك.
استنادا إلى تصنيف SOC المهني
Step 2 — Architecture and domain design, one feature at a time
Step 1 — discover requirements through stakeholder interviews and write Gherkin acceptance criteria
Generate and update architecture diagrams, living glossary, and system overview from existing project docs
Enforce code quality using ruff, pytest coverage, and static type checking
Create pull requests with conventional commits, proper formatting, and branch workflow
Flow protocol — design and operate state machine workflows with FLOW.md + WORK.md
| 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 |
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.
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.
Refactoring is only permitted while all existing tests pass.
Every individual refactoring step must leave test-fast green. There are no exceptions.
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.
After GREEN: test-fast passes for the current @id. Now restructure.
When the current structure would make the next @id awkward to implement:
Beck: "For each desired change, make the change easy (warning: this may be hard), then make the easy change."
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.
| 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 |
| 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 |
| 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 |
| 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 |
| 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.
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.
uv run task test-fast
All tests green → proceed to next catalogue entry. Any test red → see "When a Refactoring Breaks a Test" below.
Once no smells remain and test-fast is green:
uv run task test-fast # must pass
Commit (see Commit Discipline below).
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.
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.
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.
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.
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.
A refactoring that breaks a test is not a refactoring. Stop. Diagnose:
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.
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.
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.
test-fast passes| 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) |
| 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 — a method should only call methods on: self, its parameters, objects it creates, and its direct components.
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.
Command-Query Separation — a method either changes state (command) or returns a value (query), never both.
| 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 |