Dev Loop Refactor Guide

Refactoring Guide

Purpose

Systematically improve code structure, readability, maintainability, and extensibility without changing external behavior — applying proven refactoring techniques, identifying code smells, and establishing refactoring workflows that reduce technical debt safely.

Agent Protocol

Trigger

Exact user phrases: "refactor", "code smell", "technical debt", "improve code", "clean up code", "extract method", "restructure", "modernize", "legacy code", "refactoring strategy", "decouple", "extract class".

Input Context

• Codebase size and age (new project → legacy codebase)
• Language and framework (affects available refactoring tools)
• Current pain points (duplication, long methods, tight coupling, slow tests)
• Test coverage (% and quality)
• Refactoring goal (improve readability, enable new feature, reduce bugs, pay down debt)
• Risk tolerance (safety-critical, high-traffic, low-risk internal tool)

Output Artifact

Refactoring plan with prioritized code smells, technique application, and verification strategy.

Completion Criteria

• Code smells identified and cataloged
• Refactoring techniques selected for each smell
• Refactoring order prioritized (high value, low risk first)
• Test coverage verified for code being refactored
• Refactoring applied incrementally (one change at a time)
• Behavior preserved (tests pass before/after)
• Code review completed for refactoring PRs
• Documentation updated if API changed

Max Response Length

200 lines.

Framework/Methodology

Refactoring Decision Tree

What is the primary code quality issue?
├── Duplicated code → Extract method / Extract class
├── Long method (>20 lines) → Extract method / Replace temp with query
├── Large class (>300 lines) → Extract class / Extract interface
├── Too many parameters (>3) → Introduce parameter object
├── Feature envy → Move method / Extract class
├── Switch/if-else chain → Replace with strategy / polymorphism
├── Data clump → Introduce parameter object / Extract class
├── Shotgun surgery (one change = many files) → Move method / Inline class
├── Divergent change (file changes for multiple reasons) → Extract class per concern
├── Speculative generality → Inline class / Collapse hierarchy
├── Message chain (a.b().c().d()) → Hide delegate / Extract method
└── Primitive obsession → Replace primitive with value object

Refactoring Rule of Three

First time:   Do it (get it working)
Second time:  Duplicate it (get it working again)
Third time:   Refactor it (eliminate duplication)

Don't refactor on first occurrence — you don't understand the pattern yet.

The Cambrian Explosion Trap

When refactoring, resist the urge to create the "perfect" abstraction immediately. Instead, make the simplest change that improves the code, then iterate.

Strategy:
1. Identify the smell
2. Apply the minimal refactoring technique
3. Run tests (verify behavior preserved)
4. Commit
5. Repeat

Workflow

Step 1: Identify Code Smells

// SMELL: Long method (>20 lines)
// SMELL: Deep nesting (arrow code)
function processOrder(order: Order, user: User, config: Config): Promise<Result> {
  if (order && order.items && order.items.length > 0) {
    if (user && user.isActive) {
      if (config && config.enabled) {
        let total = 0;
        for (const item of order.items) {
          if (item.price && item.quantity) {
            total += item.price * item.quantity;
            if (item.discount) {
              total -= item.discount;
            }
          }
        }
        if (total > 0) {
          if (user.balance >= total) {
            user.balance -= total;
            return saveOrder(order, user, total);
          } else {
            throw new Error('Insufficient balance');
          }
        } else {
          throw new Error('Invalid total');
        }
      } else {
        throw new Error('Config not enabled');
      }
    } else {
      throw new Error('User not active');
    }
  } else {
    throw new Error('Invalid order');
  }
}

// SMELL: Primitive obsession (using string for concept)
function sendEmail(to: string, from: string, subject: string, body: string, cc?: string, bcc?: string, replyTo?: string) {
  // ...
}

// SMELL: Feature envy (method uses more from other class)
class OrderService {
  generateInvoice(order: Order): Invoice {
    const invoice = new Invoice();
    invoice.customerName = order.customer.name;
    invoice.customerEmail = order.customer.email; // Uses Customer fields
    invoice.customerAddress = order.customer.shippingAddress; // More Customer
    invoice.items = order.items;
    invoice.total = order.calculateTotal();
    return invoice;
  }
}

// SMELL: Shotgun surgery
// Changing shipping logic requires editing: OrderService, ShippingCalculator,
// ShippingController, ShippingValidator, ShippingRepository

Step 2: Apply Refactoring Techniques

// REFACTOR 1: Extract method + guard clauses
async function processOrder(order: Order, user: User, config: Config): Promise<Result> {
  validateOrderInputs(order, user, config);

  const total = calculateOrderTotal(order);
  validateUserBalance(user, total);

  user.balance -= total;
  return saveOrder(order, user, total);
}

function validateOrderInputs(order: Order | null, user: User | null, config: Config | null): void {
  if (!order?.items?.length) throw new Error('Invalid order');
  if (!user?.isActive) throw new Error('User not active');
  if (!config?.enabled) throw new Error('Config not enabled');
}

function calculateOrderTotal(order: Order): number {
  return order.items.reduce((total, item) => {
    const lineTotal = (item.price ?? 0) * (item.quantity ?? 0);
    return total + lineTotal - (item.discount ?? 0);
  }, 0);
}

function validateUserBalance(user: User, requiredAmount: number): void {
  if (user.balance < requiredAmount) {
    throw new Error('Insufficient balance');
  }
}

// REFACTOR 2: Introduce parameter object
interface EmailConfig {
  to: string;
  from: string;
  subject: string;
  body: string;
  cc?: string;
  bcc?: string;
  replyTo?: string;
}

function sendEmail(config: EmailConfig): void {
  // ...
}

// REFACTOR 3: Move method (feature envy fix)
class Customer {
  // ... fields ...
  generateInvoice(order: Order): Invoice {
    return new Invoice({
      name: this.name,
      email: this.email,
      address: this.shippingAddress,
      items: order.items,
      total: order.calculateTotal(),
    });
  }
}

// REFACTOR 4: Replace primitive with value object
class Email {
  constructor(private readonly value: string) {
    if (!value.includes('@')) throw new Error('Invalid email');
  }

  toString(): string { return this.value; }
  get domain(): string { return this.value.split('@')[1]; }
  get local(): string { return this.value.split('@')[0]; }
}

function sendEmail(to: Email, from: Email, ...): void { ... }

Step 3: Refactoring Workflow

refactoring_workflow:
  step_1:
    action: "Identify the smell"
    output: "Clear understanding of what's wrong"
    example: "This function has 7 responsibilities"
  step_2:
    action: "Ensure test coverage"
    check: "Existing tests cover the behavior being refactored"
    fallback: "Write characterization tests first"
  step_3:
    action: "Apply ONE refactoring technique"
    rule: "Only one structural change per commit"
    example: "Extract method for validation logic only"
  step_4:
    action: "Run tests"
    expectation: "All tests pass (behavior preserved)"
    if_fails: "Undo the change (the technique was wrong)"
  step_5:
    action: "Commit"
    message: "refactor: extract validation logic from processOrder"
  step_6:
    action: "Repeat"
    next: "Next smell or next technique on same code"

Step 4: Legacy Code Refactoring Strategy

legacy_refactoring:
  principles:
    - "Never refactor without tests (write characterization tests first)"
    - "Sprout method/class — add new code in new structure, then redirect"
    - "Wrap existing code before extracting"
    - "Change structure and behavior one at a time, never both"

  characterization_tests:
    - "Write tests that capture CURRENT behavior"
    - "Include edge cases that might seem wrong (known bugs)"
    - "Test at API boundaries, not internals"
    - "Accept current behavior as 'correct' for test purposes"

  sprout_method:
    - "1. Identify where new code would go"
    - "2. Write the new code in a NEW method"
    - "3. Call the new method from the existing code"
    - "4. The old code stays unchanged"

  sprout_class:
    - "1. Identify the new responsibility"
    - "2. Create a new class with the new code"
    - "3. Pass existing dependencies to the new class"
    - "4. Replace inline logic with delegated call"

  wrap_method:
    - "1. Rename the existing method to oldMethod"
    - "2. Create new method with original name"
    - "3. New method calls oldMethod with any pre/post processing"
    - "4. Gradually inline oldMethod calls into newMethod"

Step 5: Verify Behavior Preservation

// Tests MUST pass before and after refactoring (same tests)
describe('processOrder', () => {
  it('should reject null order', async () => {
    await expect(processOrder(null, user, config)).rejects.toThrow('Invalid order');
  });

  it('should reject inactive user', async () => {
    await expect(processOrder(validOrder, inactiveUser, config)).rejects.toThrow('User not active');
  });

  it('should reject insufficient balance', async () => {
    await expect(processOrder(validOrder, poorUser, config)).rejects.toThrow('Insufficient balance');
  });

  it('should deduct from user balance', async () => {
    const result = await processOrder(validOrder, richUser, config);
    expect(richUser.balance).toBe(initialBalance - expectedTotal);
  });

  // All characterizations of behavior preserved
});

Common Pitfalls

Best Practices

References

• references/refactor-guide-advanced.md — Refactoring Advanced Topics
• references/refactor-guide-code-smells.md — Code Smells Reference
• references/refactor-guide-fundamentals.md — Refactoring Fundamentals
• references/refactor-guide-techniques.md — Refactoring Techniques Reference

Implementation Patterns

Code Smell Detector

import ast
import re
from typing import List, Dict, Optional
from dataclasses import dataclass

@dataclass
class CodeSmell:
    type: str
    location: str
    description: str
    severity: str
    suggestion: str

class PythonSmellDetector:
    def __init__(self, source_code: str, filepath: str = "<unknown>"):
        self.source = source_code
        self.filepath = filepath
        self.tree = ast.parse(source_code)
        self.smells: List[CodeSmell] = []

    def detect_all(self) -> List[CodeSmell]:
        self.detect_long_function()
        self.detect_too_many_params()
        self.detect_god_class()
        self.detect_duplicated_code()
        self.detect_magic_numbers()
        self.detect_dead_code()
        return self.smells

    def detect_long_function(self, max_lines: int = 50):
        for node in ast.walk(self.tree):
            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
                lines = node.end_lineno - node.lineno if hasattr(node, 'end_lineno') else 0
                if lines > max_lines:
                    self.smells.append(CodeSmell(
                        type="long_function",
                        location=f"{self.filepath}:{node.lineno} in {node.name}()",
                        description=f"Function {node.name}() is {lines} lines (max: {max_lines})",
                        severity="medium",
                        suggestion=f"Extract {lines - max_lines} lines into helper functions. "
                                   f"Look for groups of related logic (validation, formatting, I/O).",
                    ))

    def detect_too_many_params(self, max_params: int = 5):
        for node in ast.walk(self.tree):
            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
                params = [a for a in node.args.args if a.arg != 'self']
                if len(params) > max_params:
                    self.smells.append(CodeSmell(
                        type="too_many_parameters",
                        location=f"{self.filepath}:{node.lineno} in {node.name}()",
                        description=f"{node.name}() has {len(params)} parameters (max: {max_params})",
                        severity="medium",
                        suggestion="Group related parameters into a dataclass or configuration object. "
                                   f"Params: {', '.join(p.arg for p in params)}",
                    ))

    def detect_god_class(self, max_methods: int = 20):
        for node in ast.walk(self.tree):
            if isinstance(node, ast.ClassDef):
                methods = [n for n in ast.walk(node) if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))]
                if len(methods) > max_methods:
                    self.smells.append(CodeSmell(
                        type="god_class",
                        location=f"{self.filepath}:{node.lineno} in class {node.name}",
                        description=f"Class {node.name} has {len(methods)} methods (max: {max_methods})",
                        severity="high",
                        suggestion=f"Split into {len(methods) // 10 + 1} focused classes. "
                                   f"Group methods by responsibility.",
                    ))

    def detect_magic_numbers(self):
        for node in ast.walk(self.tree):
            if isinstance(node, ast.Constant) and isinstance(node.value, (int, float)):
                if node.value not in (0, 1, -1, 100) and node.col_offset is not None:
                    # Check if it's used in a comparison or assignment
                    parent = self._find_parent(node)
                    if parent and not isinstance(parent, ast.Assign):
                        self.smells.append(CodeSmell(
                            type="magic_number",
                            location=f"{self.filepath}:{node.lineno}",
                            description=f"Magic number {node.value}",
                            severity="low",
                            suggestion=f"Replace {node.value} with a named constant.",
                        ))

    def detect_dead_code(self):
        for node in ast.walk(self.tree):
            if isinstance(node, ast.FunctionDef):
                # Check if function has no calls anywhere
                name = node.name
                if name.startswith('_') and not self._is_called(name):
                    pass  # Private methods may be unused — flag only for public
                elif not name.startswith('_') and not self._is_called(name):
                    self.smells.append(CodeSmell(
                        type="dead_code",
                        location=f"{self.filepath}:{node.lineno}",
                        description=f"Public function {name}() appears to be unused",
                        severity="low",
                        suggestion=f"Remove {name}() or add a caller. If it's a public API, add a usage comment.",
                    ))

    def detect_duplicated_code(self):
        lines = self.source.split("\n")
        seen_blocks: Dict[str, List[int]] = {}
        for i in range(len(lines) - 4):
            # Check 4-line blocks for similarity
            block = "\n".join(lines[i:i+4])
            # Normalize whitespace
            normalized = re.sub(r'\s+', ' ', block.strip())
            if len(normalized) > 40:
                if normalized not in seen_blocks:
                    seen_blocks[normalized] = []
                seen_blocks[normalized].append(i + 1)
        for block, locations in seen_blocks.items():
            if len(locations) > 1:
                self.smells.append(CodeSmell(
                    type="duplicated_code",
                    location=f"{self.filepath}:{locations[0]}",
                    description=f"Duplicated block found at lines {locations}",
                    severity="medium",
                    suggestion="Extract the duplicated block into a shared function.",
                ))

    def _find_parent(self, target_node) -> Optional[ast.AST]:
        for node in ast.walk(self.tree):
            for child in ast.iter_child_nodes(node):
                if child is target_node:
                    return node
        return None

    def _is_called(self, name: str) -> bool:
        for node in ast.walk(self.tree):
            if isinstance(node, ast.Call):
                if isinstance(node.func, ast.Name) and node.func.id == name:
                    return True
                if isinstance(node.func, ast.Attribute) and node.func.attr == name:
                    return True
        return False

Strangler Fig Pattern Implementation

from typing import Dict, Any, Callable, Optional
import re

class StranglerFigRouter:
    """
    Route traffic to new or legacy implementation based on migration state.
    Gradually replaces legacy code with new implementation.
    """
    def __init__(self):
        self.routes: Dict[str, Dict] = {}

    def register_route(
        self,
        pattern: str,
        new_impl: Callable,
        legacy_impl: Callable,
        migration_pct: float = 0.0,
    ):
        self.routes[pattern] = {
            "new": new_impl,
            "legacy": legacy_impl,
            "migration_pct": migration_pct,
            "active": False,
        }

    def set_migration_percentage(self, pattern: str, pct: float):
        if pattern in self.routes:
            self.routes[pattern]["migration_pct"] = pct
            self.routes[pattern]["active"] = pct > 0

    def route(self, request_path: str, context: Dict[str, Any] = None) -> Any:
        for pattern, route in self.routes.items():
            if re.match(pattern, request_path):
                if self._should_route_to_new(route, context):
                    return route["new"](context)
                return route["legacy"](context)
        raise ValueError(f"No route for {request_path}")

    def _should_route_to_new(self, route: Dict, context: Dict = None) -> bool:
        if route["migration_pct"] >= 100:
            return True
        if route["migration_pct"] <= 0:
            return False
        # Use request ID or user ID for deterministic routing
        identifier = str(context.get("request_id", context.get("user_id", "")))
        hash_val = sum(ord(c) for c in identifier)
        return (hash_val % 100) < route["migration_pct"]

    def get_migration_status(self) -> Dict[str, float]:
        return {
            pattern: route["migration_pct"]
            for pattern, route in self.routes.items()
        }

Architecture Decision Trees

Refactoring Approach Selection

What's the goal and risk level?
├── Improve readability (low risk)
│   └── Micro-refactoring: rename, extract, inline
│       ├── Works within a single function
│       ├── No behavior change
│       ├── Safe to do during feature work
│       └── Review: quick scan, no behavioral testing needed
│
├── Improve structure (medium risk)
│   ├── Extract class/module → Strangler Fig
│   ├── Split god function → Facade + delegates
│   ├── Replace conditional with polymorphism → Strategy pattern
│   └── Review: behavioral tests must pass, feature parity check
│
├── Improve performance (high risk)
│   ├── Algorithm replacement → Dual implementation with comparison
│   ├── Caching layer → Strangler Fig with shadow read
│   ├── Database optimization → Migration with rollback plan
│   └── Review: performance tests + behavioral tests + canary
│
└── Architecture change (very high risk)
    └── Strangler Fig Pattern
        ├── Monolith → Microservice
        ├── Synchronous → Async/event-driven
        ├── Old DB → New DB (dual-write + compare)
        └── Review: full integration tests, canary, rollback plan

Anti-Patterns

Performance Optimization

• Characterization test generation: Use AST analysis to auto-generate tests that capture current behavior. Run generated tests before and after refactoring to verify no behavioral change.
• Incremental type annotation: Use gradual type checking (e.g., mypy in non-strict mode). Add types to functions being refactored first. Let type checker catch regressions during refactoring.

References

• references/refactor-guide-advanced.md — Refactoring Advanced Topics
• references/refactor-guide-code-smells.md — Code Smells Reference
• references/refactor-guide-fundamentals.md — Refactoring Fundamentals
• references/refactor-guide-techniques.md — Refactoring Techniques Reference

Handoff

Hand off to dev-loop-code-review for refactoring PR review. Hand off to dev-loop-tech-debt-tracker for debt tracking.