Refactoring in Safe Steps

Refactoring changes the structure of code without changing what it does, and the characterization tests from the previous page are what make "without changing what it does" checkable. An agent can restructure pricing in one diff in a minute. The safe version is slower to ask for and much faster to finish: one named refactoring at a time, the tests green after each, a commit after each.

The definition is worth taking literally. Martin Fowler's is "a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing its observable behavior." Every word after "without" is a claim, and on legacy code the only way to back that claim is a test run. The agent will make the claim for free. The tests are what make it true.

One Refactoring per Step

Name the refactoring in the request, not the goal. "Extract the member discount into its own function." "Rename the variable that holds the class price." "Move the time-zone conversion into its own function." Each of these produces a diff of 20 to 40 lines that you can read against its name, in Chapter 6's order: does the diff do what the name says, and nothing else? "Clean up pricing" produces a 400-line rewrite in which structure and behaviour changed together, and no reading order will separate them again.

Plan the Sequence First

Ask for the sequence of refactorings as a numbered plan before any file changes, then read it, reorder it and cut it, as Chapter 5 taught. For pricing the agent's plan comes back with ten steps. One of them reads "simplify the rounding in the pack price". That is not a refactoring. It is a behaviour change dressed as one, the per-class rounding the previous page pinned as a suspected bug, and it would move a price members pay. Strike it. Nine steps remain, each of which can be checked by the tests that already exist.

Reading the plan also catches order problems. Renaming a variable before extracting the function that uses it doubles the work; moving the time-zone conversion before naming the cut-off makes the move harder to read. A plan costs a minute to fix. The same mistakes found in step seven cost a discard.

Green, Then Commit

After each step the agent runs the characterization tests, which Chapter 8 made part of its own feedback loop. You read the diff. If both are clean, you commit, exactly as Chapter 9 taught: a known-good point before the next request. Nine steps make nine commits, each reversible on its own.

$ git log --oneline main..
e2c9002 pricing: delete the commented-out discount code
d3959af pricing: inline the temporary total in the pack price
28ee50a pricing: collect the price constants at the top of the module
419fa07 pricing: replace the loop in the pack price with a sum
acf1ecf pricing: move the time-zone conversion into its own function
f6a4193 pricing: name the 16:00 cut-off
dec342a pricing: extract the off-peak check into its own function
78a3540 pricing: rename the variable that holds the class price
5d0ec54 pricing: extract the member discount into its own function

The rhythm is quick once it is set up. The agent makes the change and runs the tests itself; you read a diff of a few dozen lines against the step's name; you commit. Nine steps of pricing take well under an hour, most of it reading. When a step comes back much larger than its name suggests, say 150 lines for a rename, stop there: either the step was not what it said, or it should have been two. Ask for it again, smaller. A step you cannot read in two minutes is a step you will approve without reading.

The first step is typical. The member discount appears twice in pricing, once in the class price and once inside the pack loop, as the same multiply-and-round expression. Extracting it is a pure structural change: one new function, two call sites and the per-class rounding exactly where it was.

--- a/pricing.py
+++ b/pricing.py
@@ -7,11 +7,15 @@
 MEMBER_RATE = Decimal("0.90")
 
 
+def member_price(price):
+    return int((price * MEMBER_RATE).quantize(Decimal("1"), rounding=ROUND_HALF_UP))
+
+
 def class_price(starts_at, member):
     hour = starts_at.astimezone(SERVER_ZONE).hour
     price = OFF_PEAK if hour < 16 else PEAK
     if member:
-        price = int((price * MEMBER_RATE).quantize(Decimal("1"), rounding=ROUND_HALF_UP))
+        price = member_price(price)
     return price
 
 
@@ -20,6 +24,6 @@
     for _ in range(size):
         price = PEAK
         if member:
-            price = int((price * MEMBER_RATE).quantize(Decimal("1"), rounding=ROUND_HALF_UP))
+            price = member_price(price)
         total += price
     return total

============================== 15 passed in 0.21s ==============================

--- a/pricing.ts
+++ b/pricing.ts
@@ -6,11 +6,15 @@
 const hourIn = new Intl.DateTimeFormat("en-US", {
   timeZone: SERVER_ZONE, hour: "numeric", hourCycle: "h23" });
 
+function memberPrice(price: number): number {
+  return Math.floor((price * MEMBER_PERCENT + 50) / 100);   // half-up, in cents
+}
+
 export function classPrice(startsAt: Date, member: boolean): number {
   const hour = Number(hourIn.format(startsAt));
   let price = hour < 16 ? OFF_PEAK : PEAK;
   if (member) {
-    price = Math.floor((price * MEMBER_PERCENT + 50) / 100);   // half-up, in cents
+    price = memberPrice(price);
   }
   return price;
 }
@@ -20,7 +24,7 @@
   for (let i = 0; i < size; i++) {
     let price = PEAK;
     if (member) {
-      price = Math.floor((price * MEMBER_PERCENT + 50) / 100);   // half-up, in cents
+      price = memberPrice(price);
     }
     total += price;
   }

      Tests  15 passed (15)

--- a/Pricing.java
+++ b/Pricing.java
@@ -11,12 +11,16 @@
 
     private Pricing() {}
 
+    static long memberPrice(long price) {
+        return BigDecimal.valueOf(price).multiply(MEMBER_RATE)
+            .setScale(0, RoundingMode.HALF_UP).longValueExact();
+    }
+
     public static long classPrice(Instant startsAt, boolean member) {
         int hour = startsAt.atZone(SERVER_ZONE).getHour();
         long price = hour < 16 ? OFF_PEAK : PEAK;
         if (member) {
-            price = BigDecimal.valueOf(price).multiply(MEMBER_RATE)
-                .setScale(0, RoundingMode.HALF_UP).longValueExact();
+            price = memberPrice(price);
         }
         return price;
     }
@@ -26,8 +30,7 @@
         for (int i = 0; i < size; i++) {
             long price = PEAK;
             if (member) {
-                price = BigDecimal.valueOf(price).multiply(MEMBER_RATE)
-                    .setScale(0, RoundingMode.HALF_UP).longValueExact();
+                price = memberPrice(price);
             }
             total += price;
         }

BUILD SUCCESSFUL in 1s

Read against its name, this diff is easy to approve. Every removed expression reappears unchanged in the new function, and every call site now calls it. Nothing else moved. That is what a 20-line diff with a name buys you: a review that takes a minute and is actually complete.

The Two Signals to Stop

Two things in a refactoring diff mean that behaviour moved. A characterization test changed in the diff, or a characterization test failed and the agent proposes updating its expected value. Either one means the step was not a refactoring. The response is to discard the step back to the last commit, as Chapter 9 described, not to accept the new value. The changed expectation is the bug report.

At Cragmoor it happens at step 6. Asked to replace the loop in the pack price with a sum, the agent "tidies" the per-class rounding into one rounding of the total. The code is shorter and arguably more correct. Two pack tests go red: the member's 10-class pack now costs 16,155 cents instead of 16,160, and the 5-class pack 8,078 instead of 8,080. The agent's suggested fix is to change the tests.

The agent is not wrong about the arithmetic. It is wrong about the job. A price change for every member who buys a pack may be the right business decision, but it is a decision, and it is not one a refactoring gets to make on the way past. The discard costs one command and the redo costs a minute, because the last good commit is one step back.

Refactor or Change, Never Both

The rounding fix is legitimate work if Ines and the owner of the pricing rule decide it should happen. It comes after the refactoring is finished, as a separate commit. In that commit a specification test, written from the decided rule as in Chapter 8, replaces the characterization assertion, and the message says in plain words that member pack prices change and why, as Chapter 9 asked of every message. Keeping the two apart gives the reviewer one diff in which structure moved and nothing else, and one in which behaviour moved and nothing else. Each is easy to review. Mixed, neither is.

The discipline has an old name among people who refactor for a living: you wear one hat at a time. While refactoring, you add no behaviour. While changing behaviour, you do not restructure. An agent asked for both at once will do both at once, fluently, in one diff.

When the Toolchain Can Do It

Renames, moves and extractions that the editor's refactoring tools perform from the language's own symbol information are deterministic. They find every reference the compiler or type checker knows about, they do not get tired at the fortieth file, and they do not decide to tidy anything on the way. Where those tools resolve every reference, prefer them to an agent's text edit, and review the result by running the tests rather than reading every changed line. Use the agent for the steps the tools cannot express, such as "replace the loop with a sum, keeping per-class rounding", and for planning the sequence. In a dynamically typed codebase the tools resolve less, which is one more reason the characterization tests come first.