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 method Invoices.Inv. It has been working for two years:

public static string Inv(string n, string street, string city, string zip, List<Item> items, string type)
{
    decimal t = 0;
    foreach (Item i in items)
    {
        t = t + i.Price * i.Qty;
    }
    decimal d = 0;
    if (type == "gold")
    {
        d = t * 0.05m;
    }
    else if (type == "company")
    {
        if (t >= 500)
        {
            d = t * 0.10m;
        }
    }
    string s = "Invoice: " + n + "\n";
    s = s + street + ", " + zip + " " + city + "\n";
    s = s + $"Subtotal: €{t:F2}\n";
    s = s + $"Discount: €{d:F2}\n";
    s = s + $"Total incl. VAT: €{AfterDiscount(t, d) * 1.20m:F2}";
    return s;
}

static decimal AfterDiscount(decimal t, decimal 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.

public class InvoiceTest
{
    List<Item> coffee = [new("Coffee beans", 12.50m, 2), new("Mug", 8.00m, 1)];
    List<Item> moreCoffee = [new("Coffee beans", 12.50m, 4)];
    List<Item> flour = [new("Flour 25 kg", 30.00m, 20)];

    [Fact]
    public void Regular()
    {
        Assert.Equal("Invoice: Maria Petrova\n5 Shipka St, 1504 Sofia\nSubtotal: €33.00\nDiscount: €0.00\nTotal incl. VAT: €39.60",
            Invoices.Inv("Maria Petrova", "5 Shipka St", "Sofia", "1504", coffee, "regular"));
    }

    [Fact]
    public void Gold()
    {
        Assert.Equal("Invoice: Ivan Georgiev\n12 Vasil Levski St, 4000 Plovdiv\nSubtotal: €50.00\nDiscount: €2.50\nTotal incl. VAT: €57.00",
            Invoices.Inv("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold"));
    }

    [Fact]
    public void CompanyFrom500()
    {
        Assert.Equal("Invoice: Izgrev Bakery Ltd\n80 Knyaz Boris I Blvd, 9000 Varna\nSubtotal: €600.00\nDiscount: €60.00\nTotal incl. VAT: €648.00",
            Invoices.Inv("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "9000", flour, "company"));
    }

    [Fact]
    public void CompanyUnder500()
    {
        Assert.Equal("Invoice: Izgrev Bakery Ltd\n80 Knyaz Boris I Blvd, 9000 Varna\nSubtotal: €33.00\nDiscount: €0.00\nTotal incl. VAT: €39.60",
            Invoices.Inv("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "9000", coffee, "company"));
    }
}

Result

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  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 becomes postcode, and type becomes customerType.

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = 0;
    foreach (Item item in items)
    {
        subtotal = subtotal + item.Price * item.Qty;
    }
    decimal discount = 0;
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
    string text = "Invoice: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{AfterDiscount(subtotal, discount) * 1.20m:F2}";
    return text;
}

The tests

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  Regular
Tests: 4, passed: 4, failed: 0

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

In Visual Studio it is Ctrl+R, Ctrl+R (or F2), in Rider it is Shift+F6. The compiler knows which Inv is which, so exactly that one changes, not every piece of text Inv.

See it

Step 2: Extract Method

The method 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 method, with a name that says what it returns. That is Extract Method:

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Invoice: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{AfterDiscount(subtotal, discount) * 1.20m:F2}";
    return text;
}

static decimal ItemsTotal(List<Item> items)
{
    decimal sum = 0;
    foreach (Item item in items)
    {
        sum = sum + item.Price * item.Qty;
    }
    return sum;
}

static decimal DiscountFor(string customerType, decimal subtotal)
{
    decimal discount = 0;
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
    return discount;
}

The tests

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  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 Visual Studio: select the lines and press Ctrl+R, Ctrl+M (or Ctrl+. and "Extract method"). In Rider: Ctrl+Alt+M.

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 method that stores it in its own parameter instead of returning it:

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = 0;
    ApplyDiscount(customerType, subtotal, discount);
    string text = "Invoice: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{AfterDiscount(subtotal, discount) * 1.20m:F2}";
    return text;
}

static void ApplyDiscount(string customerType, decimal subtotal, decimal discount)
{
    if (customerType == "gold")
    {
        discount = subtotal * 0.05m;
    }
    else if (customerType == "company")
    {
        if (subtotal >= 500)
        {
            discount = subtotal * 0.10m;
        }
    }
}

Answer

All four pass

Gold and CompanyFrom500 fail

Only CompanyFrom500 fails

Check

More precisely, if you are curious

Result

FAIL  CompanyFrom500
FAIL  Gold
PASS  CompanyUnder500
PASS  Regular
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 method 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 method disappears. That is Inline:

static decimal AfterDiscount(decimal subtotal, decimal discount)
{
    return subtotal - discount;
}

public static string Invoice(string name, string street, string city, string postcode, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Invoice: " + name + "\n";
    text = text + street + ", " + postcode + " " + city + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{(subtotal - discount) * 1.20m:F2}";
    return text;
}

The tests

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  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 (ItemsTotal). You put it back when the name is only a jump (AfterDiscount). In Rider it is Ctrl+Alt+N, in Visual Studio it is Ctrl+. and "Inline method".

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:

Invoices.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.

A record writes the properties and the constructor:

public record Customer(string Name, string Street, string City, string Postcode);

public static string Invoice(Customer customer, List<Item> items, string customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = DiscountFor(customerType, subtotal);
    string text = "Invoice: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{(subtotal - discount) * 1.20m:F2}";
    return text;
}

The tests

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  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:

Invoices.Inv("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold")
Invoices.Invoice("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000", moreCoffee, "gold")
Invoices.Invoice(new Customer("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000"), moreCoffee, "gold")
Invoices.Invoice(new Customer("Ivan Georgiev", "12 Vasil Levski St", "Plovdiv", "4000"), moreCoffee, new Gold())

In C# the constructor can also be called with names: new 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 DiscountFor. Every new type is a new branch here, and tomorrow in other places too (the email, the report). Each type becomes a class that implements a common interface, and knows its own discount:

public interface ICustomerType
{
    decimal Discount(decimal subtotal);
}

public class Regular : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return 0;
    }
}

public class Gold : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return subtotal * 0.05m;
    }
}

public class Company : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        if (subtotal >= 500)
        {
            return subtotal * 0.10m;
        }
        return 0;
    }
}

public static string Invoice(Customer customer, List<Item> items, ICustomerType customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = customerType.Discount(subtotal);
    string text = "Invoice: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{(subtotal - discount) * 1.20m:F2}";
    return text;
}

The tests

PASS  CompanyFrom500
PASS  CompanyUnder500
PASS  Gold
PASS  Regular
Tests: 4, passed: 4, failed: 0

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.

public record Customer(string Name, string Street, string City, string Postcode);

public interface ICustomerType
{
    decimal Discount(decimal subtotal);
}

public static string Invoice(Customer customer, List<Item> items, ICustomerType customerType)
{
    decimal subtotal = ItemsTotal(items);
    decimal discount = customerType.Discount(subtotal);
    string text = "Invoice: " + customer.Name + "\n";
    text = text + customer.Street + ", " + customer.Postcode + " " + customer.City + "\n";
    text = text + $"Subtotal: €{subtotal:F2}\n";
    text = text + $"Discount: €{discount:F2}\n";
    text = text + $"Total incl. VAT: €{(subtotal - discount) * 1.20m:F2}";
    return text;
}

Step by step, in words

1. The function before the first step: 23 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 behind ICustomerType, 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:

public class Company : ICustomerType
{
    public decimal Discount(decimal subtotal)
    {
        return subtotal * 0.10m;
    }
}

A company orders €33 of coffee:

Invoices.Invoice(new Customer("Izgrev Bakery Ltd", "80 Knyaz Boris I Blvd", "Varna", "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.

1public class Company : ICustomerType2{3 public decimal Discount(decimal subtotal)4 {5 return subtotal * 0.10m;6 }7}

Check

More precisely, if you are curious

The tests

FAIL  CompanyUnder500
PASS  CompanyFrom500
PASS  Gold
PASS  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 method 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