safe-refactoring

name: safe-refactoring description: Safe refactoring patterns - code smell detection thresholds, baby-step refactoring, characterization tests, Expand-Migrate-Contract, Strangler Fig, primitive obsession, value objects keywords: refactoring, code smell, extract method, extract class, long method, god class, primitive obsession, value object, branded type, parallel change, expand contract, strangler fig, characterization test, feature flag, safe migration related: monorepo-architecture, tdd-workflow, error-handling-patterns, typescript-type-safety, domain-driven-design

Safe Refactoring Patterns

Rules for detecting code smells and refactoring safely without breaking production. Every refactoring must preserve behavior, be covered by tests, and proceed in small verified steps.

Research: docs/deepresearch/reports/TypeScript Code Smells and Safe Refactoring Patterns.md

Non-Negotiable Rules

Pattern 1: Safe Refactoring Process

WRONG - Big-bang rewrite

// Rewrite 500-line function in one commit, no tests
// -> Weekend outage, impossible to bisect

CORRECT - Baby steps with characterization tests

// Step 1: Add characterization test capturing CURRENT behavior
test('processOrder handles coupon + tax + shipping', () => {
  const result = processOrder(sampleDto);
  expect(result.total).toBe(42.99); // snapshot current behavior
  expect(result.items).toHaveLength(3);
});

// Step 2: Extract ONE small pure function
function calculateSubtotal(items: OrderItem[]): number {
  return items.reduce((sum, i) => sum + i.price * i.qty, 0);
}

// Step 3: Compile + run test suite -> passes? Commit.
// Step 4: Repeat for next extraction

Sequence: (1) Write characterization test -> (2) Extract small function -> (3) Compile + test -> (4) Commit -> repeat. If >3 extraction steps needed, wrap in feature flag.

Feature flag for risky refactors

const useNewProcessor = config.get('feature.newOrderProcessor');
const processor = useNewProcessor ? newOrderProcessor : legacyProcessor;

Pattern 2: Expand-Migrate-Contract (Parallel Change)

For any change to a public API, DB schema, or shared interface.

WRONG - Direct breaking change

// Before: all callers use this
function sendEmail(to: string, body: string): void { ... }

// After: signature changed, 47 callers break
function sendEmail(params: EmailParams): void { ... }

CORRECT - Three-phase migration

// Phase 1: EXPAND - add new alongside old
function sendEmail(to: string, body: string): void { ... }  // @deprecated
function sendEmailV2(params: EmailParams): void { ... }

// Phase 2: MIGRATE - update callers one at a time, each its own commit
// file1.ts: sendEmail(to, body) -> sendEmailV2({ to, body, format: 'html' })
// file2.ts: same...

// Phase 3: CONTRACT - remove old after all callers migrated
// Delete sendEmail, rename sendEmailV2 -> sendEmail

DB schema variant:

1. EXPAND: Add new column (nullable), dual-write both old and new
2. MIGRATE: Backfill old rows, switch reads to new column
3. CONTRACT: Drop old column after verification period

Rule: Never combine expand + contract in one commit. Each phase is a separate PR/commit.

Pattern 3: Primitive Obsession -> Value Objects

WRONG - Raw primitives for domain concepts

function charge(amount: number, currency: string, userId: string) { ... }
// No validation, easy to swap amount/currency args, no type safety
charge(100, 'user-123', 'USD'); // args swapped silently

CORRECT - Branded types + Value Objects

// Branded type for IDs (zero runtime cost)
type UserId = string & { readonly __brand: 'UserId' };
function userId(raw: string): UserId {
  if (!raw.startsWith('usr_')) throw new Error(`Invalid UserId: ${raw}`);
  return raw as UserId;
}

// Value Object for Money (rich behavior)
type Currency = 'USD' | 'EUR' | 'GBP';
class Money {
  constructor(
    public readonly amount: number,
    public readonly currency: Currency,
  ) {
    if (amount < 0) throw new Error('Amount must be non-negative');
  }
  add(other: Money): Money {
    if (this.currency !== other.currency) throw new Error('Currency mismatch');
    return new Money(this.amount + other.amount, this.currency);
  }
}

function charge(money: Money, user: UserId) { ... }
// charge(new Money(100, 'USD'), userId('usr_123')); // type-safe, validated

When to use:

• IDs: Branded types (zero runtime cost, prevents mixing UserId with OrderId)
• Money, dates, coordinates: Value Objects (validation + behavior)
• Status/enums: Discriminated unions (compiler-enforced exhaustiveness)

Smell: Same validation logic for a primitive repeated in >2 places -> extract Value Object.

Pattern 4: Switch Statement Explosion -> Strategy

WRONG - Switch duplicated across codebase

// In pricing.ts
switch (user.type) {
  case 'ADMIN': return price * 0.5;
  case 'PREMIUM': return price * 0.7;
  case 'GUEST': return price;
}

// In permissions.ts - SAME switch on user.type
switch (user.type) {
  case 'ADMIN': return ALL_PERMISSIONS;
  case 'PREMIUM': return PREMIUM_PERMISSIONS;
  case 'GUEST': return GUEST_PERMISSIONS;
}
// Adding new user type -> touch EVERY switch (shotgun surgery)

CORRECT - Discriminated union + strategy map

type User = { kind: 'admin'; /* ... */ } | { kind: 'premium' } | { kind: 'guest' };

const discountStrategy: Record<User['kind'], (price: number) => number> = {
  admin: (p) => p * 0.5,
  premium: (p) => p * 0.7,
  guest: (p) => p,
};
// Adding new kind -> TS error on incomplete Record -> find all strategies at once

Threshold: >3 cases OR same switch in >2 places -> extract to strategy map or polymorphism. Single-location switch with <=3 cases is fine.

Pattern 5: When to Refactor vs Rewrite

Decision Framework

Strangler Fig Pattern

// Step 1: New module handles new requests
app.post('/orders', stranglerMiddleware(legacyHandler, newHandler));

// Step 2: Route increasing traffic to new handler
function stranglerMiddleware(legacy: Handler, next: Handler): Handler {
  return (req, res) => {
    if (shouldUseNew(req)) return next(req, res);
    return legacy(req, res);
  };
}

// Step 3: When 100% traffic on new -> remove legacy

Rule: Never big-bang rewrite unless the codebase has zero users and zero tests. Even then, prefer incremental. Rewrites fail because nobody understands all edge cases.

Code Smell Detection Heuristics (for AI agents)

Important: These heuristics are triggers for proposing a refactor, not auto-applying. Always: (1) confirm with tests, (2) baby-step extraction, (3) compile + verify, (4) commit atomically.

Common Anti-Patterns Checklist

Before proposing or executing a refactor:

• Characterization test exists for the behavior being changed
• Each step is small enough to revert independently
• Public API changes follow Expand-Migrate-Contract
• Feature flag wraps risky structural changes
• No big-bang rewrite without Strangler Fig consideration
• Primitives with domain meaning replaced with Value Objects (not raw strings/numbers)
• Switch statements checked for duplication across codebase
• Extracted functions are pure where possible (easier to test)
• Commit after each successful extraction step
• No premature abstraction (Rule of Three from [[monorepo-architecture]])

See Also

• [[monorepo-architecture]] - God objects, Rule of Three, module boundaries, interface design
• [[tdd-workflow]] - Test-first development that enables safe refactoring
• [[error-handling-patterns]] - Error handling patterns to preserve during refactoring
• [[typescript-type-safety]] - Type safety patterns (branded types, satisfies, unknown)
• [[domain-driven-design]] - Value Objects, aggregates, bounded contexts