Refactoring in small steps

Code in:JavaC#PythonJavaScript

In short: Refactoring changes the structure of code without changing its behaviour, in small named steps such as Rename and Extract Method, with green tests after each one.

Problem

The invoice nobody wants to touch

The online shop prints invoices with the function inv. It has been working for two years:

export function inv(n, street, city, zip, items, type) {
  let t = 0;
  for (const i of items) {
    t = t + i.price * i.qty;
  }
  let d = 0;
  if (type === "gold") {
    d = t * 0.05;
  } else if (type === "company") {
    if (t >= 500) {
      d = t * 0.1;
    }
  }
  let s = `Invoice: ${n}\n`;
  s = s + `${street}, ${zip} ${city}\n`;
  s = s + `Subtotal: €${t.toFixed(2)}\n`;
  s = s + `Discount: €${d.toFixed(2)}\n`;
  s = s + `Total incl. VAT: €${(afterDiscount(t, d) * 1.2).toFixed(2)}`;
  return s;
}

function afterDiscount(t, d) {
  return t - d;
}

What is t? And d? Six parameters in a row, four of them text: whoever swaps the city and the postcode gets an invoice with "1504 Sofia" in the wrong place, and the compiler says nothing. The chain of if on the customer type keeps growing: next month partners with 3% are coming. And afterDiscount does one subtraction, but makes you jump somewhere else to see it.

You want to tidy it up. But the invoices go to accounting, and a mistake in them is a mistake in the taxes.

The idea

Refactoring: the shape changes, the behaviour doesn't

Refactoring is a change to the shape of the code where the behaviour stays the same: the same inputs give the same outputs. It isn't "rewriting". It isn't "I also fixed a bug while I was there" either. Those are other changes, made separately.

It is done in small named steps. Each step is so small that it can hardly go wrong, and after each one you run the tests (the safety net from the previous lesson). The names come from Martin Fowler's catalogue and are the same in every language and every IDE: Rename, Extract Method, Inline, Introduce Parameter Object, Replace Conditional with Polymorphism.

The net comes first. Four tests pin down the four kinds of invoice the shop prints today: a regular customer, a gold customer, a company over and under €500.

import { test } from "node:test";
import assert from "node:assert/strict";
import { inv } from "./invoice.js";

const coffee = [{ name: "Coffee beans", price: 12.5, qty: 2 }, { name: "Mug", price: 8, qty: 1 }];
const moreCoffee = [{ name: "Coffee beans", price: 12.5, qty: 4 }];
const flour = [{ name: "Flour 25 kg", price: 30, qty: 20 }];

test("regular customer", () => {
  assert.equal(
    inv("Maria Petrova", "5 Shipka St", "Sofia", "1504", coffee, "regular"),
    "Invoice: Maria Petrova\n5 Shipka St, 1504 Sofia\nSubtotal: €33.00\nDiscount: €0.00\nTotal incl. VAT: €39.60",
  );
});

test("gold customer", () => {
  assert.equal(
    inv("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold"),
    "Invoice: Ivan Georgiev\n12 Vasil Levski St, 4000 Plovdiv\nSubtotal: €50.00\nDiscount: €2.50\nTotal incl. VAT: €57.00",
  );
});

test("company from €500", () => {
  assert.equal(
    inv("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "9000", flour, "company"),
    "Invoice: Izgrev Bakery Ltd\n80 Knyaz Boris I Blvd, 9000 Varna\nSubtotal: €600.00\nDiscount: €60.00\nTotal incl. VAT: €648.00",
  );
});

test("company under €500", () => {
  assert.equal(
    inv("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "9000", coffee, "company"),
    "Invoice: Izgrev Bakery Ltd\n80 Knyaz Boris I Blvd, 9000 Varna\nSubtotal: €33.00\nDiscount: €0.00\nTotal incl. VAT: €39.60",
  );
});

Result

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

See it

Step 1: Rename

One-letter names become words: t is subtotal, d is discount, s is text, inv is invoice. zip becomes postcode, and type becomes customerType.

export function invoice(name, street, city, postcode, items, customerType) {
  let subtotal = 0;
  for (const item of items) {
    subtotal = subtotal + item.price * item.qty;
  }
  let discount = 0;
  if (customerType === "gold") {
    discount = subtotal * 0.05;
  } else if (customerType === "company") {
    if (subtotal >= 500) {
      discount = subtotal * 0.1;
    }
  }
  let text = `Invoice: ${name}\n`;
  text = text + `${street}, ${postcode} ${city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${(afterDiscount(subtotal, discount) * 1.2).toFixed(2)}`;
  return text;
}

The tests

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

Rename also changes every place that calls the function, including the tests. The expected invoices are not touched.

In VS Code it is F2. For a local variable it is safe. For an object property (customer.postcode) the editor doesn't know which object is which: obj[key] or an object that came in as JSON are not links it can see. That is why in JavaScript, after a rename, the tests tell you more than in Java or C#.

See it

Step 2: Extract Method

The function does two separate jobs that have nothing to do with the invoice text: the total of the lines and the discount. Each goes into its own function, with a name that says what it returns. That is Extract Method:

export function invoice(name, street, city, postcode, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = discountFor(customerType, subtotal);
  let text = `Invoice: ${name}\n`;
  text = text + `${street}, ${postcode} ${city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${(afterDiscount(subtotal, discount) * 1.2).toFixed(2)}`;
  return text;
}

function itemsTotal(items) {
  let sum = 0;
  for (const item of items) {
    sum = sum + item.price * item.qty;
  }
  return sum;
}

function discountFor(customerType, subtotal) {
  let discount = 0;
  if (customerType === "gold") {
    discount = subtotal * 0.05;
  } else if (customerType === "company") {
    if (subtotal >= 500) {
      discount = subtotal * 0.1;
    }
  }
  return discount;
}

The tests

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

Now invoice reads top to bottom like a list: how much it is, how much the discount is, what the text is. The details are one jump away, only if you need them.

In VS Code: select the lines, press Ctrl+. and choose "Extract to function in module scope".

The IDE looks at which variables go into the selected lines and which come out, and makes the parameters and the return value. If you do the step by hand, that is exactly where it goes wrong.

Try it

Predict the output

Someone does Extract Method by hand. The discount goes into a function that stores it in its own parameter instead of returning it:

export function invoice(name, street, city, postcode, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = 0;
  applyDiscount(customerType, subtotal, discount);
  let text = `Invoice: ${name}\n`;
  text = text + `${street}, ${postcode} ${city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${(afterDiscount(subtotal, discount) * 1.2).toFixed(2)}`;
  return text;
}

function applyDiscount(customerType, subtotal, discount) {
  if (customerType === "gold") {
    discount = subtotal * 0.05;
  } else if (customerType === "company") {
    if (subtotal >= 500) {
      discount = subtotal * 0.1;
    }
  }
}

Answer

"gold customer" and "company from €500" fail

All four pass

Only "company from €500" fails

Check

More precisely, if you are curious

Result

FAIL  company from €500
FAIL  gold customer
PASS  company under €500
PASS  regular customer
Tests: 4, passed: 2, failed: 2

discount inside applyDiscount is a copy of the value. The new value stays in there and disappears when the function ends, and outside, discount is still 0. Where the discount is 0 anyway (a regular customer, a company under €500), the tests pass. That is why the net has a test for every branch of the if. The extracted code has to return the value, the way the IDE does it.

See it

Step 3: Inline

The opposite of Extract Method. afterDiscount does one subtraction, and its name says nothing more than subtotal - discount. It is used in one place. The body goes where the call is, and the function disappears. That is Inline:

function afterDiscount(subtotal, discount) {
  return subtotal - discount;
}

export function invoice(name, street, city, postcode, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = discountFor(customerType, subtotal);
  let text = `Invoice: ${name}\n`;
  text = text + `${street}, ${postcode} ${city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${((subtotal - discount) * 1.2).toFixed(2)}`;
  return text;
}

The tests

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

Extract Method and Inline don't contradict each other. You extract when the name explains something the code doesn't say (itemsTotal). You put it back when the name is only a jump (afterDiscount). In VS Code, for a variable it is Ctrl+. and "Inline variable"; a function is inlined by hand.

See it

Step 4: Introduce Parameter Object

The name, the street, the city and the postcode always travel together, yet they are four parameters in a row. A swap doesn't show:

invoice("Maria Petrova", "5 Shipka St", "1504", "Sofia", coffee, "regular")

Output

Invoice: Maria Petrova
5 Shipka St, Sofia 1504
Subtotal: €33.00
Discount: €0.00
Total incl. VAT: €39.60

The four become one object: the customer. That is Introduce Parameter Object.

In JavaScript you don't need a class: the customer is a plain object with field names, { name, street, city, postcode }.

export function invoice(customer, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = discountFor(customerType, subtotal);
  let text = `Invoice: ${customer.name}\n`;
  text = text + `${customer.street}, ${customer.postcode} ${customer.city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${((subtotal - discount) * 1.2).toFixed(2)}`;
  return text;
}

The tests

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

The calls in the tests change with the shape, and the expected invoices stay the same. Here is one call through the steps:

inv("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold")
invoice("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold")
invoice({ name: "Ivan Georgiev", street: "12 Vasil Levski St", city: "Plovdiv", postcode: "4000" }, moreCoffee, "gold")
invoice({ name: "Ivan Georgiev", street: "12 Vasil Levski St", city: "Plovdiv", postcode: "4000" }, moreCoffee, new Gold())

In JavaScript the object has field names, and there are no places to swap: { city: "Sofia", postcode: "1504" } is the same as { postcode: "1504", city: "Sofia" }.

See it

Step 5: Replace Conditional with Polymorphism

What's left is the chain of if on the customer type in discountFor. Every new type is a new branch here, and tomorrow in other places too (the email, the report). Each type becomes a class with a discount method, and knows its own discount:

export class Regular {
  discount(subtotal) {
    return 0;
  }
}

export class Gold {
  discount(subtotal) {
    return subtotal * 0.05;
  }
}

export class Company {
  discount(subtotal) {
    if (subtotal >= 500) {
      return subtotal * 0.1;
    }
    return 0;
  }
}

export function invoice(customer, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = customerType.discount(subtotal);
  let text = `Invoice: ${customer.name}\n`;
  text = text + `${customer.street}, ${customer.postcode} ${customer.city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${((subtotal - discount) * 1.2).toFixed(2)}`;
  return text;
}

The tests

PASS  company from €500
PASS  company under €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 4, failed: 0

There is no interface here: each class simply has a discount method, and the invoice calls it (duck typing). If the rule is a single calculation, a function in a table is enough, as in the Strategy lesson.

This is Strategy from the patterns module: the rule is a separate object, and the invoice only asks. If the customer type changes while the program runs (they become gold after the tenth order), the same move leads to State. The text "gold" from the database turns into an object in one place, at the entrance, with a table as in the Strategy lesson.

export function invoice(customer, items, customerType) {
  const subtotal = itemsTotal(items);
  const discount = customerType.discount(subtotal);
  let text = `Invoice: ${customer.name}\n`;
  text = text + `${customer.street}, ${customer.postcode} ${customer.city}\n`;
  text = text + `Subtotal: €${subtotal.toFixed(2)}\n`;
  text = text + `Discount: €${discount.toFixed(2)}\n`;
  text = text + `Total incl. VAT: €${((subtotal - discount) * 1.2).toFixed(2)}`;
  return text;
}

Step by step, in words

1. The function before the first step: 18 lines in one block. Below are the four tests and the totals of the four invoices.
2. Rename: t becomes subtotal, d becomes discount, s becomes text. The tests are green, the totals are the same.
3. Extract Method: the loop goes into itemsTotal, the chain of if into discountFor. One line is left for each.
4. Inline: afterDiscount only subtracted. Its body goes back into the line, and the function is gone.
5. Introduce Parameter Object: name, street, city and postcode become one customer.
6. Replace Conditional with Polymorphism: the if is gone. Each customer type is a class, and the function calls customerType.discount(subtotal).
7. Five steps. The shape of invoice is different; the four tests and the four invoices are the same as at the start.

The program that prints the four invoices was run with the code from before the first step and with the code after the fifth. The output is the same:

Output before and after

Invoice: Maria Petrova
5 Shipka St, 1504 Sofia
Subtotal: €33.00
Discount: €0.00
Total incl. VAT: €39.60

Invoice: Ivan Georgiev
12 Vasil Levski St, 4000 Plovdiv
Subtotal: €50.00
Discount: €2.50
Total incl. VAT: €57.00

Invoice: Izgrev Bakery Ltd
80 Knyaz Boris I Blvd, 9000 Varna
Subtotal: €600.00
Discount: €60.00
Total incl. VAT: €648.00

Invoice: Izgrev Bakery Ltd
80 Knyaz Boris I Blvd, 9000 Varna
Subtotal: €33.00
Discount: €0.00
Total incl. VAT: €39.60

Try it

Find the bug

Someone writes the class for a company like this:

export class Company {
  discount(subtotal) {
    return subtotal * 0.1;
  }
}

A company orders €33 of coffee:

invoice({ name: "Izgrev Bakery Ltd", street: "80 Knyaz Boris I Blvd", city: "Varna", postcode: "9000" }, coffee, new Company())

Output

Invoice: Izgrev Bakery Ltd
80 Knyaz Boris I Blvd, 9000 Varna
Subtotal: €33.00
Discount: €3.30
Total incl. VAT: €35.64

Which line is the bug on?

Pick the line with the bug.

1export class Company {2 discount(subtotal) {3 return subtotal * 0.1;4 }5}

Check

More precisely, if you are curious

The tests

FAIL  company under €500
PASS  company from €500
PASS  gold customer
PASS  regular customer
Tests: 4, passed: 3, failed: 1

The idea

When to stop

Refactoring has no end unless you give it one. You stop when the code is clear enough for the change that is coming. The task is to add partners with 3%? Step 5 makes it one new class. The task is something else? Maybe you don't need step 5.

It isn't worth it when:

• nobody touches or reads the code. Ugly code that works and doesn't change is not urgent;
• there are two variants and they won't grow. One if is clearer than an interface and two classes;
• there is no net under the code. Then the first step is the net, not refactoring.

And one rule: don't mix. You change either the shape or the behaviour, never both in one step. Kent Beck calls it "two hats": with one you rearrange and the tests aren't touched, with the other you add and write a new test. You switch hats on purpose.

Exercise

Partners with 3%

Solve the task in your editor.

Partners. The shop adds a "partner" customer type with a 3% discount on every order. Start from the code after step 5. First write a test for a partner with the €33 order, then the Partner class. The other classes and invoice are not touched.

Output

Invoice: Maria Petrova
5 Shipka St, 1504 Sofia
Subtotal: €33.00
Discount: €0.99
Total incl. VAT: €38.41

More tasks

• Extract Method for the text. Extract the address line into a function addressLine(customer). Run the tests after the step.
• The other order. Do the five steps in a different order: Introduce Parameter Object first, then Rename. Do the tests pass after each one? Which step was easier earlier?
• The type from the database. The customer type comes in as text ("gold"). Write the table that turns it into an object, and decide what happens with an unknown type. Today's code gives a discount of 0 for an unknown type: keep that with a test.

Test

Check yourself

1. Which of these is refactoring?

The discount for a gold customer becomes 7%The invoice starts showing VAT on its own lineThe loop for the subtotal moves into a separate method with the same result

2. After Introduce Parameter Object the tests don't compile, because they call the method with six parameters. What do you do?

Change the expected invoices too, so that they matchChange only the calls in the tests; the expected invoices stay as they areDelete the tests and write new ones at the end

3. When is Inline better than Extract Method?

When the small method says as much as its body and only adds a jump when readingWhen the method is longNever: more methods is always better

4. The chain of if on the customer type becomes an interface and a class for each type. What is this called in the patterns module?

SingletonStrategy (or State, if the type changes while the program runs)Observer

5. Five steps, tests after each one. On the fourth, one test fails. What does that mean?

The fourth step changed the behaviour, and the first three are fineThe test is wrongYou have to go back to the start

Check

Start the lesson over

Related lessons

• Builds on: Names in code · Functions: one job, few parameters, no hidden side effects · Code smells: long method, large class, duplication, primitives instead of types, long if chains · Tests as a safety net
• Continues in: SOLID without dogma · Exercise: a messy class, cleaned up step by step
• Tasks on this topic: Clean code and refactoring