Code Quality

Code Quality

Systematic code improvement through review, refactoring, and debugging with verification gates.

Module Selection

---

Core Principles

YAGNI, KISS, DRY - Always honor these.

Be honest, be brutal, straight to the point, and be concise.

Technical correctness over social comfort. Verify before implementing. Ask before assuming. Evidence before claims.

---

Code Review

Receiving Feedback

Pattern: READ → UNDERSTAND → VERIFY → EVALUATE → RESPOND → IMPLEMENT

Rules:

• ❌ No performative agreement ("You're right!", "Great point!")
• ❌ No implementation before verification
• ✅ Restate requirement, ask questions, push back with reasoning
• ✅ YAGNI check: grep for usage before implementing suggestions

Sources:

• Human partner → Implement after understanding
• External reviewer → Verify technically before implementing

Requesting Review

When: After major features, before merge, after complex fixes

Process: 1. Get SHAs: BASE_SHA=$(git rev-parse HEAD~1), HEAD_SHA=$(git rev-parse HEAD) 2. Dispatch code-reviewer subagent with: WHAT, PLAN, SHAs, DESCRIPTION 3. Fix Critical immediately, Important before proceeding, note Minor

Verification Gates

Iron Law: NO COMPLETION CLAIMS WITHOUT FRESH VERIFICATION EVIDENCE

Gate: IDENTIFY → RUN → READ → VERIFY → THEN claim

Skip any step = lying, not verifying.

---

Refactoring

Mindset

Simplify relentlessly. Preserve behavior religiously. Measure everything.

Every refactoring: small and safe, tested immediately, measurably better.

Protocol

1. Assessment - Baseline metrics, identify smells, classify debt 2. Safety Net - Verify test coverage, add characterization tests 3. Red-Green-Refactor - Write failing test, minimal pass, improve design 4. Pattern Application - SOLID, design patterns, functional transforms 5. Validation - Measure improvements, verify behavior preserved

Code Smells (5 Categories)

1. Bloaters: Long Method, Large Class, Long Parameter List 2. OO Abusers: Switch Statements, Temporary Field 3. Change Preventers: Divergent Change, Shotgun Surgery 4. Dispensables: Duplicate Code, Dead Code, Lazy Class 5. Couplers: Feature Envy, Inappropriate Intimacy

Quick Patterns

OOP: Extract Method, Inline, Replace Temp with Query, Guard Clauses Functional: Map/Filter/Reduce, Pure Functions, Composition, Immutability

---

Debugging

Core Principle

NO FIXES WITHOUT ROOT CAUSE INVESTIGATION FIRST

Random fixes waste time and create new bugs.

The Four Techniques

1. Systematic Debugging (references/systematic-debugging.md)

• Phase 1: Root Cause Investigation
• Phase 2: Pattern Analysis
• Phase 3: Hypothesis Testing
• Phase 4: Implementation

2. Root Cause Tracing (references/root-cause-tracing.md) Trace backward through call stack to find original trigger.

3. Defense-in-Depth (references/defense-in-depth.md) Validate at every layer: Entry → Business logic → Environment → Debug

4. Verification (references/verification.md) Run command. Read output. Then claim result.

Quick Reference

Bug → systematic-debugging.md (Phase 1-4)
  Error deep in stack? → root-cause-tracing.md
  Found root cause? → defense-in-depth.md
  About to claim success? → verification.md

Red Flags

Stop if thinking:

• "Quick fix for now, investigate later"
• "Just try changing X and see"
• "It's probably X, let me fix that"
• "Should work now" / "Seems fixed"

---

References

Code Review

• references/code-review-reception.md - Response protocols
• references/requesting-code-review.md - Request process
• references/verification-before-completion.md - Verification gates

Refactoring

• references/oop-refactoring-catalog.md - Martin Fowler's patterns
• references/functional-refactoring-patterns.md - FP transformations
• references/code-smells-reference.md - 23 smells, 5 categories
• references/testing-strategies.md - Characterization tests, TDD

Debugging

• references/systematic-debugging.md - Four-phase framework
• references/root-cause-tracing.md - Call stack analysis
• references/defense-in-depth.md - Multi-layer validation
• references/verification.md - Verification protocols

---

Bottom Line

1. Review: Technical rigor over social performance 2. Refactor: Small, safe, tested, measurable 3. Debug: Root cause first, fix once 4. Always: Evidence before claims

Verify. Question. Then implement. Evidence. Then claim.

Code Review Reception

Overview

Code review requires technical evaluation, not emotional performance.

Core principle: Verify before implementing. Ask before assuming. Technical correctness over social comfort.

The Response Pattern

WHEN receiving code review feedback:

1. READ: Complete feedback without reacting
2. UNDERSTAND: Restate requirement in own words (or ask)
3. VERIFY: Check against codebase reality
4. EVALUATE: Technically sound for THIS codebase?
5. RESPOND: Technical acknowledgment or reasoned pushback
6. IMPLEMENT: One item at a time, test each

Forbidden Responses

NEVER:

• "You're absolutely right!" (explicit CLAUDE.md violation)
• "Great point!" / "Excellent feedback!" (performative)
• "Let me implement that now" (before verification)

INSTEAD:

• Restate the technical requirement
• Ask clarifying questions
• Push back with technical reasoning if wrong
• Just start working (actions > words)

Handling Unclear Feedback

IF any item is unclear:
  STOP - do not implement anything yet
  ASK for clarification on unclear items

WHY: Items may be related. Partial understanding = wrong implementation.

Example:

your human partner: "Fix 1-6"
You understand 1,2,3,6. Unclear on 4,5.

❌ WRONG: Implement 1,2,3,6 now, ask about 4,5 later
✅ RIGHT: "I understand items 1,2,3,6. Need clarification on 4 and 5 before proceeding."

Source-Specific Handling

From your human partner

• Trusted - implement after understanding
• Still ask if scope unclear
• No performative agreement
• Skip to action or technical acknowledgment

From External Reviewers

BEFORE implementing:
  1. Check: Technically correct for THIS codebase?
  2. Check: Breaks existing functionality?
  3. Check: Reason for current implementation?
  4. Check: Works on all platforms/versions?
  5. Check: Does reviewer understand full context?

IF suggestion seems wrong:
  Push back with technical reasoning

IF can't easily verify:
  Say so: "I can't verify this without [X]. Should I [investigate/ask/proceed]?"

IF conflicts with your human partner's prior decisions:
  Stop and discuss with your human partner first

your human partner's rule: "External feedback - be skeptical, but check carefully"

YAGNI Check for "Professional" Features

IF reviewer suggests "implementing properly":
  grep codebase for actual usage

  IF unused: "This endpoint isn't called. Remove it (YAGNI)?"
  IF used: Then implement properly

your human partner's rule: "You and reviewer both report to me. If we don't need this feature, don't add it."

Implementation Order

FOR multi-item feedback:
  1. Clarify anything unclear FIRST
  2. Then implement in this order:
     - Blocking issues (breaks, security)
     - Simple fixes (typos, imports)
     - Complex fixes (refactoring, logic)
  3. Test each fix individually
  4. Verify no regressions

When To Push Back

Push back when:

• Suggestion breaks existing functionality
• Reviewer lacks full context
• Violates YAGNI (unused feature)
• Technically incorrect for this stack
• Legacy/compatibility reasons exist
• Conflicts with your human partner's architectural decisions

How to push back:

• Use technical reasoning, not defensiveness
• Ask specific questions
• Reference working tests/code
• Involve your human partner if architectural

Signal if uncomfortable pushing back out loud: "Strange things are afoot at the Circle K"

Acknowledging Correct Feedback

When feedback IS correct:

✅ "Fixed. [Brief description of what changed]"
✅ "Good catch - [specific issue]. Fixed in [location]."
✅ [Just fix it and show in the code]

❌ "You're absolutely right!"
❌ "Great point!"
❌ "Thanks for catching that!"
❌ "Thanks for [anything]"
❌ ANY gratitude expression

Why no thanks: Actions speak. Just fix it. The code itself shows you heard the feedback.

If you catch yourself about to write "Thanks": DELETE IT. State the fix instead.

Gracefully Correcting Your Pushback

If you pushed back and were wrong:

✅ "You were right - I checked [X] and it does [Y]. Implementing now."
✅ "Verified this and you're correct. My initial understanding was wrong because [reason]. Fixing."

❌ Long apology
❌ Defending why you pushed back
❌ Over-explaining

State the correction factually and move on.

Common Mistakes

Real Examples

Performative Agreement (Bad):

Reviewer: "Remove legacy code"
❌ "You're absolutely right! Let me remove that..."

Technical Verification (Good):

Reviewer: "Remove legacy code"
✅ "Checking... build target is 10.15+, this API needs 13+. Need legacy for backward compat. Current impl has wrong bundle ID - fix it or drop pre-13 support?"

YAGNI (Good):

Reviewer: "Implement proper metrics tracking with database, date filters, CSV export"
✅ "Grepped codebase - nothing calls this endpoint. Remove it (YAGNI)? Or is there usage I'm missing?"

Unclear Item (Good):

your human partner: "Fix items 1-6"
You understand 1,2,3,6. Unclear on 4,5.
✅ "Understand 1,2,3,6. Need clarification on 4 and 5 before implementing."

The Bottom Line

External feedback = suggestions to evaluate, not orders to follow.

Verify. Question. Then implement.

No performative agreement. Technical rigor always.

Code Smells Reference

Comprehensive catalog of code smells organized by category, based on refactoring.guru and Martin Fowler's work.

Table of Contents

• Understanding Code Smells
• Category 1: Bloaters
• Category 2: Object-Orientation Abusers
• Category 3: Change Preventers
• Category 4: Dispensables
• Category 5: Couplers
• Finding Specific Content

---

Understanding Code Smells

Definition: Surface indications that usually correspond to deeper problems in the system. Not bugs—code works correctly—but weaknesses in design that slow development or increase risk of bugs.

Origin: Term coined by Kent Beck, popularized by Martin Fowler

Key Principle: Code smells are hints, not rules. Context matters. Use professional judgment.

---

Category 1: Bloaters

Code, methods, and classes that have grown so large they're hard to work with. Usually accumulate over time as program evolves.

Long Method (>20-30 lines)

Description: Method doing too much, hard to understand

Signs:

• Method exceeds 20-30 lines
• Multiple levels of abstraction
• Hard to name meaningfully
• Contains comments explaining sections

Problems:

• Cognitive overload
• Hard to test
• Difficult to reuse
• Hides business logic

Refactorings:

• Extract Method
• Replace Temp with Query
• Decompose Conditional
• Preserve Whole Object

Example:

// Before: Long method
function processOrder(order) {
  // Validate order (10 lines)
  // Calculate totals (15 lines)
  // Apply discounts (20 lines)
  // Save to database (10 lines)
  // Send notifications (15 lines)
}

// After: Extracted methods
function processOrder(order) {
  validateOrder(order);
  const total = calculateTotal(order);
  const discountedTotal = applyDiscounts(total, order);
  saveOrder(order, discountedTotal);
  sendNotifications(order);
}

---

Large Class (>200-300 lines)

Description: Class trying to do too much

Signs:

• Class exceeds 200-300 lines
• Many instance variables
• Many methods
• Violates Single Responsibility Principle

Problems:

• Hard to understand
• Difficult to maintain
• Low cohesion
• Changes affect many parts

Refactorings:

• Extract Class
• Extract Subclass
• Extract Interface
• Replace Data Value with Object

---

Long Parameter List (>3-4 parameters)

Description: Too many parameters make method calls complex

Signs:

• Method takes >3-4 parameters
• Parameters often passed together
• Same parameters across multiple methods

Problems:

• Hard to remember parameter order
• Error-prone calls
• Difficult to read
• Often indicates missing abstraction

Refactorings:

• Introduce Parameter Object
• Preserve Whole Object
• Replace Parameter with Query

Example:

// Before: Long parameter list
function createUser(name, email, age, address, phone, role, department) {
  // ...
}

// After: Parameter object
function createUser(userData) {
  // ...
}

---

Primitive Obsession

Description: Overuse of primitives instead of small objects

Signs:

• Using strings/numbers for domain concepts
• Constants for coded information
• Field names simulating types (userID, userName)

Problems:

• Loss of type safety
• Validation scattered
• Business rules in multiple places
• Difficult to extend

Refactorings:

• Replace Data Value with Object
• Replace Type Code with Class
• Extract Class

Example:

// Before: Primitive obsession
const email = "user@example.com";  // Just a string
function validateEmail(email) { /* validation */ }

// After: Value object
class Email {
  constructor(value) {
    if (!this.isValid(value)) {
      throw new Error('Invalid email');
    }
    this.value = value;
  }

  isValid(email) {
    return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
  }
}

---

Data Clumps

Description: Same group of data items always appear together

Signs:

• Same 2-3 parameters in multiple methods
• Groups of fields that go together
• Removing one makes others meaningless

Problems:

• Duplicated parameter lists
• Missing abstraction
• Difficult to maintain

Refactorings:

• Extract Class
• Introduce Parameter Object
• Preserve Whole Object

---

Category 2: Object-Orientation Abusers

Incomplete or incorrect application of object-oriented principles.

Switch Statements

Description: Type checking with switch/if-else instead of polymorphism

Signs:

• Switch on type code
• Multiple places switching on same value
• Adding new type requires finding all switches

Problems:

• Violates Open/Closed Principle
• Scattered logic
• Easy to miss cases

Refactorings:

• Replace Conditional with Polymorphism
• Replace Type Code with Subclasses
• Introduce Null Object

Example:

// Before: Switch statement
function getArea(shape) {
  switch (shape.type) {
    case 'circle':
      return Math.PI * shape.radius ** 2;
    case 'rectangle':
      return shape.width * shape.height;
    case 'triangle':
      return 0.5 * shape.base * shape.height;
  }
}

// After: Polymorphism
class Circle {
  getArea() { return Math.PI * this.radius ** 2; }
}
class Rectangle {
  getArea() { return this.width * this.height; }
}
class Triangle {
  getArea() { return 0.5 * this.base * this.height; }
}

---

Temporary Field

Description: Field only set in certain circumstances

Signs:

• Instance variable used by only some methods
• Field null most of the time
• Confusing object state

Problems:

• Unexpected null values
• Hard to understand object lifecycle
• Maintenance confusion

Refactorings:

• Extract Class
• Introduce Null Object

---

Refused Bequest

Description: Subclass doesn't use inherited methods

Signs:

• Subclass overrides methods to do nothing
• Subclass throws "not supported" errors
• Inheritance for code reuse, not specialization

Problems:

• Violates Liskov Substitution Principle
• Misleading hierarchy
• Fragile design

Refactorings:

• Replace Inheritance with Delegation
• Extract Superclass (push down used methods)

---

Alternative Classes with Different Interfaces

Description: Classes doing similar things with different method names

Signs:

• Similar functionality, different interfaces
• Duplicated logic
• Should be interchangeable but aren't

Problems:

• Code duplication
• Can't use polymorphically
• Maintenance burden

Refactorings:

• Rename Method
• Move Method
• Extract Superclass

---

Category 3: Change Preventers

Changes in one place require changes in many other places.

Divergent Change

Description: Class changes for multiple reasons

Signs:

• One class modified for different types of changes
• "When we add X, we change these methods; when we add Y, we change those methods"
• Violates Single Responsibility Principle

Problems:

• High change frequency
• Risk of breaking unrelated functionality
• Hard to understand impact

Refactorings:

• Extract Class
• Extract Superclass
• Extract Subclass

Example:

// Before: Divergent change
class User {
  // Database operations
  save() { /* ... */ }
  load() { /* ... */ }

  // Business logic
  validateEmail() { /* ... */ }
  hashPassword() { /* ... */ }

  // Presentation
  toJSON() { /* ... */ }
  toHTML() { /* ... */ }
}

// After: Separated concerns
class UserRepository {
  save(user) { /* ... */ }
  load(id) { /* ... */ }
}

class UserValidator {
  validateEmail(email) { /* ... */ }
}

class UserPresenter {
  toJSON(user) { /* ... */ }
  toHTML(user) { /* ... */ }
}

---

Shotgun Surgery

Description: Single change requires many small edits across multiple classes

Signs:

• Adding feature requires changes in many places
• Changes scattered across codebase
• Hard to find all affected areas

Problems:

• Easy to miss changes
• High risk of bugs
• Time-consuming changes

Refactorings:

• Move Method
• Move Field
• Inline Class

---

Parallel Inheritance Hierarchies

Description: Creating subclass in one hierarchy requires creating in another

Signs:

• Two hierarchies grow in parallel
• Adding to one means adding to other
• Often sharing prefix names

Problems:

• Duplicate work
• Easy to forget parallel change
• Maintenance burden

Refactorings:

• Move Method
• Move Field

---

Category 4: Dispensables

Something pointless whose absence would make code cleaner and more efficient.

Comments (Excessive)

Description: Comments explaining what code does (not why)

Signs:

• Comment explains what next lines do
• Comments compensate for poor naming
• Outdated comments

Problems:

• Comments lie (code changes, comments don't)
• Clutter
• Should be in code, not comments

Refactorings:

• Extract Method (name explains intent)
• Rename Method
• Introduce Assertion

Note: Good comments explain WHY, not WHAT

---

Duplicate Code

Description: Same code structure in multiple places

Signs:

• Copy-pasted code
• Similar algorithms
• Same expressions in multiple methods

Problems:

• Changes needed in multiple places
• Inconsistent modifications
• Maintenance nightmare

Refactorings:

• Extract Method
• Extract Class
• Pull Up Method
• Form Template Method

---

Dead Code

Description: Unused code

Signs:

• Unreachable code (after return)
• Unused methods, classes, variables
• Parameters never used

Problems:

• Maintenance confusion
• False sense of importance
• Wasted mental effort

Refactorings:

• Remove Dead Code (delete it!)

How to Find:

• Code coverage tools
• Static analysis
• IDE unused warnings

---

Lazy Class

Description: Class doing too little to justify existence

Signs:

• Class with few methods
• Created for "future" that never came
• Simple delegations only

Problems:

• Unnecessary abstraction
• Maintenance overhead
• Clutters codebase

Refactorings:

• Inline Class
• Collapse Hierarchy

---

Speculative Generality

Description: Unused abstraction for hypothetical future

Signs:

• Hooks and special cases "just in case"
• Abstract classes with one subclass
• Unused parameters
• Methods called from tests only

Problems:

• YAGNI violation
• Complexity without benefit
• Hard to understand purpose

Refactorings:

• Inline Class
• Remove Parameter
• Rename Method (make purpose clear)

---

Category 5: Couplers

Excessive coupling between classes.

Feature Envy

Description: Method uses another class more than its own

Signs:

• Method calling many getters on another object
• Logic clearly belongs elsewhere
• Method uses few of its own class's features

Problems:

• Wrong responsibility
• Poor cohesion
• Difficult to change

Refactorings:

• Move Method
• Extract Method then Move Method

Example:

// Before: Feature envy
class OrderReport {
  generate(order) {
    // Uses order extensively
    const total = order.items.reduce((sum, item) =>
      sum + item.price * item.quantity, 0
    );
    const tax = total * order.taxRate;
    const shipping = order.calculateShipping();
    return total + tax + shipping;
  }
}

// After: Moved to Order
class Order {
  calculateTotal() {
    const subtotal = this.items.reduce((sum, item) =>
      sum + item.price * item.quantity, 0
    );
    const tax = subtotal * this.taxRate;
    const shipping = this.calculateShipping();
    return subtotal + tax + shipping;
  }
}

---

Inappropriate Intimacy

Description: Classes too tightly coupled

Signs:

• Classes accessing each other's private fields
• Spending too much time together
• Changing one requires changing other

Problems:

• Hard to change independently
• Fragile design
• Testing difficulties

Refactorings:

• Move Method
• Move Field
• Extract Class
• Hide Delegate

---

Message Chains

Description: Client asks object for another object, which asks for another...

Signs:

• Long chains: a.getB().getC().getD()
• Law of Demeter violations
• Client knows too much about structure

Problems:

• Fragile - breaks if chain changes
• High coupling
• Hard to test

Refactorings:

• Hide Delegate
• Extract Method

Example:

// Before: Message chain
const manager = employee.getDepartment().getManager();

// After: Hide delegate
const manager = employee.getManager();

// In Employee class:
getManager() {
  return this.department.getManager();
}

---

Middle Man

Description: Class exists only to delegate to another

Signs:

• Most methods just delegate
• Class adds no value
• Simple pass-through

Problems:

• Unnecessary indirection
• Maintenance overhead
• Confusing design

Refactorings:

• Remove Middle Man
• Inline Method
• Replace Delegation with Inheritance

---

Incomplete Library Class

Description: Library missing needed functionality

Signs:

• Need methods library doesn't provide
• Can't modify library code
• Workarounds in multiple places

Problems:

• Scattered workarounds
• Duplicated solutions
• Difficult to change

Refactorings:

• Introduce Foreign Method
• Introduce Local Extension

---

Finding Specific Content

Search patterns for quick reference:

# Find specific smell by name
grep -i "long method" references/code-smells-reference.md

# Find smell category
grep -i "bloaters" references/code-smells-reference.md

# Find refactoring for smell
grep -A 5 "Long Method" references/code-smells-reference.md | grep "Refactorings"

# Find all smells in category
grep -A 100 "Category 1: Bloaters" references/code-smells-reference.md

---

Sources:

• refactoring.guru - comprehensive code smell taxonomy
• Martin Fowler, "Refactoring" (2nd Edition, 2018)
• Industry best practices and patterns

Defense-in-Depth Validation

Validate at every layer data passes through to make bugs impossible.

Core Principle

Validate at EVERY layer data passes through. Make bug structurally impossible.

When fix bug caused by invalid data, adding validation at one place feels sufficient. But single check can be bypassed by different code paths, refactoring, or mocks.

Why Multiple Layers

Single validation: "We fixed bug" Multiple layers: "We made bug impossible"

Different layers catch different cases:

• Entry validation catches most bugs
• Business logic catches edge cases
• Environment guards prevent context-specific dangers
• Debug logging helps when other layers fail

The Four Layers

Layer 1: Entry Point Validation

Purpose: Reject obviously invalid input at API boundary

function createProject(name: string, workingDirectory: string) {
  if (!workingDirectory || workingDirectory.trim() === '') {
    throw new Error('workingDirectory cannot be empty');
  }
  if (!existsSync(workingDirectory)) {
    throw new Error(`workingDirectory does not exist: ${workingDirectory}`);
  }
  if (!statSync(workingDirectory).isDirectory()) {
    throw new Error(`workingDirectory is not a directory: ${workingDirectory}`);
  }
  // proceed
}

Layer 2: Business Logic Validation

Purpose: Ensure data makes sense for this operation

function initializeWorkspace(projectDir: string, sessionId: string) {
  if (!projectDir) {
    throw new Error('projectDir required for workspace initialization');
  }
  // proceed
}

Layer 3: Environment Guards

Purpose: Prevent dangerous operations in specific contexts

async function gitInit(directory: string) {
  // In tests, refuse git init outside temp directories
  if (process.env.NODE_ENV === 'test') {
    const normalized = normalize(resolve(directory));
    const tmpDir = normalize(resolve(tmpdir()));

    if (!normalized.startsWith(tmpDir)) {
      throw new Error(
        `Refusing git init outside temp dir during tests: ${directory}`
      );
    }
  }
  // proceed
}

Layer 4: Debug Instrumentation

Purpose: Capture context for forensics

async function gitInit(directory: string) {
  const stack = new Error().stack;
  logger.debug('About to git init', {
    directory,
    cwd: process.cwd(),
    stack,
  });
  // proceed
}

Applying the Pattern

When find bug:

1. Trace data flow - Where does bad value originate? Where used? 2. Map all checkpoints - List every point data passes through 3. Add validation at each layer - Entry, business, environment, debug 4. Test each layer - Try to bypass layer 1, verify layer 2 catches it

Example from Real Session

Bug: Empty projectDir caused git init in source code

Data flow: 1. Test setup → empty string 2. Project.create(name, '') 3. WorkspaceManager.createWorkspace('') 4. git init runs in process.cwd()

Four layers added:

• Layer 1: Project.create() validates not empty/exists/writable
• Layer 2: WorkspaceManager validates projectDir not empty
• Layer 3: WorktreeManager refuses git init outside tmpdir in tests
• Layer 4: Stack trace logging before git init

Result: All 1847 tests passed, bug impossible to reproduce

Key Insight

All four layers were necessary. During testing, each layer caught bugs others missed:

• Different code paths bypassed entry validation
• Mocks bypassed business logic checks
• Edge cases on different platforms needed environment guards
• Debug logging identified structural misuse

Don't stop at one validation point. Add checks at every layer.

Functional Programming Refactoring Patterns

Transform imperative, stateful code into functional, declarative code following modern FP best practices.

Table of Contents

• Core FP Principles
• Transformation Patterns
• FP-Specific Code Smells
• Benefits of Functional Refactoring
• Finding Specific Content

---

Core FP Principles

Immutability

Definition: Data cannot be modified after creation; return new copies instead Benefits: Predictable state, no hidden mutations, safe concurrency Application: Use spread operators, Array.map/filter instead of mutations

Pure Functions

Definition: Same input always produces same output; no side effects Benefits: Deterministic behavior, easy testing, referential transparency Application: No external state access, no I/O, no mutations

Declarative Style

Definition: Express what to compute, not how Benefits: Intent over implementation, less error-prone Application: map/filter/reduce instead of for-loops

Function Composition

Definition: Build complex operations from simple functions Benefits: Reusability, modularity, pipeline thinking Application: compose(...fns), pipe(...fns), chaining operations

First-Class Functions

Definition: Functions as values - pass, return, store Benefits: Higher-order functions, callbacks, strategies Application: Functions as parameters, return functions, store in data structures

---

Transformation Patterns

1. Replace Loops with Map/Filter/Reduce

Before (Imperative):

const results = [];
for (let i = 0; i < items.length; i++) {
  if (items[i].active) {
    results.push(items[i].name.toUpperCase());
  }
}

After (Functional):

const results = items
  .filter(item => item.active)
  .map(item => item.name.toUpperCase());

Benefits:

• No mutation of results array
• No index tracking
• Declarative intent (filter then transform)
• Chainable operations

When to Apply:

• Any for/while loop building collection
• Array processing with conditionals
• Multiple transformation steps

---

2. Extract Pure Functions

Before (Impure - side effects):

let total = 0;
function addToTotal(value) {
  total += value;  // Mutates external state
  console.log('Adding:', value);  // Side effect: I/O
  return total;
}

After (Pure):

function add(a, b) {
  return a + b;  // No side effects, deterministic
}

// Usage
const total = items.reduce(add, 0);

Benefits:

• Testable without mocks
• No hidden dependencies
• Safe to parallelize
• Referentially transparent

When to Apply:

• Functions accessing global state
• Functions with I/O mixed with logic
• Functions with unpredictable behavior

---

3. Higher-Order Functions (HOFs)

Pattern: Functions that take or return functions

HOF Returning Function (Currying-like):

const multiplyBy = (factor) => (number) => number * factor;

// Specialized functions
const double = multiplyBy(2);
const triple = multiplyBy(3);

double(5);  // 10
triple(5);  // 15

HOF Taking Function (Callback):

const applyOperation = (operation, value) => operation(value);

applyOperation(double, 10);  // 20
applyOperation(Math.sqrt, 16);  // 4

Benefits:

• Code reuse through parameterization
• Delayed execution, lazy evaluation
• Strategy pattern without classes

When to Apply:

• Common operations with varying behavior
• Configurable transformations
• Abstraction over algorithms

---

4. Function Composition

Pattern: Combine simple functions into complex ones

Individual Functions:

const trim = (str) => str.trim();
const lowercase = (str) => str.toLowerCase();
const removeSpaces = (str) => str.replace(/\s+/g, '');

Compose Utility (right-to-left):

const compose = (...fns) => (x) =>
  fns.reduceRight((v, f) => f(v), x);

Pipe Utility (left-to-right):

const pipe = (...fns) => (x) =>
  fns.reduce((v, f) => f(v), x);

Usage:

const normalize = pipe(trim, lowercase, removeSpaces);

normalize('  Hello World  ');  // "helloworld"

Benefits:

• Build complex from simple
• Reusable pipeline
• Point-free style (no intermediate variables)

When to Apply:

• Multi-step transformations
• Data pipelines
• Reducing nested function calls

---

5. Currying

Pattern: Transform multi-argument function into single-argument sequence

Before (Multiple Arguments):

const add = (a, b, c) => a + b + c;
add(1, 2, 3);  // 6

After (Curried):

const curriedAdd = (a) => (b) => (c) => a + b + c;

// Partial application
const add5 = curriedAdd(5);
const add5And10 = add5(10);
add5And10(3);  // 18

// Or directly
curriedAdd(5)(10)(3);  // 18

Auto-Curry Utility:

const curry = (fn) => {
  return function curried(...args) {
    if (args.length >= fn.length) {
      return fn.apply(this, args);
    } else {
      return (...moreArgs) => curried(...args, ...moreArgs);
    }
  };
};

const add = curry((a, b, c) => a + b + c);
add(1)(2)(3);  // 6
add(1, 2)(3);  // 6
add(1)(2, 3);  // 6

Benefits:

• Partial application
• Reusable specialized functions
• Function factories

When to Apply:

• Functions with multiple parameters
• Creating specialized versions
• Building function pipelines

---

6. Eliminate Mutation

Before (Mutation):

const user = { name: 'Alice', age: 30 };
user.age = 31;  // Mutates object

const numbers = [1, 2, 3];
numbers.push(4);  // Mutates array

After (Immutable):

// Object update
const user = { name: 'Alice', age: 30 };
const updatedUser = { ...user, age: 31 };  // New object

// Array update
const numbers = [1, 2, 3];
const newNumbers = [...numbers, 4];  // New array

// Nested update
const state = {
  user: { name: 'Alice', preferences: { theme: 'light' } }
};
const newState = {
  ...state,
  user: {
    ...state.user,
    preferences: {
      ...state.user.preferences,
      theme: 'dark'
    }
  }
};

Immutability Libraries:

// Immer (JavaScript)
import produce from 'immer';

const nextState = produce(state, draft => {
  draft.user.preferences.theme = 'dark';
});

// Immutable.js
import { Map } from 'immutable';

const map1 = Map({ a: 1, b: 2 });
const map2 = map1.set('b', 3);  // New map

Benefits:

• No unexpected mutations
• Time-travel debugging
• Easier reasoning about state

When to Apply:

• Any state updates
• Array/object transformations
• React/Redux state management

---

7. Replace Null with Maybe/Option Monad

Before (Null Checks):

function getUserEmail(user) {
  if (user && user.profile && user.profile.email) {
    return user.profile.email;
  }
  return null;
}

// Usage
const email = getUserEmail(user);
if (email) {
  sendEmail(email);
}

After (Maybe Monad):

const Maybe = (value) => ({
  map: (fn) => value != null ? Maybe(fn(value)) : Maybe(null),
  flatMap: (fn) => value != null ? fn(value) : Maybe(null),
  getOrElse: (defaultValue) => value != null ? value : defaultValue,
  isPresent: () => value != null
});

const getUserEmail = (user) =>
  Maybe(user)
    .map(u => u.profile)
    .map(p => p.email)
    .getOrElse('no-email@example.com');

// Usage
const email = getUserEmail(user);
// No null check needed, always has value
sendEmail(email);

Functional Libraries:

// Folktale (JavaScript)
import { Maybe } from 'folktale/maybe';

const result = Maybe.fromNullable(user)
  .map(u => u.profile)
  .map(p => p.email)
  .getOrElse('default@example.com');

Benefits:

• No null pointer exceptions
• Explicit handling of absence
• Composable null checks

When to Apply:

• Nested null checks
• Optional values
• Chaining operations on potentially null values

---

8. Separate Side Effects from Pure Logic

Before (Mixed):

function processOrder(order) {
  // Pure calculation
  const total = order.items.reduce((sum, item) =>
    sum + item.price, 0
  );

  // Side effects mixed in
  saveToDatabase(order);
  sendEmail(order.customer);
  console.log('Order processed:', order.id);

  return total;
}

After (Separated):

// Pure logic
function calculateOrderTotal(order) {
  return order.items.reduce((sum, item) => sum + item.price, 0);
}

function prepareOrderSummary(order, total) {
  return {
    orderId: order.id,
    customer: order.customer,
    total,
    timestamp: new Date().toISOString()
  };
}

// Side effects isolated
function performOrderEffects(orderSummary) {
  return Promise.all([
    saveToDatabase(orderSummary),
    sendEmail(orderSummary.customer, orderSummary),
    logOrder(orderSummary)
  ]);
}

// Orchestration
async function processOrder(order) {
  const total = calculateOrderTotal(order);  // Pure
  const summary = prepareOrderSummary(order, total);  // Pure
  await performOrderEffects(summary);  // Effects at boundary
  return total;
}

Benefits:

• Pure logic easy to test
• Side effects explicit and controlled
• Effects pushed to boundaries

When to Apply:

• Functions mixing calculation and I/O
• Business logic with database calls
• Any function with side effects

---

FP-Specific Code Smells

Mutation

Smell: Modifying variables/objects in place Refactoring: Use spread operators, map/filter, immutable libraries Example: user.age++ → { ...user, age: user.age + 1 }

Side Effects in Pure Functions

Smell: I/O, logging, state changes in computation Refactoring: Separate pure logic from effects Example: Extract console.log, database calls, API requests

Imperative Loops

Smell: Using for/while instead of map/filter/reduce Refactoring: Replace with functional array methods Example: for-loop building array → .map()/.filter()

Nested Conditionals

Smell: Deep if-else instead of pattern matching/guards Refactoring: Guard clauses, early returns, Maybe monad Example: Nested if-else → guard clauses with early returns

Null/Undefined Checks

Smell: Manual null handling everywhere Refactoring: Maybe/Option monad Example: Multiple if (x != null) → Maybe(x).map(...)

Shared Mutable State

Smell: Global variables accessed by multiple functions Refactoring: Pass state as parameters, return new state Example: Global counter → functional accumulator

Object-Oriented Patterns in FP

Smell: Classes with methods instead of functions with data Refactoring: Convert to functions operating on data structures Example: user.getName() → getName(user)

---

Benefits of Functional Refactoring

Testability

• Pure functions easy to test (no mocks, no setup)
• Deterministic: same input always produces same output
• No hidden dependencies or state

Predictability

• Referential transparency
• No action at a distance
• Easier to reason about code

Parallelism

• No shared mutable state
• Safe concurrent execution
• Automatic parallelization possible

Debugging

• No hidden state changes
• Time-travel debugging (immutability)
• Stack traces more meaningful

Reusability

• Composable functions more reusable than methods
• Higher-order functions create function families
• Small, focused functions easy to combine

---

Finding Specific Content

Search patterns for quick reference:

# Find transformation pattern
grep -i "map filter reduce" references/functional-refactoring-patterns.md

# Find monad information
grep -i "maybe monad" references/functional-refactoring-patterns.md

# Find immutability patterns
grep -i "immutab" references/functional-refactoring-patterns.md

# Find HOF examples
grep -i "higher-order" references/functional-refactoring-patterns.md

---

Sources:

• Functional programming best practices (2024-2025)
• Martin Fowler's functional refactoring patterns
• Industry guides on immutability and pure functions

OOP Refactoring Catalog

Comprehensive catalog of Object-Oriented refactoring patterns from Martin Fowler's "Refactoring: Improving the Design of Existing Code" (2nd Edition, 2018).

Table of Contents

• Method-Level Refactorings
• Class-Level Refactorings
• Conditional Refactorings
• Data Refactorings
• General Refactorings
• SOLID Principles
• Automated Tooling

---

Method-Level Refactorings

Extract Method

Problem: Long method doing too much Solution: Break into focused, named functions When: Method > 20-30 lines, complex logic, duplicated code fragments

Inline Method

Problem: Method body clearer than method name Solution: Replace method call with body content When: Over-abstraction, trivial delegation, preparing for Extract Method elsewhere

Extract Variable

Problem: Complex expression hard to understand Solution: Assign expression to well-named variable When: Nested calculations, repeated expressions, unclear intent

Inline Variable

Problem: Variable adds no clarity Solution: Replace variable with expression directly When: Simple assignment, used once, refactoring preparation

Replace Temp with Query

Problem: Temporary variable storing calculation Solution: Convert to method returning value When: Calculation used multiple times, promotes reusability

Split Temporary Variable

Problem: Variable assigned multiple times for different purposes Solution: Create separate variable for each purpose When: Variable has multiple responsibilities, confusing updates

---

Class-Level Refactorings

Extract Class

Problem: Class doing work of two classes Solution: Create new class, move relevant fields/methods When: Large class, subset of methods operating on subset of data

Inline Class

Problem: Class doing too little Solution: Merge class into another When: Class lost responsibility through refactoring, needless abstraction

Move Method

Problem: Method uses another class more than its own Solution: Move method to class it uses most When: Feature Envy smell, improving cohesion

Move Field

Problem: Field used more by another class Solution: Move field to class that uses it most When: Data and behavior should be together

Hide Delegate

Problem: Client calling delegate object through another Solution: Create delegating method on server When: Reducing coupling, encapsulation improvement

Remove Middle Man

Problem: Class doing too much delegation Solution: Client calls delegate directly When: Class exists only to delegate

Introduce Foreign Method

Problem: Need method on class you can't modify Solution: Create method in client class with first parameter as server object When: Library/framework limitation, temporary solution

Introduce Local Extension

Problem: Need several foreign methods Solution: Create subclass or wrapper with needed methods When: Multiple foreign methods needed, more permanent solution

---

Conditional Refactorings

Decompose Conditional

Problem: Complex conditional with unclear intent Solution: Extract condition and each branch into named methods When: Long conditionals, nested if-else, unclear business logic

Consolidate Conditional Expression

Problem: Multiple conditionals with same result Solution: Combine into single conditional, extract to method When: Multiple checks leading to same action

Consolidate Duplicate Conditional Fragments

Problem: Same code in every branch Solution: Move common code outside conditional When: Duplicated pre/post processing in branches

Replace Conditional with Polymorphism

Problem: Conditional choosing behavior based on type Solution: Create subclasses with overridden methods When: Type checking with switch/if-else, OOP preferred

Replace Nested Conditional with Guard Clauses

Problem: Deep nesting making normal path unclear Solution: Use early returns for special cases When: Nested if-else, abnormal conditions mixed with normal flow

Introduce Null Object

Problem: Repeated null checks Solution: Create null object class with do-nothing behavior When: Many null checks, default behavior for null case

Introduce Assertion

Problem: Assumption not explicitly stated Solution: Add assertion to make assumption explicit When: Documenting preconditions, debugging complex logic

---

Data Refactorings

Replace Magic Numbers

Problem: Numeric literals with unclear meaning Solution: Create named constant or enum When: Numbers with business meaning, repeated literals

Introduce Parameter Object

Problem: Related parameters always passed together Solution: Group into object When: Long parameter lists, data clumps, 3+ related parameters

Remove Parameter

Problem: Parameter no longer used Solution: Delete parameter When: Unused parameters, changing requirements

Preserve Whole Object

Problem: Extracting multiple values from object to pass Solution: Pass entire object When: Getting several values from object, reducing parameter list

Replace Parameter with Query

Problem: Parameter value derivable from another Solution: Remove parameter, get value via method call When: Removing unnecessary parameters, improving encapsulation

Replace Query with Parameter

Problem: Method accessing global/external state Solution: Pass value as parameter When: Removing dependencies, making pure function

Replace Data Value with Object

Problem: Primitive value needs additional data/behavior Solution: Convert to object When: Primitive Obsession smell, value has validation rules

Change Value to Reference

Problem: Many identical instances of value object Solution: Convert to reference object with single instance When: Memory optimization, centralized updates

Change Reference to Value

Problem: Reference object small, immutable, awkward to manage Solution: Convert to value object When: Simplifying design, enabling immutability

---

General Refactorings

Remove Dead Code

Problem: Unused methods, variables, parameters Solution: Delete unused code When: Code coverage reveals unused paths, outdated features

Separate Query from Modifier

Problem: Method both returns value and changes state Solution: Split into query and command methods When: Side effects in queries, Command-Query Separation principle

Parameterize Method

Problem: Several methods doing similar things with different values Solution: Single method with parameter for varying value When: Duplicated logic with literal variations

Replace Constructor with Factory Method

Problem: Constructor doing more than simple construction Solution: Replace with factory method When: Complex creation logic, type-based construction, polymorphic creation

Encapsulate Field

Problem: Public field Solution: Make private, provide accessors When: Direct field access, adding validation/transformation

Encapsulate Collection

Problem: Method returning collection reference Solution: Return read-only view, provide add/remove methods When: Preventing external modification, maintaining invariants

Replace Type Code with Class

Problem: Numeric/string type code Solution: Create class for type code When: Type safety, type-specific behavior

Replace Type Code with Subclasses

Problem: Type code affecting behavior Solution: Create subclass for each type When: Type-dependent behavior, polymorphism preferred

Replace Subclass with Fields

Problem: Subclasses differing only in constant values Solution: Replace with fields in superclass When: Trivial subclasses, reducing class explosion

---

SOLID Principles

Single Responsibility Principle (SRP)

Definition: Class should have one reason to change Violations: Large classes, Divergent Change smell Refactorings: Extract Class, Extract Method, Move Method

Open/Closed Principle (OCP)

Definition: Open for extension, closed for modification Violations: Modifying existing code for new features Refactorings: Replace Conditional with Polymorphism, Strategy Pattern, Template Method

Liskov Substitution Principle (LSP)

Definition: Subtypes must be substitutable for base types Violations: Refused Bequest smell, strengthened preconditions Refactorings: Extract Interface, Replace Inheritance with Delegation

Interface Segregation Principle (ISP)

Definition: Clients depend only on methods they use Violations: Fat interfaces, clients implementing unused methods Refactorings: Extract Interface, Interface splitting

Dependency Inversion Principle (DIP)

Definition: Depend on abstractions, not concretions Violations: Direct dependencies on concrete classes Refactorings: Extract Interface, Dependency Injection, Inversion of Control

---

Automated Tooling

Static Analysis Tools

SonarQube - Comprehensive code quality platform (all languages)

• Detects code smells, bugs, security vulnerabilities
• Quality gates, technical debt tracking
• Integration: CI/CD pipelines, IDE plugins

ESLint - JavaScript/TypeScript linting

• Customizable rules, auto-fix capabilities
• Integration: VSCode, WebStorm, CI/CD

Pylint/Ruff - Python code analysis

• PEP 8 compliance, code smell detection
• Fast performance (Ruff), comprehensive checks (Pylint)

RuboCop - Ruby static analysis

• Style guide enforcement, auto-correction
• Customizable rules, Rails integration

Checkstyle/PMD/SpotBugs - Java quality tools

• Checkstyle: Style compliance
• PMD: Code smell detection
• SpotBugs: Bug pattern detection

IDE Refactoring Support

Visual Studio Code

• Built-in: Extract Method, Rename Symbol, Move to File
• Extensions: Refactoring tools, language-specific support

IntelliJ IDEA

• Advanced refactoring automation (20+ refactorings)
• Safe refactoring with usage search
• Language support: Java, Kotlin, JavaScript, etc.

Eclipse

• Extensive refactoring tools
• Quick fixes, automated transformations

PyCharm

• Python-specific refactorings
• Type-aware transformations

CI/CD Integration

Quality Gates

• Set thresholds: code coverage (>80%), complexity limits
• Fail builds on violations
• Prevent technical debt accumulation

Continuous Monitoring

• Track metrics over time
• Trend analysis, regression detection
• Dashboard reporting

---

Finding Specific Content

Search patterns for quick reference:

# Find specific refactoring
grep -i "extract method" references/oop-refactoring-catalog.md

# Find SOLID principle
grep -i "single responsibility" references/oop-refactoring-catalog.md

# Find tooling information
grep -i "sonarqube" references/oop-refactoring-catalog.md

---

Source: Martin Fowler, "Refactoring: Improving the Design of Existing Code" (2nd Edition, 2018) Additional: refactoring.guru, industry best practices

Requesting Code Review

Dispatch code-reviewer subagent to catch issues before they cascade.

Core principle: Review early, review often.

When to Request Review

Mandatory:

• After each task in subagent-driven development
• After completing major feature
• Before merge to main

Optional but valuable:

• When stuck (fresh perspective)
• Before refactoring (baseline check)
• After fixing complex bug

How to Request

1. Get git SHAs:

BASE_SHA=$(git rev-parse HEAD~1)  # or origin/main
HEAD_SHA=$(git rev-parse HEAD)

2. Dispatch code-reviewer subagent:

Use Task tool with code-reviewer type, fill template at code-reviewer.md

Placeholders:

• {WHAT_WAS_IMPLEMENTED} - What you just built
• {PLAN_OR_REQUIREMENTS} - What it should do
• {BASE_SHA} - Starting commit
• {HEAD_SHA} - Ending commit
• {DESCRIPTION} - Brief summary

3. Act on feedback:

• Fix Critical issues immediately
• Fix Important issues before proceeding
• Note Minor issues for later
• Push back if reviewer is wrong (with reasoning)

Example

[Just completed Task 2: Add verification function]

You: Let me request code review before proceeding.

BASE_SHA=$(git log --oneline | grep "Task 1" | head -1 | awk '{print $1}')
HEAD_SHA=$(git rev-parse HEAD)

[Dispatch code-reviewer subagent]
  WHAT_WAS_IMPLEMENTED: Verification and repair functions for conversation index
  PLAN_OR_REQUIREMENTS: Task 2 from docs/plans/deployment-plan.md
  BASE_SHA: a7981ec
  HEAD_SHA: 3df7661
  DESCRIPTION: Added verifyIndex() and repairIndex() with 4 issue types

[Subagent returns]:
  Strengths: Clean architecture, real tests
  Issues:
    Important: Missing progress indicators
    Minor: Magic number (100) for reporting interval
  Assessment: Ready to proceed

You: [Fix progress indicators]
[Continue to Task 3]

Integration with Workflows

Subagent-Driven Development:

• Review after EACH task
• Catch issues before they compound
• Fix before moving to next task

Executing Plans:

• Review after each batch (3 tasks)
• Get feedback, apply, continue

Ad-Hoc Development:

• Review before merge
• Review when stuck

Red Flags

Never:

• Skip review because "it's simple"
• Ignore Critical issues
• Proceed with unfixed Important issues
• Argue with valid technical feedback

If reviewer wrong:

• Push back with technical reasoning
• Show code/tests that prove it works
• Request clarification

See template at: requesting-code-review/code-reviewer.md

Root Cause Tracing

Systematically trace bugs backward through call stack to find original trigger.

Core Principle

Trace backward through call chain until finding original trigger, then fix at source.

Bugs often manifest deep in call stack (git init in wrong directory, file created in wrong location). Instinct is to fix where error appears, but that's treating symptom.

When to Use

Use when:

• Error happens deep in execution (not at entry point)
• Stack trace shows long call chain
• Unclear where invalid data originated
• Need to find which test/code triggers problem

The Tracing Process

1. Observe the Symptom

Error: git init failed in /Users/jesse/project/packages/core

2. Find Immediate Cause

What code directly causes this?

await execFileAsync('git', ['init'], { cwd: projectDir });

3. Ask: What Called This?

WorktreeManager.createSessionWorktree(projectDir, sessionId)
  → called by Session.initializeWorkspace()
  → called by Session.create()
  → called by test at Project.create()

4. Keep Tracing Up

What value was passed?

• projectDir = '' (empty string!)
• Empty string as cwd resolves to process.cwd()
• That's the source code directory!

5. Find Original Trigger

Where did empty string come from?

const context = setupCoreTest(); // Returns { tempDir: '' }
Project.create('name', context.tempDir); // Accessed before beforeEach!

Adding Stack Traces

When can't trace manually, add instrumentation:

async function gitInit(directory: string) {
  const stack = new Error().stack;
  console.error('DEBUG git init:', {
    directory,
    cwd: process.cwd(),
    stack,
  });

  await execFileAsync('git', ['init'], { cwd: directory });
}

Critical: Use console.error() in tests (not logger - may not show)

Run and capture:

npm test 2>&1 | grep 'DEBUG git init'

Analyze stack traces:

• Look for test file names
• Find line number triggering call
• Identify pattern (same test? same parameter?)

Finding Which Test Causes Pollution

If something appears during tests but don't know which test:

Use bisection script: scripts/find-polluter.sh

./scripts/find-polluter.sh '.git' 'src/**/*.test.ts'

Runs tests one-by-one, stops at first polluter.

Key Principle

NEVER fix just where error appears. Trace back to find original trigger.

When found immediate cause:

• Can trace one level up? → Trace backwards
• Is this the source? → Fix at source
• Then add validation at each layer (see defense-in-depth.md)

Real Example

Symptom: .git created in packages/core/ (source code)

Trace chain: 1. git init runs in process.cwd() ← empty cwd parameter 2. WorktreeManager called with empty projectDir 3. Session.create() passed empty string 4. Test accessed context.tempDir before beforeEach 5. setupCoreTest() returns { tempDir: '' } initially

Root cause: Top-level variable initialization accessing empty value

Fix: Made tempDir a getter that throws if accessed before beforeEach

Also added defense-in-depth:

• Layer 1: Project.create() validates directory
• Layer 2: WorkspaceManager validates not empty
• Layer 3: NODE_ENV guard refuses git init outside tmpdir
• Layer 4: Stack trace logging before git init

Systematic Debugging

Four-phase debugging framework that ensures root cause investigation before attempting fixes.

The Iron Law

NO FIXES WITHOUT ROOT CAUSE INVESTIGATION FIRST

If haven't completed Phase 1, cannot propose fixes.

The Four Phases

Must complete each phase before proceeding to next.

Phase 1: Root Cause Investigation

BEFORE attempting ANY fix:

1. Read Error Messages Carefully - Don't skip past errors/warnings, read stack traces completely 2. Reproduce Consistently - Can trigger reliably? Exact steps? If not reproducible → gather more data 3. Check Recent Changes - What changed? Git diff, recent commits, new dependencies, config changes 4. Gather Evidence in Multi-Component Systems

• For EACH component boundary: log data entering/exiting, verify environment propagation
• Run once to gather evidence showing WHERE it breaks
• THEN analyze to identify failing component

5. Trace Data Flow - Where does bad value originate? Trace up call stack until finding source (see root-cause-tracing.md)

Phase 2: Pattern Analysis

Find pattern before fixing:

1. Find Working Examples - Locate similar working code in same codebase 2. Compare Against References - Read reference implementation COMPLETELY, understand fully before applying 3. Identify Differences - List every difference however small, don't assume "that can't matter" 4. Understand Dependencies - What other components, settings, config, environment needed?

Phase 3: Hypothesis and Testing

Scientific method:

1. Form Single Hypothesis - "I think X is root cause because Y", be specific not vague 2. Test Minimally - SMALLEST possible change to test hypothesis, one variable at a time 3. Verify Before Continuing - Worked? → Phase 4. Didn't work? → NEW hypothesis. DON'T add more fixes 4. When Don't Know - Say "I don't understand X", don't pretend, ask for help

Phase 4: Implementation

Fix root cause, not symptom:

1. Create Failing Test Case - Simplest reproduction, automated if possible, MUST have before fixing 2. Implement Single Fix - Address root cause identified, ONE change, no "while I'm here" improvements 3. Verify Fix - Test passes? No other tests broken? Issue actually resolved? 4. If Fix Doesn't Work

• STOP. Count: How many fixes tried?
• If < 3: Return to Phase 1, re-analyze with new information
• If ≥ 3: STOP and question architecture

5. If 3+ Fixes Failed: Question Architecture

• Pattern: Each fix reveals new shared state/coupling problem elsewhere
• STOP and question fundamentals: Is pattern sound? Wrong architecture?
• Discuss with human partner before more fixes

Red Flags - STOP and Follow Process

If catch yourself thinking:

• "Quick fix for now, investigate later"
• "Just try changing X and see if it works"
• "Add multiple changes, run tests"
• "Skip the test, I'll manually verify"
• "It's probably X, let me fix that"
• "I don't fully understand but this might work"
• "One more fix attempt" (when already tried 2+)

ALL mean: STOP. Return to Phase 1.

Human Partner Signals You're Doing It Wrong

• "Is that not happening?" - Assumed without verifying
• "Will it show us...?" - Should have added evidence gathering
• "Stop guessing" - Proposing fixes without understanding
• "Ultrathink this" - Question fundamentals, not just symptoms
• "We're stuck?" (frustrated) - Approach isn't working

When see these: STOP. Return to Phase 1.

Common Rationalizations

Real-World Impact

From debugging sessions:

• Systematic approach: 15-30 minutes to fix
• Random fixes approach: 2-3 hours of thrashing
• First-time fix rate: 95% vs 40%
• New bugs introduced: Near zero vs common

Testing Strategies for Refactoring

Comprehensive guide to testing approaches that enable safe refactoring with behavior preservation.

Table of Contents

• Testing Philosophy
• Characterization Tests
• Test-Driven Refactoring
• Snapshot Testing
• Test Coverage Strategies
• Regression Prevention
• Finding Specific Content

---

Testing Philosophy

Core Principle: Tests are your safety net for refactoring

Key Concepts:

• Tests document current behavior
• Fast, reliable tests enable confident changes
• Refactoring should never change observable behavior
• Tests run continuously during refactoring

Testing Pyramid for Refactoring:

        /\
       /E2E\       <- Few: Critical user journeys
      /------\
     /Integr.\    <- Some: Component interactions
    /----------\
   /Unit Tests \  <- Many: Pure functions, logic
  /--------------\

---

Characterization Tests

What Are Characterization Tests?

Definition: Tests that capture what code currently DOES, not what it SHOULD do

Purpose: Create safety net for legacy code without existing tests

Key Principle: "I don't know what this code should do, but I know what it does today"

When to Use

• Legacy code without tests
• Undocumented behavior
• Before refactoring unfamiliar code
• Complex algorithms to preserve
• Code with unclear requirements

How to Create

1. Identify Behavior to Preserve:

// Legacy function - what does it do?
function processPayment(amount, type, customer) {
  // ... 50 lines of complex logic ...
  return result;
}

2. Write Tests Capturing Current Behavior:

describe('processPayment - Characterization Tests', () => {
  test('basic credit card payment', () => {
    const result = processPayment(100, 'credit', { id: 1, tier: 'gold' });
    // Record what it ACTUALLY returns
    expect(result).toEqual({
      status: 'approved',
      amount: 100,
      fee: 2.9,
      total: 102.9
    });
  });

  test('gold tier gets discount', () => {
    const result = processPayment(100, 'credit', { id: 1, tier: 'gold' });
    expect(result.fee).toBe(2.4);  // Discovered behavior
  });

  test('handles negative amounts', () => {
    const result = processPayment(-50, 'credit', { id: 1 });
    // Even if wrong, capture current behavior
    expect(result.status).toBe('error');
  });
});

3. Run Tests to Confirm Current Behavior:

• Tests should pass immediately (describe reality)
• Adjust expectations if tests fail (discovering actual behavior)
• Don't fix bugs yet - just document them

4. Now Safe to Refactor:

• Tests prove behavior unchanged
• Can improve structure without fear
• Fix bugs in separate, explicit change

Characterization Testing Tools

Jest (JavaScript/TypeScript):

// Generate tests from execution
test('characterize getUserDetails', () => {
  const result = getUserDetails(123);
  expect(result).toMatchSnapshot();
});

ApprovalTests (Multiple Languages):

from approvaltests import verify

def test_process_order():
    result = process_order(sample_order)
    verify(result)  # Stores result as approved file

Example-Based Testing:

// Record inputs and outputs
const characterizationSuite = [
  { input: [100, 'credit'], output: { status: 'approved', amount: 100 } },
  { input: [0, 'debit'], output: { status: 'rejected', reason: 'invalid' } },
  // ... more cases discovered through exploration
];

characterizationSuite.forEach(({ input, output }) => {
  test(`handles ${JSON.stringify(input)}`, () => {
    expect(legacyFunction(...input)).toEqual(output);
  });
});

Best Practices

✅ Do:

• Test actual behavior, not desired behavior
• Capture edge cases and corner cases
• Include "wrong" behavior (to be fixed later)
• Make tests comprehensive before refactoring
• Run tests frequently during refactoring

❌ Don't:

• Fix bugs while creating tests
• Assume behavior - verify it
• Write tests for how code should work
• Skip edge cases

---

Test-Driven Refactoring

Red-Green-Refactor Cycle

Core TDD Loop:

1. RED: Write failing test
    ↓
2. GREEN: Make it pass (quickly)
    ↓
3. REFACTOR: Improve design
    ↓
   (Repeat)

Applying to Refactoring

When You Have Tests: 1. Ensure tests pass (GREEN) 2. Refactor code 3. Ensure tests still pass (stay GREEN) 4. Commit

When Adding Features: 1. RED: Write test for new feature 2. GREEN: Implement minimally 3. REFACTOR: Improve design 4. Commit

Example: TDD Refactoring Session

Starting Point:

// Ugly but working code
function calc(a, b, op) {
  if (op == '+') return a + b;
  if (op == '-') return a - b;
  if (op == '*') return a * b;
  if (op == '/') return a / b;
}

// Tests pass
test('calculator works', () => {
  expect(calc(5, 3, '+')).toBe(8);
  expect(calc(5, 3, '-')).toBe(2);
  expect(calc(5, 3, '*')).toBe(15);
  expect(calc(5, 3, '/')).toBe(1.666...);
});

Refactoring Steps (staying green):

Step 1: Extract strategy map

const operations = {
  '+': (a, b) => a + b,
  '-': (a, b) => a - b,
  '*': (a, b) => a * b,
  '/': (a, b) => a / b
};

function calc(a, b, op) {
  return operations[op](a, b);
}

// Run tests: ✓ Still green

Step 2: Add validation

function calc(a, b, op) {
  if (!operations[op]) {
    throw new Error(`Unknown operation: ${op}`);
  }
  return operations[op](a, b);
}

// Add test for new behavior (RED)
test('throws on invalid operation', () => {
  expect(() => calc(5, 3, '%')).toThrow('Unknown operation');
});

// Run tests: ✓ Green again

TDD Best Practices for Refactoring

Keep Tests Passing:

• Make one small change
• Run tests
• If red, revert or fix immediately
• Never accumulate failing tests

Test Behavior, Not Implementation:

// ❌ Bad: Tests implementation
test('uses addition function', () => {
  expect(calc.operations['+']).toBeDefined();
});

// ✅ Good: Tests behavior
test('adds numbers', () => {
  expect(calc(2, 3, '+')).toBe(5);
});

Commit Frequently:

• After each green refactoring
• Small, reversible steps
• Git history shows refactoring journey

---

Snapshot Testing

What is Snapshot Testing?

Definition: Capture output once, compare against it in future runs

Use Cases:

• Complex output structures
• UI components
• Data transformations
• API responses

How It Works

First Run:

test('renders user profile', () => {
  const output = renderProfile({ name: 'Alice', age: 30 });
  expect(output).toMatchSnapshot();
});

// Creates __snapshots__/test.spec.js.snap:
exports[`renders user profile 1`] = `
"<div class='profile'>
  <h1>Alice</h1>
  <p>Age: 30</p>
</div>"
`;

Subsequent Runs:

• Compares output to stored snapshot
• Fails if output differs
• Developer reviews: intentional change or bug?

Best Practices

✅ Good Uses:

• Complex data structures
• React/Vue component output
• Generated HTML/XML
• Configuration objects

❌ Avoid:

• Data with timestamps/random values
• Tests that should be more specific
• Excessively large snapshots
• Binary or opaque data

Handling Changes:

# Review changes
npm test -- -u  # Update all snapshots

# Or selectively in test runner
# i - Update this snapshot
# u - Update all failing snapshots
# s - Skip this test

---

Test Coverage Strategies

Coverage Metrics

Types of Coverage:

• Line Coverage: % of lines executed
• Branch Coverage: % of if/else paths taken
• Function Coverage: % of functions called
• Statement Coverage: % of statements executed

Target: >80% for code being refactored

Measuring Coverage

Jest:

npm test -- --coverage

# Output:
File      | % Stmts | % Branch | % Funcs | % Lines
----------|---------|----------|---------|--------
utils.js  |   85.71 |    75.00 |  100.00 |   85.71

Istanbul (nyc):

nyc mocha tests/

# HTML report
nyc --reporter=html mocha tests/
open coverage/index.html

Prioritizing Coverage

1. High-Risk Areas First:

• Complex algorithms
• Business-critical logic
• Frequently changed code
• Bug-prone areas

2. Before Refactoring:

• Add tests if coverage <80%
• Focus on public API
• Test edge cases and error paths

3. During Refactoring:

• Coverage should not decrease
• May need new tests for extracted functions
• Delete tests for removed code

---

Regression Prevention

Regression Testing Strategy

Definition: Tests ensuring old bugs don't reappear

Process: 1. Bug discovered 2. Write test reproducing bug (RED) 3. Fix bug (GREEN) 4. Test now prevents regression 5. Commit test + fix together

Example:

// Bug report: "Calculator divides by zero without error"

// 1. Write failing test
test('throws on division by zero', () => {
  expect(() => calc(5, 0, '/')).toThrow('Division by zero');
});

// 2. Fix the bug
function calc(a, b, op) {
  if (op === '/' && b === 0) {
    throw new Error('Division by zero');
  }
  return operations[op](a, b);
}

// 3. Test now passes and prevents regression

Test Suites for Refactoring

Unit Tests:

• Pure functions
• Business logic
• Algorithms
• Coverage: 80-100%

Integration Tests:

• Component interactions
• Database operations
• API calls
• Coverage: 60-80%

End-to-End Tests:

• Critical user journeys
• Complete workflows
• Coverage: 20-30% (expensive, slow)

Test Organization

tests/
├── unit/
│   ├── utils.test.js
│   ├── models.test.js
│   └── services.test.js
├── integration/
│   ├── api.test.js
│   └── database.test.js
├── e2e/
│   └── checkout.test.js
├── characterization/
│   └── legacy.test.js
└── snapshots/
    └── components.test.js.snap

---

Finding Specific Content

Search patterns for quick reference:

# Find characterization test info
grep -i "characterization" references/testing-strategies.md

# Find TDD cycle
grep -i "red-green-refactor" references/testing-strategies.md

# Find snapshot testing
grep -i "snapshot" references/testing-strategies.md

# Find coverage strategies
grep -i "coverage" references/testing-strategies.md

---

Sources:

• Working Effectively with Legacy Code (Michael Feathers)
• Test-Driven Development by Example (Kent Beck)
• Industry best practices (2024-2025)

Verification Before Completion

Overview

Claiming work is complete without verification is dishonesty, not efficiency.

Core principle: Evidence before claims, always.

Violating the letter of this rule is violating the spirit of this rule.

The Iron Law

NO COMPLETION CLAIMS WITHOUT FRESH VERIFICATION EVIDENCE

If you haven't run the verification command in this message, you cannot claim it passes.

The Gate Function

BEFORE claiming any status or expressing satisfaction:

1. IDENTIFY: What command proves this claim?
2. RUN: Execute the FULL command (fresh, complete)
3. READ: Full output, check exit code, count failures
4. VERIFY: Does output confirm the claim?
   - If NO: State actual status with evidence
   - If YES: State claim WITH evidence
5. ONLY THEN: Make the claim

Skip any step = lying, not verifying

Common Failures

Red Flags - STOP

• Using "should", "probably", "seems to"
• Expressing satisfaction before verification ("Great!", "Perfect!", "Done!", etc.)
• About to commit/push/PR without verification
• Trusting agent success reports
• Relying on partial verification
• Thinking "just this once"
• Tired and wanting work over
• ANY wording implying success without having run verification

Rationalization Prevention

Key Patterns

Tests:

✅ [Run test command] [See: 34/34 pass] "All tests pass"
❌ "Should pass now" / "Looks correct"

Regression tests (TDD Red-Green):

✅ Write → Run (pass) → Revert fix → Run (MUST FAIL) → Restore → Run (pass)
❌ "I've written a regression test" (without red-green verification)

Build:

✅ [Run build] [See: exit 0] "Build passes"
❌ "Linter passed" (linter doesn't check compilation)

Requirements:

✅ Re-read plan → Create checklist → Verify each → Report gaps or completion
❌ "Tests pass, phase complete"

Agent delegation:

✅ Agent reports success → Check VCS diff → Verify changes → Report actual state
❌ Trust agent report

Why This Matters

From 24 failure memories:

• your human partner said "I don't believe you" - trust broken
• Undefined functions shipped - would crash
• Missing requirements shipped - incomplete features
• Time wasted on false completion → redirect → rework
• Violates: "Honesty is a core value. If you lie, you'll be replaced."

When To Apply

ALWAYS before:

• ANY variation of success/completion claims
• ANY expression of satisfaction
• ANY positive statement about work state
• Committing, PR creation, task completion
• Moving to next task
• Delegating to agents

Rule applies to:

• Exact phrases
• Paraphrases and synonyms
• Implications of success
• ANY communication suggesting completion/correctness

The Bottom Line

No shortcuts for verification.

Run the command. Read the output. THEN claim the result.

This is non-negotiable.

Verification Before Completion

Run verification commands and confirm output before claiming success.

Core Principle

Evidence before claims, always.

Claiming work complete without verification is dishonesty, not efficiency.

The Iron Law

NO COMPLETION CLAIMS WITHOUT FRESH VERIFICATION EVIDENCE

If haven't run verification command in this message, cannot claim it passes.

The Gate Function

BEFORE claiming any status or expressing satisfaction:

1. IDENTIFY: What command proves this claim?
2. RUN: Execute FULL command (fresh, complete)
3. READ: Full output, check exit code, count failures
4. VERIFY: Does output confirm claim?
   - If NO: State actual status with evidence
   - If YES: State claim WITH evidence
5. ONLY THEN: Make claim

Skip any step = lying, not verifying

Common Failures

Red Flags - STOP

• Using "should", "probably", "seems to"
• Expressing satisfaction before verification ("Great!", "Perfect!", "Done!")
• About to commit/push/PR without verification
• Trusting agent success reports
• Relying on partial verification
• Thinking "just this once"
• Tired and wanting work over
• ANY wording implying success without having run verification

Rationalization Prevention

Key Patterns

Tests:

✅ [Run test command] [See: 34/34 pass] "All tests pass"
❌ "Should pass now" / "Looks correct"

Regression tests (TDD Red-Green):

✅ Write → Run (pass) → Revert fix → Run (MUST FAIL) → Restore → Run (pass)
❌ "I've written regression test" (without red-green verification)

Build:

✅ [Run build] [See: exit 0] "Build passes"
❌ "Linter passed" (linter doesn't check compilation)

Requirements:

✅ Re-read plan → Create checklist → Verify each → Report gaps or completion
❌ "Tests pass, phase complete"

Agent delegation:

✅ Agent reports success → Check VCS diff → Verify changes → Report actual state
❌ Trust agent report

When To Apply

ALWAYS before:

• ANY variation of success/completion claims
• ANY expression of satisfaction
• ANY positive statement about work state
• Committing, PR creation, task completion
• Moving to next task
• Delegating to agents

Rule applies to:

• Exact phrases
• Paraphrases and synonyms
• Implications of success
• ANY communication suggesting completion/correctness

The Bottom Line

No shortcuts for verification.

Run command. Read output. THEN claim result.

Non-negotiable.

find-polluter.sh Test Documentation

Purpose

Bisection script to find which test creates unwanted files or state pollution.

Manual Test Procedure

Setup Test Scenario

# Create test directory
mkdir -p /tmp/polluter-test && cd /tmp/polluter-test

# Create clean test
cat > test1.test.js << 'EOF'
console.log('Test 1: clean');
EOF

# Create polluter test
cat > test2.test.js << 'EOF'
const fs = require('fs');
fs.mkdirSync('.git', { recursive: true });
console.log('Test 2: creates pollution');
EOF

# Create another clean test
cat > test3.test.js << 'EOF'
console.log('Test 3: clean');
EOF

Run Script

# For projects with npm test
/path/to/find-polluter.sh '.git' 'src/**/*.test.ts'

# For node-only tests (modify script to use 'node' instead of 'npm test')
./find-polluter.sh '.git' '*.test.js'

Expected Output

🔍 Searching for test that creates: .git
Test pattern: *.test.js

Found 3 test files

[1/3] Testing: ./test1.test.js
[2/3] Testing: ./test2.test.js

🎯 FOUND POLLUTER!
   Test: ./test2.test.js
   Created: .git

Cleanup

rm -rf /tmp/polluter-test

Test Results

✅ Script logic verified (2025-11-11)

• Correctly iterates through test files
• Detects pollution creation
• Reports the polluting test file
• Exits early when polluter found

Usage Notes

Prerequisites:

• Test runner (npm test) must be configured in project
• Test pattern must match actual test files
• Pollution path must be accurate

Customization: If your project doesn't use npm test, modify line 42:

# Replace
npm test "$TEST_FILE" > /dev/null 2>&1 || true

# With your test command
node "$TEST_FILE" > /dev/null 2>&1 || true
# Or
jest "$TEST_FILE" > /dev/null 2>&1 || true

Common Use Cases

1. Find test creating .git directory:

   ./find-polluter.sh '.git' 'src/**/*.test.ts'

2. Find test creating node_modules:

   ./find-polluter.sh 'node_modules' 'test/**/*.spec.js'

3. Find test creating specific file:

   ./find-polluter.sh 'unwanted-file.txt' '**/*.test.js'