Chapter 13: Worked Example: Refactoring a Messy Function to Standard

Why this matters

Every principle in this book has been demonstrated in isolation. Real code violates several standards simultaneously, and refactoring it requires applying multiple chapters' worth of judgment together, in the right order, without losing sight of the original behavior. This chapter works through one realistically messy function end to end, exactly the kind of exercise a coding-standards-focused interview or code review commonly presents.

The starting point: a function violating nearly every standard in this book

// process order and stuff
function po(o: any, d: any) {
  var t = 0;
  for (var i = 0; i < o.items.length; i++) {
    t = t + o.items[i].price * o.items[i].qty;
  }
  if (d == true) {
    t = t * 0.9;
  }
  try {
    db.save({id: o.id, total: t});
  } catch (e) {}
  return t;
}

Step 1: fix naming (Chapter 2)

function processOrder(order: any, applyDiscount: any) {
  var total = 0;
  for (var i = 0; i < order.items.length; i++) {
    total = total + order.items[i].price * order.items[i].qty;
  }
  if (applyDiscount == true) {
    total = total * 0.9;
  }
  try {
    db.save({id: order.id, total: total});
  } catch (e) {}
  return total;
}

Step 2: fix types, replacing any (Chapter 9)

interface OrderItem {
  price: number;
  qty: number;
}
interface Order {
  id: string;
  items: OrderItem[];
}

function processOrder(order: Order, applyDiscount: boolean): number {
  var total = 0;
  for (var i = 0; i < order.items.length; i++) {
    total = total + order.items[i].price * order.items[i].qty;
  }
  if (applyDiscount == true) {
    total = total * 0.9;
  }
  try {
    db.save({id: order.id, total: total});
  } catch (e) {}
  return total;
}

Step 3: fix error handling — the empty catch block (Chapter 5)

function processOrder(order: Order, applyDiscount: boolean): number {
  var total = 0;
  for (var i = 0; i < order.items.length; i++) {
    total = total + order.items[i].price * order.items[i].qty;
  }
  if (applyDiscount == true) {
    total = total * 0.9;
  }
  try {
    db.save({id: order.id, total: total});
  } catch (e) {
    logger.error("Failed to save order total", { orderId: order.id, error: e });
    throw e; // don't silently swallow -- callers need to know this failed
  }
  return total;
}

Step 4: separate pure logic from the side effect (Chapter 7's testability principle)

// Pure, trivially testable -- no mocking needed to verify this logic:
function calculateOrderTotal(order: Order, applyDiscount: boolean): number {
  const rawTotal = order.items.reduce((sum, item) => sum + item.price * item.qty, 0);
  return applyDiscount ? rawTotal * 0.9 : rawTotal;
}

// Thin wrapper handling the I/O side effect, now separated from the calculation:
function processOrder(order: Order, applyDiscount: boolean, db: Database): number {
  const total = calculateOrderTotal(order, applyDiscount);
  try {
    db.save({ id: order.id, total });
  } catch (e) {
    logger.error("Failed to save order total", { orderId: order.id, error: e });
    throw e;
  }
  return total;
}

Step 5: modernize idioms and clean up remaining style issues (Chapters 3, 9)

interface OrderItem {
  price: number;
  qty: number;
}
interface Order {
  id: string;
  items: OrderItem[];
}
interface Database {
  save(record: { id: string; total: number }): void;
}

const LOYALTY_DISCOUNT_MULTIPLIER = 0.9;

function calculateOrderTotal(order: Order, shouldApplyDiscount: boolean): number {
  const rawTotal = order.items.reduce((sum, item) => sum + item.price * item.qty, 0);
  return shouldApplyDiscount ? rawTotal * LOYALTY_DISCOUNT_MULTIPLIER : rawTotal;
}

function processOrder(order: Order, shouldApplyDiscount: boolean, db: Database): number {
  const total = calculateOrderTotal(order, shouldApplyDiscount);
  try {
    db.save({ id: order.id, total });
  } catch (error) {
    logger.error("Failed to save order total", { orderId: order.id, error });
    throw error;
  }
  return total;
}

The final result is now testable with zero mocking for the actual business logic:

// Testing the pure calculation requires no database, no mocking, nothing but a call:
expect(calculateOrderTotal({ id: "1", items: [{ price: 100, qty: 2 }] }, false)).toBe(200);
expect(calculateOrderTotal({ id: "1", items: [{ price: 100, qty: 2 }] }, true)).toBe(180);

The narration a strong candidate provides throughout

[SENIOR INSIGHT] The specific order of fixes matters for communicating judgment clearly: naming and typing first (cheap, mechanical, immediately clarifies intent), then error handling (a correctness/reliability fix), then structural separation (a design improvement enabling testability), then final idiom polish. Presenting fixes in this order — cheap and mechanical first, structural and judgment-heavy last — mirrors how a real refactor should be sequenced and reviewed incrementally (small, atomic commits, per Chapter 8), rather than attempting one enormous, all-at-once rewrite that's hard to review and risky to land.

Common mistakes

1. Fixing everything in one giant, tangled rewrite rather than a sequence of small, individually-reviewable, individually-revertible steps (directly violating Chapter 8's atomic commit guidance, even within a single refactoring exercise).
2. Missing the empty catch block — the single most damaging issue in the original function, and one that's easy to overlook when focused on more visible issues like naming and typing.
3. Stopping at surface-level fixes (naming, types) without addressing the deeper structural issue (mixed pure logic and I/O) that actually determines whether the function is testable and maintainable long-term.
4. Not explaining the reasoning behind each fix — presenting a diff without narrating which standard each change addresses and why loses most of the signal a reviewer or interviewer is actually looking for.

Discussion / interview questions

1. "Given this messy function, walk through how you'd refactor it to meet good coding standards." Strong answer: Works through fixes in a sensible order (naming/typing first, error handling next, structural separation for testability last), explicitly naming which standard/principle each fix addresses, rather than presenting a single, unexplained final rewrite.

2. "What's the single most damaging issue in the original function, and why?" Strong answer: The empty catch block — it silently discards information about a failed database save, meaning a production failure here would leave zero trace, exactly Chapter 5's "cardinal sin" of error handling.

3. "Why is separating calculateOrderTotal from processOrder valuable beyond just 'cleaner code'?" Strong answer: It makes the actual business logic (the calculation) testable with zero mocking, since it's now a pure function with no I/O dependency — directly Chapter 7's testability principle, and a concrete example of how a structural fix (separating concerns) produces a testability payoff, not just an aesthetic one.

4. "If you were reviewing this refactor as a single, large pull request, what would you suggest instead?" Strong answer: Suggest splitting it into the sequence of smaller, atomic commits/PRs shown in this chapter's steps — each independently reviewable and revertible — rather than reviewing one large, tangled diff that mixes several unrelated categories of fix together (Chapter 8's PR-sizing guidance).

Chapter summary / cheat sheet

• A realistic refactor applies multiple standards together: naming (Ch. 2) → types (Ch. 9) → error handling (Ch. 5) → structural separation for testability (Ch. 7) → idiom polish (Ch. 3, 9).
• Sequence fixes from cheap/mechanical to structural/judgment-heavy, and land them as separate, atomic, reviewable steps (Ch. 8) rather than one large rewrite.
• The empty catch block is the single most damaging issue in the original example — don't let more visible issues (naming, typing) distract from finding it.
• Separating pure logic from side effects isn't just cleaner — it's the specific structural change that makes the core logic testable without any mocking at all.

---