aiskillstore/refactoring

Refactoring

Improve code structure without changing behavior. One small step at a time.

Iron Law: ONE REFACTORING → TEST → COMMIT. Never batch changes.

When to Use

Answer IN ORDER. Stop at first match:

1. User says "refactor", "clean up", "restructure"? → Use this skill
2. User asks to "extract", "rename", "simplify"? → Use this skill
3. Code smell identified? → Use this skill
4. User wants to add feature or fix bug? → Skip (use tdd-enforcer)
5. User wants formatting/style fixes? → Skip (use /lint)

Code smells (common triggers):

• Duplicated code (same logic in multiple places)
• Long function (>30 lines, doing too much)
• Magic numbers/strings (unexplained literals)
• Deep nesting (>3 levels of indentation)
• Dead code (unused functions, unreachable branches)
• Poor naming (unclear what something does)

---

Phase 1: ASSESS

Is this actually refactoring?

| User Intent | Action | | ------------------- | ----------------------------------------------- | | "Make this cleaner" | ✓ Refactoring | | "Add validation" | ✗ New behavior → tdd-enforcer | | "Fix this bug" | ✗ Bug fix → tdd-enforcer or systematic-debugger | | "Format this code" | ✗ Style → /lint |

If not refactoring: Explain and suggest correct approach.

---

Phase 2: PROTECT

Does the code have tests?

| Coverage | Action | | ---------------- | --------------------------------------------- | | Well-tested | Skip to Phase 3 | | Partial coverage | Add characterization tests for untested parts | | No tests | Add characterization tests first |

Characterization Tests

Capture current behavior before refactoring:

// Characterization test - captures ACTUAL behavior
it('processOrder returns current behavior', () => {
  const result = processOrder({ items: [], user: null });
  // Whatever it returns NOW is the expected value
  expect(result).toEqual({ status: 'empty', total: 0 });
});

Purpose: Safety net, not specification. Test what the code DOES, not what it SHOULD do.

---

Phase 3: REFACTOR

Iron Law: ONE refactoring at a time. Run tests after EVERY change.

Refactoring Catalog

Tier 1 - Always Safe (no behavior change possible):

| Smell | Refactoring | Example | | -------------------- | -------------------- | -------------------------------------- | | Unclear name | Rename | d → discountAmount | | Long function | Extract Function | Pull 10 lines into calculateTax() | | Unnecessary variable | Inline Variable | Remove temp = x; return temp; | | Misplaced code | Move Function | Move validate() to Validator class |

// ❌ Before: unclear name
const d = price * 0.2;

// ✅ After: Rename
const discountAmount = price * 0.2;

Tier 2 - Safe with Tests (low risk if tests exist):

| Smell | Refactoring | Example | | ------------------- | ------------------------- | ------------------------------------------------- | | Repeated expression | Extract Variable | order.items.length > 0 → const hasItems = ... | | Complex conditional | Decompose Conditional | Extract if branches to named functions | | Nested conditionals | Guard Clauses | Early returns instead of deep nesting | | Magic literal | Replace Magic Literal | 0.2 → VIP_DISCOUNT_RATE | | Unused code | Remove Dead Code | Delete unreachable branches |

// ❌ Before: nested conditionals
function getDiscount(user) {
  if (user) {
    if (user.isVIP) {
      return 0.2;
    } else {
      return 0.1;
    }
  }
  return 0;
}

// ✅ After: Guard Clauses
function getDiscount(user) {
  if (!user) return 0;
  if (user.isVIP) return 0.2;
  return 0.1;
}

Tier 3 - Requires Care (higher risk, break into smaller steps):

| Smell | Refactoring | Caution | | -------------------------- | ------------------------------ | ------------------------------------------- | | God class | Extract Class | Do incrementally, move one method at a time | | Type-checking conditionals | Replace with Polymorphism | Requires class hierarchy | | Too many parameters | Introduce Parameter Object | Changes function signature | | Complex loop | Replace Loop with Pipeline | Ensure equivalent behavior |

Tie-breaker: If multiple refactorings apply, choose smallest scope first (Rename < Extract Variable < Extract Function < Extract Class).

---

Phase 4: VERIFY

After each refactoring:

1. Run tests - Must pass
2. If tests pass: Commit with refactor: [what changed]
3. If tests fail: Revert immediately

Revert Protocol

git checkout -- <changed-files>

After revert:

• Was the refactoring too large? → Try smaller step
• Did it accidentally change behavior? → Reconsider approach
• DO NOT attempt to "fix" a failed refactoring

After 2 Failed Attempts

STOP. Ask user:

"I've attempted this refactoring twice and tests keep failing. This suggests either:

1. The refactoring is too large (need smaller steps)
2. The code has hidden dependencies
3. Tests are brittle

How would you like to proceed?"

---

Phase 5: ITERATE

More refactoring needed?
├─ Yes → Return to Phase 3 (one more refactoring)
└─ No → Done
    └─ Report: "Refactoring complete. Changes: [summary]"

---

Edge Cases

Partial test coverage:

• Identify which functions are tested vs untested
• Add characterization tests only for code you're about to refactor
• Don't boil the ocean - test what you touch

Refactoring reveals a bug:

• STOP refactoring
• Note the bug location
• Ask user: "Found potential bug at X. Fix it now (switching to tdd-enforcer) or continue refactoring?"

User requests large refactoring:

• Break into steps: "I'll refactor this incrementally. First: [step 1]"
• Complete each step fully before next
• Never batch multiple refactorings in one edit

---

Anti-Patterns

| Don't | Do | | ------------------------------- | ------------------------------------- | | Batch multiple refactorings | One refactoring → test → commit | | "Fix" a failed refactoring | Revert, then try smaller step | | Refactor without tests | Add characterization tests first | | Change behavior during refactor | That's a feature/fix, not refactoring | | Skip the commit | Commit after every green test |

---

Key Takeaways

1. One change at a time - Never batch refactorings
2. Tests before refactoring - No safety net = no refactoring
3. Revert on failure - Don't fix, revert and retry smaller
4. Commit after each success - refactor: [description]
5. Smallest scope first - Rename < Extract < Move < Restructure