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:

def inv(n, street, city, zip, items, type):
    t = 0
    for i in items:
        t = t + i.price * i.qty
    d = 0
    if type == "gold":
        d = t * 0.05
    elif type == "company":
        if t >= 500:
            d = t * 0.10
    s = "Invoice: " + n + "\n"
    s = s + street + ", " + zip + " " + city + "\n"
    s = s + f"Subtotal: €{t:.2f}\n"
    s = s + f"Discount: €{d:.2f}\n"
    s = s + f"Total incl. VAT: €{after_discount(t, d) * 1.20:.2f}"
    return s

def after_discount(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 after_discount 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.

COFFEE = [Item("Coffee beans", 12.50, 2), Item("Mug", 8.00, 1)]
MORE_COFFEE = [Item("Coffee beans", 12.50, 4)]
FLOUR = [Item("Flour 25 kg", 30.00, 20)]


class InvoiceTest(unittest.TestCase):
    def test_regular(self):
        self.assertEqual(
            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",
        )

    def test_gold(self):
        self.assertEqual(
            inv("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", MORE_COFFEE, "gold"),
            "Invoice: Ivan Georgiev\n12 Vasil Levski St, 4000 Plovdiv\nSubtotal: €50.00\nDiscount: €2.50\nTotal incl. VAT: €57.00",
        )

    def test_company_from_500(self):
        self.assertEqual(
            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",
        )

    def test_company_under_500(self):
        self.assertEqual(
            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  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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 and type are also names of built-in functions in Python, which the parameters hide. They become postcode and customer_type.

def invoice(name, street, city, postcode, items, customer_type):
    subtotal = 0
    for item in items:
        subtotal = subtotal + item.price * item.qty
    discount = 0
    if customer_type == "gold":
        discount = subtotal * 0.05
    elif customer_type == "company":
        if subtotal >= 500:
            discount = subtotal * 0.10
    text = "Invoice: " + name + "\n"
    text = text + street + ", " + postcode + " " + city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{after_discount(subtotal, discount) * 1.20:.2f}"
    return text

The tests

PASS  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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 PyCharm it is Shift+F6, in VS Code it is F2. Without types, though, the IDE sometimes guesses: a name in a string, getattr(obj, "name") or a dictionary key are not links it can see. That is why in Python, 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:

def invoice(name, street, city, postcode, items, customer_type):
    subtotal = items_total(items)
    discount = discount_for(customer_type, subtotal)
    text = "Invoice: " + name + "\n"
    text = text + street + ", " + postcode + " " + city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{after_discount(subtotal, discount) * 1.20:.2f}"
    return text

def items_total(items):
    total = 0
    for item in items:
        total = total + item.price * item.qty
    return total


def discount_for(customer_type, subtotal):
    discount = 0
    if customer_type == "gold":
        discount = subtotal * 0.05
    elif customer_type == "company":
        if subtotal >= 500:
            discount = subtotal * 0.10
    return discount

The tests

PASS  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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 PyCharm: select the lines and press Ctrl+Alt+M. In VS Code: Ctrl+. and "Extract method".

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:

def invoice(name, street, city, postcode, items, customer_type):
    subtotal = items_total(items)
    discount = 0
    apply_discount(customer_type, subtotal, discount)
    text = "Invoice: " + name + "\n"
    text = text + street + ", " + postcode + " " + city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{after_discount(subtotal, discount) * 1.20:.2f}"
    return text


def apply_discount(customer_type, subtotal, discount):
    if customer_type == "gold":
        discount = subtotal * 0.05
    elif customer_type == "company":
        if subtotal >= 500:
            discount = subtotal * 0.10

Answer

Only test_company_from_500 fails

All four pass

test_gold and test_company_from_500 fail

Check

More precisely, if you are curious

Result

FAIL  test_company_from_500
FAIL  test_gold
PASS  test_company_under_500
PASS  test_regular
Tests: 4, passed: 2, failed: 2

discount inside apply_discount 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. after_discount 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:

def after_discount(subtotal, discount):
    return subtotal - discount

def invoice(name, street, city, postcode, items, customer_type):
    subtotal = items_total(items)
    discount = discount_for(customer_type, subtotal)
    text = "Invoice: " + name + "\n"
    text = text + street + ", " + postcode + " " + city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{(subtotal - discount) * 1.20:.2f}"
    return text

The tests

PASS  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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 (items_total). You put it back when the name is only a jump (after_discount). In PyCharm it is Ctrl+Alt+N.

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.

@dataclass writes __init__ and the comparison:

@dataclass(frozen=True)
class Customer:
    name: str
    street: str
    city: str
    postcode: str

def invoice(customer, items, customer_type):
    subtotal = items_total(items)
    discount = discount_for(customer_type, subtotal)
    text = "Invoice: " + customer.name + "\n"
    text = text + customer.street + ", " + customer.postcode + " " + customer.city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{(subtotal - discount) * 1.20:.2f}"
    return text

The tests

PASS  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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", MORE_COFFEE, "gold")
invoice("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", MORE_COFFEE, "gold")
invoice(Customer("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000"), MORE_COFFEE, "gold")
invoice(Customer("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000"), MORE_COFFEE, Gold())

In Python both the dataclass constructor and functions accept arguments by name: Customer(name="...", city="...", ...). Then the order doesn't matter.

See it

Step 5: Replace Conditional with Polymorphism

What's left is the chain of if on the customer type in discount_for. 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:

class Regular:
    def discount(self, subtotal):
        return 0


class Gold:
    def discount(self, subtotal):
        return subtotal * 0.05


class Company:
    def discount(self, subtotal):
        if subtotal >= 500:
            return subtotal * 0.10
        return 0

def invoice(customer, items, customer_type):
    subtotal = items_total(items)
    discount = customer_type.discount(subtotal)
    text = "Invoice: " + customer.name + "\n"
    text = text + customer.street + ", " + customer.postcode + " " + customer.city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{(subtotal - discount) * 1.20:.2f}"
    return text

The tests

PASS  test_company_from_500
PASS  test_company_under_500
PASS  test_gold
PASS  test_regular
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.

@dataclass(frozen=True)
class Customer:
    name: str
    street: str
    city: str
    postcode: str


def invoice(customer, items, customer_type):
    subtotal = items_total(items)
    discount = customer_type.discount(subtotal)
    text = "Invoice: " + customer.name + "\n"
    text = text + customer.street + ", " + customer.postcode + " " + customer.city + "\n"
    text = text + f"Subtotal: €{subtotal:.2f}\n"
    text = text + f"Discount: €{discount:.2f}\n"
    text = text + f"Total incl. VAT: €{(subtotal - discount) * 1.20:.2f}"
    return text

Step by step, in words

1. The function before the first step: 15 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 items_total, the chain of if into discount_for. One line is left for each.
4. Inline: after_discount 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 customer_type.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:

class Company:
    def discount(self, subtotal):
        return subtotal * 0.10

A company orders €33 of coffee:

invoice(Customer("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "9000"), COFFEE, 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.

1class Company:2 def discount(self, subtotal):3 return subtotal * 0.10

Check

More precisely, if you are curious

The tests

FAIL  test_company_under_500
PASS  test_company_from_500
PASS  test_gold
PASS  test_regular
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 address_line(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