Safe refactoring: characterization tests and commits that separate behavior changes Intermediate

This page answers: where the running example’s “before” code goes wrong, and how to refactor it while proving behavior doesn’t change.

Running example: CheckoutService “before”

The kind of code you meet in real reviews: it works, it has an e2e test for the happy path, and it has already caused incidents.

@Injectable()
export class CheckoutService {
  constructor(private readonly db: Pool) {}

  async checkout(userId: string, items: any[], coupon: string | null, express: boolean, dryRun: boolean) {
    if (items && items.length > 0) {
      const res = await this.db.query('SELECT id, email FROM users WHERE id = $1', [userId]);
      const user = res.rows[0];
      if (user) {
        let total = 0;
        for (const i of items) total += i.price * i.qty;
        if (coupon) {
          if (coupon === 'VIP10' && new Date().getHours() < 12) total = total * 0.9;
        }
        if (total > 500000) {
          // free ship
        } else total += express ? 50000 : 30000;
        if (!dryRun) {
          try {
            await fetch('https://pay.example.com/charges', {
              method: 'POST',
              body: JSON.stringify({ userId, total }),
            });
          } catch (e) {
            console.log(e);
          }
          // await this.mailer.sendReceipt(user.email, total);
          await this.db.query('INSERT INTO orders (user_id, total) VALUES ($1, $2)', [userId, total]);
        }
        return total;
      } else {
        throw new Error('error');
      }
    }
    return 0;
  }
}

Reading it like a reviewer, you can count at least seven problems:

#	Where	Real consequence
1	catch (e) { console.log(e) } then INSERT anyway	Network loss to the payment gateway → order saved without collecting payment
2	No response.ok check	fetch() only rejects on network errors; HTTP 402/500 still resolve → the order is saved too
3	new Date().getHours()	Hour in the process’s timezone; a container running in UTC shifts “before noon” by 7 hours versus Vietnam time
4	total * 0.9	Produces fractional amounts like 587293.2000000001 VND (see the demo below)
5	express, dryRun	checkout(id, items, null, true, false): nobody can read the call site
6	500000, 50000, 30000	Marketing changing the free-shipping threshold means grepping for numbers
7	throw new Error('error'), return 0	The client can’t tell a missing user, an empty cart and a system error apart

Each of these has its own BAD/GOOD pair on the Common mistakes page.

Rule number one: refactoring doesn’t change behavior

• Refactoring changes structure without changing behavior.
• Fixing a bug changes behavior.
• When both land in one commit, reviewers can’t tell which diff is intentional, and git revert can’t undo one without the other.

Loading diagram…

1. Write characterization tests first. They pin the current behavior, including wrong behavior (fractional VND). To make it testable, create a small seam: pass now in instead of calling new Date() inside.
2. Every refactor commit must keep tests green without changing the tests.
3. Behavior-fixing commits come after, change tests deliberately, and say fix: plus the reason in the message.

Demo: a golden master catches a refactor that “looks equivalent”

The examples/src/clean-code/ folder contains:

• legacy-pricing.ts: the pricing part of the old code.
• pricing.ts: the refactored version.
• characterization.test.ts: generates 10,000 random carts with a fixed seed and compares both outputs. It adds a “harmless” variant that changes total * 0.9 to total * 90 / 100.

npx --prefix examples ts-node examples/src/clean-code/characterization.test.ts

Real output:

quoteTotalVnd: 10000/10000 cases match the legacy code
quoteTotalVndPercent: 201 cases differ; first example: legacy=587293.2000000001 percent=587293.2

Two lessons from these two lines:

• A “mathematically equivalent” formula change still changes floating-point results. Without a golden master it slips through review.
• The old code returns 587293.2000000001 VND. That’s a bug, but fixing it (rounding) is commit 5, not part of the refactor.

When to apply / exceptions

Situation	What to do
Long-lived, frequently changed code (domain logic, use cases, integration adapters)	Apply: this is where reading/changing costs accumulate
About to change a piece of code	Refactor that piece in a separate commit, before adding the feature
Want to open a “clean up the whole module” PR	No, not without a business reason
One-off scripts, spikes, one-time migrations	Exception, not needed
A hot path measured to be slow	Prioritize performance, comment the reason
Code with no tests and behavior you don’t fully understand	Write characterization tests first, refactor nothing yet

Self-check

1. Is changing total * 0.9 to total * 90 / 100 a pure refactor? How does the demo above answer that?
2. Why does the fetch() in the “before” code still lead to saving the order even when the payment gateway returns HTTP 500?

Next: Functions and side effects