Refactoring safely

Theo's daily report function was one of the oldest pieces of code at Ridgeline. It had grown a branch at a time for three years: cafe sales, then online sales, then shipping, then a special case for gift cards that nobody remembered adding. It worked, more or less, but every change to it took an afternoon, and twice a small change had broken something unrelated. Rosa asked whether it should be rewritten from scratch. Jordan suggested something less risky: improve it in small, safe steps, checking after every step that it still produced exactly the same reports.

Changing the structure of code without changing what it does is called refactoring. The term was popularized by Martin Fowler's book Refactoring, first published in 1999, which listed dozens of small, named transformations. This lesson explains why refactoring matters, how to make it safe with tests, including characterization tests for code that has none, the most useful refactorings and the code smells that call for them, and why small steps beat big rewrites.

Why refactor?

Code that works but is hard to understand is expensive. Every change takes longer, because the developer must first work out what the code does. Mistakes are more likely, because tangled code hides the consequences of a change. And developers avoid touching it, so it gets worse as workarounds pile up around it. Refactoring keeps code easy to change. It is not a separate project scheduled once a year but a habit: tidy the area you are about to work in, then make your change in the tidied code. Kent Beck's advice sums it up: make the change easy, then make the easy change.

Refactoring changes structure only. If a step changes behavior, even to fix a bug you noticed along the way, it is not a refactoring, and it should be a separate commit with its own test. Keeping the two kinds of change apart is what makes each one safe to review and easy to undo.

Tests are the safety net

How do you know a refactoring did not change behavior? By running tests before and after every step. If the tests passed before and still pass after, the behavior they cover is unchanged. This is why the TDD cycle puts refactoring after green, and why code without tests is so hard to improve: without a safety net, every structural change is a gamble.

Theo's report function had no tests at all, so Jordan's first step was to write some. But what should they say? Nobody knew exactly what the function was supposed to do in every case; its behavior was the specification. The answer is a characterization test: a test that records what the code currently does, right or wrong, so that any change in behavior is noticed. You run the existing code on a range of representative inputs, save the outputs, and write tests asserting those exact outputs.

import unittest

def report(*cases):
    """Run TestCase classes and print one line per test (a small test runner)."""
    loader = unittest.TestLoader()
    tests = [t for case in cases for t in loader.loadTestsFromTestCase(case)]
    result = unittest.TestResult()
    unittest.TestSuite(tests).run(result)
    problems = {}
    for kind, items in (("FAIL", result.failures), ("ERROR", result.errors)):
        for test, tb in items:
            problems.setdefault(test.id().split(" ")[0], []).append((kind, tb.strip().splitlines()[-1]))
    for t in tests:
        found = problems.get(t.id(), [])
        print(f"{t.id().rsplit('.', 1)[-1]:<34} {found[0][0] if found else 'ok'}")
        for kind, line in found:
            print("    " + line)
    print(f"{len(tests)} tests: {len(tests) - len(problems)} passed, {len(problems)} failed")
def daily_report(sales, kind):
    # sales: list of (sku, qty, cents); kind: "cafe" or "shop"
    t = 0
    lines = []
    for s in sales:
        if kind == "cafe":
            amt = s[1] * s[2]
            amt = amt + round(amt * 0.05)
            t = t + amt
            lines.append(s[0] + " x" + str(s[1]) + " " + str(amt))
        else:
            amt = s[1] * s[2]
            if amt >= 5000:
                ship = 0
            else:
                ship = 795
            amt = amt + round(amt * 0.05) + ship
            t = t + amt
            lines.append(s[0] + " x" + str(s[1]) + " " + str(amt))
    lines.append("TOTAL " + str(t))
    return "\n".join(lines)

SALES = [("RC-101", 2, 1850), ("RC-104", 1, 2025)]
BIG = [("RC-102", 3, 2200)]

class TestDailyReportCharacterization(unittest.TestCase):
    """Pins down what daily_report does TODAY, before any refactoring."""
    def test_cafe(self):
        self.assertEqual(daily_report(SALES, "cafe"), "RC-101 x2 3885\nRC-104 x1 2126\nTOTAL 6011")

    def test_shop_small_order_pays_shipping(self):
        self.assertEqual(daily_report(SALES, "shop"), "RC-101 x2 4680\nRC-104 x1 2921\nTOTAL 7601")

    def test_shop_big_order_ships_free(self):
        self.assertEqual(daily_report(BIG, "shop"), "RC-102 x3 6930\nTOTAL 6930")

    def test_no_sales(self):
        self.assertEqual(daily_report([], "cafe"), "TOTAL 0")

report(TestDailyReportCharacterization)

test_cafe                          ok
test_no_sales                      ok
test_shop_big_order_ships_free     ok
test_shop_small_order_pays_shipping ok
4 tests: 4 passed, 0 failed

Where did the expected strings come from? From running the old function, which is exactly what lesson 1 said not to do for ordinary tests. Characterization tests are the exception, because their purpose is different: not to prove the code right, but to detect any change. If the old code has a bug, the characterization test pins the bug too, and that is fine for now. When Jordan later fixes a bug deliberately, the matching test is updated in the same commit, with a note explaining the change.

For code with many possible outputs, such as a report generator, the same idea is sometimes called a golden master or snapshot test: run the code on a fixed set of inputs, save the whole output to a file, and compare every future run against that file. pytest has plugins that manage snapshot files automatically.

Small steps

With the safety net in place, Jordan refactored in steps of a few minutes each, running the tests after every one and committing whenever they were green. If a step broke a test, the change was small enough to see the problem at once, or to undo it with a single git restore. This rhythm feels slow at first, but it is faster than a big rewrite, because there are never long stretches of broken code and never a day spent hunting for which of forty changes broke the report.

Common refactorings

Most refactoring is done with a small set of named moves. Knowing their names helps teams talk about changes, and modern editors can perform several of them automatically, which is safer than editing by hand.

The signs that code needs one of these moves are called code smells, another term from Fowler and Beck: a long function, duplicated code, a long parameter list, magic numbers, deep nesting, comments that explain what confusing code does, and names that need decoding. A smell is not always a problem, but it is always worth a second look.

Refactoring the report, step by step

Here is the daily report after Jordan's steps. The behavior is identical, so the characterization tests from earlier still pass unchanged, but the structure is new. The tax and shipping rules are named constants and small functions, the duplicated branch has gone, the tuple fields have names, and the string building is readable. Compare it with the original listing above.

import unittest

def report(*cases):
    """Run TestCase classes and print one line per test (a small test runner)."""
    loader = unittest.TestLoader()
    tests = [t for case in cases for t in loader.loadTestsFromTestCase(case)]
    result = unittest.TestResult()
    unittest.TestSuite(tests).run(result)
    problems = {}
    for kind, items in (("FAIL", result.failures), ("ERROR", result.errors)):
        for test, tb in items:
            problems.setdefault(test.id().split(" ")[0], []).append((kind, tb.strip().splitlines()[-1]))
    for t in tests:
        found = problems.get(t.id(), [])
        print(f"{t.id().rsplit('.', 1)[-1]:<34} {found[0][0] if found else 'ok'}")
        for kind, line in found:
            print("    " + line)
    print(f"{len(tests)} tests: {len(tests) - len(problems)} passed, {len(problems)} failed")
from typing import NamedTuple

TAX_RATE = 0.05
FREE_SHIPPING_FROM = 5000      # cents
SHIPPING = 795                 # cents

class Sale(NamedTuple):
    sku: str
    qty: int
    cents: int

def with_tax(cents):
    return cents + round(cents * TAX_RATE)

def shipping_for(cents):
    return 0 if cents >= FREE_SHIPPING_FROM else SHIPPING

def line_amount(sale, kind):
    goods = sale.qty * sale.cents
    amount = with_tax(goods)
    if kind == "shop":
        amount += shipping_for(goods)
    return amount

def daily_report(sales, kind):
    """One line per sale with its amount in cents, then the day's total."""
    lines, total = [], 0
    for sale in map(Sale._make, sales):
        amount = line_amount(sale, kind)
        total += amount
        lines.append(f"{sale.sku} x{sale.qty} {amount}")
    lines.append(f"TOTAL {total}")
    return "\n".join(lines)

SALES = [("RC-101", 2, 1850), ("RC-104", 1, 2025)]
BIG = [("RC-102", 3, 2200)]

class TestDailyReportCharacterization(unittest.TestCase):
    def test_cafe(self):
        self.assertEqual(daily_report(SALES, "cafe"), "RC-101 x2 3885\nRC-104 x1 2126\nTOTAL 6011")

    def test_shop_small_order_pays_shipping(self):
        self.assertEqual(daily_report(SALES, "shop"), "RC-101 x2 4680\nRC-104 x1 2921\nTOTAL 7601")

    def test_shop_big_order_ships_free(self):
        self.assertEqual(daily_report(BIG, "shop"), "RC-102 x3 6930\nTOTAL 6930")

    def test_no_sales(self):
        self.assertEqual(daily_report([], "cafe"), "TOTAL 0")

report(TestDailyReportCharacterization)

test_cafe                          ok
test_no_sales                      ok
test_shop_big_order_ships_free     ok
test_shop_small_order_pays_shipping ok
4 tests: 4 passed, 0 failed

Reading the new version, one oddity jumps out that was invisible in the old one: shipping is decided per sale line rather than per order, so a customer who buys two bags on separate lines pays shipping twice. That is probably a bug. Because it is behavior, not structure, it does not belong in the refactoring. Jordan opened issue #45 for Theo to confirm the rule, to be fixed later as a separate, tested change. Making code clearer often reveals bugs like this, which is one more reason to refactor.

def shipping_band(subtotal_cents):
    # before: nested conditions, hard to follow
    if subtotal_cents > 0:
        if subtotal_cents < 2500:
            return 1295
        else:
            if subtotal_cents < 5000:
                return 795
            else:
                return 0
    else:
        raise ValueError("empty order")

def shipping_band_v2(subtotal_cents):
    # after: guard clause first, then a flat sequence of cases
    if subtotal_cents <= 0:
        raise ValueError("empty order")
    if subtotal_cents < 2500:
        return 1295
    if subtotal_cents < 5000:
        return 795
    return 0

print(all(shipping_band(c) == shipping_band_v2(c) for c in range(1, 8000)))

True

The last line of the listing is a handy trick while refactoring a small pure function: keep the old version briefly, compare the two over many inputs, and delete the old one once you are sure. It is a characterization test in one line.

Refactoring versus rewriting

Rewriting a working system from scratch is tempting, because the old code is ugly and a fresh start feels clean. It is also one of the riskiest things a team can do. The old code contains years of fixes for situations nobody remembers, and a rewrite has to rediscover them all, usually through bugs in production. Meanwhile the old system must be maintained in parallel, and nothing useful ships until the rewrite is complete. Refactoring in small steps keeps the system working and shippable the whole time, and lets the team stop at any point when the code is good enough.

There are times when replacing a component is right, such as when the underlying technology is being retired. Even then, the safer path is usually to replace it piece by piece behind a stable interface, routing a little more work to the new code each week, a technique Fowler calls the strangler fig pattern, after the vine that slowly grows around and replaces a tree.

def receipt_lines(drinks, beans, merch):
    out = []
    for d in drinks:
        out.append(d + round(d * 0.05))
    for b in beans:
        out.append(b + round(b * 0.05))
    for m in merch:
        out.append(m + round(m * 0.05))
    return out

def roast_minutes(level):
    if level == 'light':
        return 10
    elif level == 'medium':
        return 12
    elif level == 'medium-dark':
        return 13
    elif level == 'dark':
        return 15
    raise ValueError(level)

EXPECTED = {
    ('RC-101', 42): 'RC-101: 42 bags',
}

for (code, bags), label in EXPECTED.items():
    print(code, bags, '->', legacy_label(code, bags), '| expected', label)