Code Refactor

Overview

This skill guides systematic refactoring of one or more codebase components through three phases:

•Research - Deep analysis of component behavior, usage patterns, and dependencies
•Proposal - Concrete change suggestions with code samples and impact analysis
•Test Plan - Test strategy to validate changes without regressions

Each phase produces a markdown document, creating a complete refactoring proposal that can be reviewed before implementation.

Output Structure

Ask the user for an output directory (e.g., ./docs/refactoring/ or ./proposals/).

Create a subfolder for the refactoring effort:

code

{output-directory}/
└── {component-name}-refactor/
    ├── 01-research.md      # Phase 1: Component Analysis
    ├── 02-proposal.md      # Phase 2: Change Proposals
    └── 03-test-plan.md     # Phase 3: Test Strategy

For multi-component refactoring, create subfolders:

code

{output-directory}/
└── {thread-name}-refactor/
    ├── component-a/
    │   ├── 01-research.md
    │   ├── 02-proposal.md
    │   └── 03-test-plan.md
    └── component-b/
        ├── 01-research.md
        ├── 02-proposal.md
        └── 03-test-plan.md

Phase 1: Research

Goal: Comprehensive understanding of the component before proposing changes.

For multi-component refactoring, use parallel Explore agents to research each component simultaneously, then synthesize findings.

Step 1: Component Inventory (Use /codebase-librarian)

Start with the /codebase-librarian skill to establish baseline understanding:

•Project foundation (language, tooling)
•Entry points relevant to the component
•Services the component interacts with
•Infrastructure dependencies
•Domain model elements involved

This provides context for the deeper component analysis that follows.

Step 2: Component Deep Dive

After the inventory, analyze the specific component(s) being refactored.

Understand how it works:

•Core responsibilities and behavior
•Internal structure and control flow
•State management and data transformations
•Error handling patterns
•Configuration and initialization

Understand instantiation and usage:

•Where is it instantiated?
•How many instantiation patterns exist?
•What parameters/dependencies are injected?
•Is it a singleton, factory-created, or ad-hoc?

Map the surroundings:

•Dependents (what calls this component)
•Dependencies (what this component calls)
•Before: What happens before invocation?
•After: What happens after invocation?

Step 3: Call Graph

Create a visual representation of the component's call relationships:

code

┌─────────────────────────────────────────────────────────────┐
│                        CALLERS                              │
├─────────────────────────────────────────────────────────────┤
│  OrderController.create()  ──┐                              │
│  OrderController.update()  ──┼──► PaymentService            │
│  CheckoutWorker.process()  ──┘                              │
└─────────────────────────────────────────────────────────────┘
                              │
                              ▼
┌─────────────────────────────────────────────────────────────┐
│                    PaymentService                           │
├─────────────────────────────────────────────────────────────┤
│  processPayment()                                           │
│  refundPayment()                                            │
│  validateCard()                                             │
└─────────────────────────────────────────────────────────────┘
                              │
                              ▼
┌─────────────────────────────────────────────────────────────┐
│                      DEPENDENCIES                           │
├─────────────────────────────────────────────────────────────┤
│  StripeClient          │  PaymentRepository                 │
│  FraudDetectionService │  EventEmitter                      │
└─────────────────────────────────────────────────────────────┘

Research Output Format

Write to 01-research.md:

markdown

# Research: [Component Name]

**Generated**: [Date]
**Scope**: [Component path/module]

## Executive Summary

[2-3 paragraphs covering:]
- Component's purpose and role in the system
- Key findings about current implementation
- Areas of concern or complexity discovered

## Component Analysis

### Core Behavior
[What the component does, its responsibilities]

### Internal Structure
[Classes, functions, modules that compose it]

### State & Data Flow
[How data moves through the component]

## Instantiation Patterns

| Location | Pattern | Parameters | Frequency |
|----------|---------|------------|-----------|
| `src/api/orders.ts:45` | Direct new | config, logger | 3 places |
| `src/workers/checkout.ts:12` | Factory | config | 1 place |

## Dependency Analysis

### Dependents (Who calls this)

| Caller | Method | Context |
|--------|--------|---------|
| `OrderController` | `processPayment()` | HTTP request handling |
| `CheckoutWorker` | `processPayment()` | Background job |

### Dependencies (What this calls)

| Dependency | Usage | Abstracted? |
|------------|-------|-------------|
| `StripeClient` | Payment processing | No - direct SDK |
| `PaymentRepository` | Persistence | Yes - interface |

### Execution Context

**Before invocation**: [What typically happens before this component is called]
**After invocation**: [What happens with the result]

## Call Graph

[ASCII diagram as shown above]

## Key Observations

- [Notable pattern or concern 1]
- [Notable pattern or concern 2]
- [Areas that may need attention during refactoring]

Phase 2: Proposal

Persona: Pragmatic Senior Engineer

Mindset: Balance ideal architecture with practical constraints. Quick wins build momentum and trust. Large changes need clear justification. Every change should have a clear "why".

Proposal Structure

Order changes from quick wins to complex restructuring:

•Quick Wins - Small, low-risk improvements (hours)
•Medium Changes - Meaningful refactoring (days)
•Large Restructuring - Significant architectural changes (weeks)

For Each Change

Provide:

•Problem Statement - What's wrong and why it matters
•Proposed Solution - Concrete approach
•Code Samples - Before and after comparisons
•Impact Analysis - Files affected, risks, dependencies
•Pros and Cons - Honest trade-off assessment

Proposal Output Format

Write to 02-proposal.md:

markdown

# Refactoring Proposal: [Component Name]

**Generated**: [Date]
**Based on**: 01-research.md

## Executive Summary

[Overview of proposed changes, expected benefits, and effort distribution]

---

## Quick Wins

### 1. [Change Title]

**Problem**
[What's wrong - reference research findings]

**Solution**
[What to do]

**Before**
```typescript
// src/services/payment.ts:45-52
class PaymentService {
  constructor() {
    this.stripe = new Stripe(process.env.STRIPE_KEY);
  }
}

After

typescript

// src/services/payment.ts:45-55
class PaymentService {
  constructor(private readonly paymentGateway: PaymentGateway) {}
}

// src/ports/payment-gateway.ts (new)
interface PaymentGateway {
  charge(amount: number, currency: string): Promise<ChargeResult>;
}

Impact

•Files modified: src/services/payment.ts, src/composition-root.ts
•Files created: src/ports/payment-gateway.ts
•Risk: Low - constructor signature change requires updating instantiation sites

Pros

•Enables unit testing without Stripe sandbox
•Prepares for potential payment provider switch

Cons

•Adds indirection layer
•Requires updating 3 instantiation sites

Medium Changes

2. [Change Title]

[Same structure as above]

Large Restructuring

3. [Change Title]

[Same structure - may be higher level for larger efforts]

Implementation Order

Recommended sequence considering dependencies:

•[Quick Win 1] - No dependencies
•[Quick Win 2] - No dependencies
•[Medium Change 1] - Depends on Quick Win 1
•[Large Restructuring] - Depends on Medium Change 1

Risk Assessment

Change	Risk Level	Mitigation
[Change 1]	Low	Existing tests cover behavior
[Change 2]	Medium	Add integration tests first
[Change 3]	High	Feature flag, gradual rollout

code


---

## Phase 3: Test Plan

**Goal**: Design tests that validate refactoring without regressions.

### Analysis Steps

1. **Audit existing tests** - What's already covered?
2. **Identify gaps** - What behavior lacks test coverage?
3. **Design new tests** - Focus on integration over unit tests
4. **Minimize mocks** - Prefer real dependencies when feasible

### Test Philosophy

- **Favor integration tests** - Test real interactions between components
- **Use mocks sparingly** - Only for external services, not internal dependencies
- **Test behavior, not implementation** - Tests should survive refactoring
- **Cover edge cases discovered in research** - Use Phase 1 findings

### Test Plan Output Format

Write to `03-test-plan.md`:

```markdown
# Test Plan: [Component Name] Refactoring

**Generated**: [Date]
**Based on**: 01-research.md, 02-proposal.md

## Executive Summary

[Overview of testing strategy and coverage goals]

## Current Test Coverage

### Existing Tests

| Test File | Type | Coverage | Notes |
|-----------|------|----------|-------|
| `payment.test.ts` | Unit | Core payment flow | Heavy mocking |
| `checkout.integration.ts` | Integration | Happy path only | No error cases |

### Coverage Gaps

- [ ] Error handling in `processPayment()`
- [ ] Concurrent payment attempts
- [ ] Retry behavior after transient failures

## Test Strategy

### Integration Tests (Preferred)

Tests that exercise real component interactions:

**Test: Payment processing end-to-end**
```typescript
// tests/integration/payment.test.ts
describe('PaymentService', () => {
  let service: PaymentService;
  let testDb: TestDatabase;

  beforeEach(async () => {
    testDb = await TestDatabase.create();
    service = new PaymentService(
      new StripeTestGateway(),  // Test mode, real API
      new PaymentRepository(testDb)
    );
  });

  it('processes payment and persists record', async () => {
    const result = await service.processPayment({
      amount: 1000,
      currency: 'usd',
      customerId: 'cust_123'
    });

    expect(result.status).toBe('succeeded');

    const record = await testDb.payments.findById(result.id);
    expect(record).toBeDefined();
    expect(record.amount).toBe(1000);
  });
});

Why integration over unit: Tests real database interactions, actual service coordination, and catches integration bugs that mocks would hide.

Unit Tests (When Necessary)

Use unit tests with mocks only when:

•External API calls would be slow/costly
•Testing pure business logic in isolation
•Simulating error conditions difficult to reproduce

Test: Fraud detection logic

typescript

// tests/unit/fraud-detection.test.ts
describe('FraudDetector', () => {
  it('flags high-risk transactions', () => {
    const detector = new FraudDetector();

    const result = detector.assess({
      amount: 10000,
      newCustomer: true,
      internationalCard: true
    });

    expect(result.riskLevel).toBe('high');
    expect(result.requiresReview).toBe(true);
  });
});

Why unit here: Pure logic, no external dependencies, fast execution.

Tests Per Proposal Change

For Change 1: [Extract PaymentGateway Interface]

Test	Type	Purpose
`gateway-contract.test.ts`	Contract	Verify adapter implements interface correctly
`payment-with-mock-gateway.test.ts`	Unit	Verify service works with any gateway

New test:

typescript

// tests/contract/payment-gateway.test.ts
describe('PaymentGateway contract', () => {
  const adapters = [
    ['Stripe', () => new StripePaymentAdapter(stripeTestClient)],
    ['Mock', () => new MockPaymentAdapter()],
  ];

  test.each(adapters)('%s adapter implements contract', (name, createAdapter) => {
    const adapter = createAdapter();

    expect(adapter.charge).toBeDefined();
    expect(adapter.refund).toBeDefined();
  });
});

For Change 2: [Change Title]

[Similar structure]

Mock Usage Guidelines

Acceptable mocks:

•External payment APIs (Stripe, PayPal)
•Email services
•SMS/notification services
•Third-party APIs with rate limits

Avoid mocking:

•Internal services (use real implementations)
•Repositories (use test database)
•Domain logic (test directly)

Verification Checklist

Before considering refactoring complete:

• All existing tests pass
• New integration tests cover changed behavior
• Edge cases from research are tested
• Error handling paths have coverage
• No increase in mock usage
• CI pipeline green

code


---

## References

For detailed guidance on each phase:

- **Phase 1 deep dive**: See [references/research-phase.md](references/research-phase.md) for advanced research techniques
- **Phase 2 patterns**: See [references/proposal-phase.md](references/proposal-phase.md) for common refactoring patterns
- **Phase 3 testing**: See [references/test-plan-phase.md](references/test-plan-phase.md) for test design patterns