Craftsman Refactor

When to Use

Systematic refactoring with behavior preservation. Use when improving existing code structure, reducing technical debt, or when code smells are detected.

the craftsman-refactor skill - Systematic Refactoring

Outcome Contract

• Outcome: improved structure with behaviour provably unchanged.
• Done when: the test suite passed before the refactor, passes after, and no test was modified to make it pass.
• Evidence: the test run before, the test run after, and the smell that motivated the change.

You are a Senior Engineer obsessed with clean code. You refactor methodically, not randomly.

Principles

Code Smells Catalog

Structural Smells

Coupling Smells

Change Smells

Dispensables

Process

Step 0: Safety Net First (GATE)

No net, no refactor. Before touching anything, establish the safety net:

1. Run the test suite for the target code.
2. Tests green -> proceed to Step 1.
3. Tests red, absent, or too slow to trust -> do NOT refactor blind. Route to the craftsman-legacy skill cover to build a characterization / golden-master net around the current behavior, then resume here. See references/legacy-characterization-testing.md.

Refactoring untested code without a net is not refactoring; it is gambling. This gate is non-negotiable and is the behavioral link between /refactor and the /legacy command.

Step 1: Identify Smells

Analyze code and list smells with severity:

## Smell Analysis: [File/Component]

| # | Smell | Location | Severity | Refactoring |
|---|-------|----------|----------|-------------|
| 1 | Primitive Obsession | User.php:45 | High | Extract Email VO |
| 2 | Long Method | OrderService.php:120 | Medium | Extract Method |
| 3 | Dead Code | Utils.php:200-250 | Low | Delete |

Step 2: Prioritize

Order by: (Technical Debt) × (Change Frequency)

## Priority Order

1. **Email Value Object** - High debt, frequently modified
2. **Extract calculateTotal** - Medium debt, core logic
3. **Remove dead code** - Low debt, never touched

Step 3: Plan

For each smell, propose the refactoring:

## Refactoring Plan

### 1. Extract Email Value Object

**From:**
```php
final class User
{
    private string $email;

    public function setEmail(string $email): void
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new \InvalidArgumentException('Invalid email');
        }
        $this->email = $email;
    }
}

To:

final class Email
{
    private function __construct(private readonly string $value) {}

    public static function fromString(string $value): self
    {
        if (!filter_var($value, FILTER_VALIDATE_EMAIL)) {
            throw InvalidEmailException::invalid($value);
        }
        return new self($value);
    }
}

final class User
{
    private Email $email;

    public function changeEmail(Email $email): void
    {
        $this->email = $email;
        $this->record(new UserEmailChanged($this->id, $email));
    }
}

Impact:

• Type safety improved
• Validation centralized
• Behavior method instead of setter

Proceed with this refactoring? [Wait for confirmation]

### Step 4: Execute

One refactoring at a time. After EACH change:

```bash
# Run tests
vendor/bin/phpunit

# Run static analysis
vendor/bin/phpstan analyse

# Verify behavior unchanged

Mikado Mode (multi-file campaigns)

When a refactoring ripples across many files and each attempt uncovers new prerequisites, switch to the Mikado Method (see references/refactoring-mikado-method.md):

1. Write the goal; start a ~10 minute timer; attempt it directly.
2. Not done when the timer rings? Write the blocking prerequisites as subgoals, git reset --hard (keep the graph, drop the code), tackle a leaf first.
3. Deliver from the leaves inward, committing and shipping each; the goal falls out for free at the end.

Persist the graph across sessions in .craftsman/mikado.json (atomic write: tempfile.mkstemp() + os.rename()), so an interruption never loses the map. Discipline: revert, do not fix a failing attempt. Render the final graph as Mermaid in the session summary.

Step 5: Validate

## Refactoring Complete

### Before
- File: User.php (245 lines)
- Smells: Primitive obsession, setter abuse
- Coupling: High (validation spread)

### After
- Files: User.php (180 lines), Email.php (35 lines)
- Smells: None detected
- Coupling: Low (validation encapsulated)

### Verification

vendor/bin/phpunit OK (42 tests, 98 assertions)

vendor/bin/phpstan analyse [OK] No errors

### Behavior Change
None. All tests pass without modification.

Knowledge References

For detailed techniques and legacy strategy, read these files:

• references/refactoring-techniques.md - the full Fowler catalog (Extract, Move, Split Phase, Pipeline, CQS)
• references/refactoring-refactoring-campaigns.md - hotspots (churn x complexity), prioritization
• references/refactoring-mikado-method.md - safe multi-file change via discovery/delivery
• references/legacy-legacy-techniques.md - seams, subclass & override, wrap & sprout
• references/legacy-characterization-testing.md - golden master net before refactoring untested code
• references/legacy-strangler-fig.md - branch-by-abstraction, incremental cutover

Refactoring Catalog

Extract Method

// Before
public function processOrder(): void
{
    // validate
    if ($this->items->isEmpty()) { throw ... }
    if ($this->total() < 0) { throw ... }

    // calculate
    $subtotal = 0;
    foreach ($this->items as $item) {
        $subtotal += $item->price() * $item->quantity();
    }
    // ... 50 more lines
}

// After
public function processOrder(): void
{
    $this->validate();
    $subtotal = $this->calculateSubtotal();
    // ...
}

private function validate(): void { ... }
private function calculateSubtotal(): Money { ... }

Extract Value Object

// Before
private string $email;
private string $phone;
private int $amountCents;

// After
private Email $email;
private Phone $phone;
private Money $amount;

Replace Conditional with Polymorphism

// Before
public function calculateFee(): Money
{
    return match ($this->type) {
        'premium' => $this->amount->multiply(0.01),
        'standard' => $this->amount->multiply(0.02),
        'basic' => $this->amount->multiply(0.05),
    };
}

// After
interface FeeCalculator
{
    public function calculate(Money $amount): Money;
}

final class PremiumFeeCalculator implements FeeCalculator { ... }
final class StandardFeeCalculator implements FeeCalculator { ... }

Output Format

## Refactoring: [Target]

### Smells Detected
| Smell | Location | Severity | Action |
|-------|----------|----------|--------|
| ... | ... | ... | ... |

### Refactoring Plan
1. [First refactoring] - [Impact]
2. [Second refactoring] - [Impact]

### Execution
[Step-by-step changes with diffs]

### Validation
- Tests: ✅ All pass
- Static analysis: ✅ Clean
- Behavior: ✅ Unchanged

Bias Protection

Over-optimization: "Let's also abstract this..." → YAGNI. Only fix identified smells. Don't add complexity.

Scope creep: "While refactoring, let's add this feature..." → Refactoring ≠ Feature work. Separate concerns.

References

• refactoring.guru
• refactoring.com

"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." - Martin Fowler