Exercise: a messy class, cleaned up step by step

Code in:JavaC#PythonJavaScript

In short: A refactoring exercise: a scooter rental class is cleaned up in five small steps (names, magic numbers, money, query apart from command, plans) while the tests stay green.

Problem

The scooters nobody dares to touch

A company that rents out e-scooters bills every ride with the RideMgr class. It has worked for three years. The calc method works out the price from the customer's plan (pay as you go, student or monthly pass), drops the unlock fee from the fourth ride of the day, adds a night surcharge, adds up the revenue and sends a receipt:

class RideMgr {
    private final Map<String, Integer> c = new HashMap<>();
    private double s = 0;
    private final List<String> out = new ArrayList<>();

    // Price: €1 to unlock and €0.15 per minute.
    // At night the price goes up by 50%.
    public double calc(String to, String t, int m, boolean f) {
        c.put(to, c.getOrDefault(to, 0) + 1);
        double p = 0;
        if (t.equals("payg")) {
            p = 1.0 + m * 0.2;
        } else if (t.equals("student")) {
            p = (1.0 + m * 0.2) * 0.8;
        } else if (t.equals("monthly")) {
            if (m > 45) {
                p = (m - 45) * 0.2;
            }
        }
        if (c.get(to) > 3 && !t.equals("monthly")) {
            p = p - 1.0;
        }
        if (f) {
            p = p + 0.5;
        }
        s = s + p;
        out.add(to + ": " + m + " min, €" + String.format("%.2f", p));
        return p;
    }

    public List<String> sent() {
        return out;
    }

    public double day() {
        return s;
    }
}

Next month two changes arrive:

• a business plan: no unlock fee, €0.18 per minute, no discount for the fourth ride;
• the price before the end: the app shows what the ride will cost while it is still going.

The second one cannot be done with this class. Here is what happens if the app calls calc to show the price, and then again to take the money:

RideMgr rides = new RideMgr();
double shown = rides.calc("ana@example.com", "payg", 10, false);
System.out.println("Price before the end: " + String.format("€%.2f", shown));
double charged = rides.calc("ana@example.com", "payg", 10, false);
System.out.println("Charged: " + String.format("€%.2f", charged));
System.out.println("Receipts: " + rides.sent().size());
System.out.println("Revenue: " + String.format("€%.2f", rides.day()));

Output

Price before the end: €3.00
Charged: €3.00
Receipts: 2
Revenue: €6.00

One ride, two receipts and €6 of revenue. calc sounds like a question, but it counts, adds up and sends.

There is no new idea in this lesson. It is an exercise: the class goes through the tools of the module one by one, in small steps, and you run the tests after each step.

See it

Step 0: the safety net

First the net, as in the lesson on tests. Nine tests pin what the class does today: the price for each plan, the night, the fourth ride, the receipt and the revenue.

import java.util.List;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;

class RideMgrTest {
    static final String EMAIL = "ana@example.com";

    RideMgr rides = new RideMgr();

    String ride(String plan, int minutes, boolean night) {
        return money(rides.calc(EMAIL, plan, minutes, night));
    }

    String money(double amount) {
        return String.format("%.2f", amount);
    }

    @Test
    void payAsYouGoRide() {
        assertEquals("3.00", ride("payg", 10, false));
    }

    @Test
    void studentRide() {
        assertEquals("2.40", ride("student", 10, false));
    }

    @Test
    void monthlyWithinFreeMinutes() {
        assertEquals("0.00", ride("monthly", 30, false));
    }

    @Test
    void monthlyOverFreeMinutes() {
        assertEquals("3.00", ride("monthly", 60, false));
    }

    @Test
    void nightSurcharge() {
        assertEquals("3.50", ride("payg", 10, true));
    }

    @Test
    void fourthRideUnlockIsFree() {
        ride("payg", 10, false);
        ride("payg", 10, false);
        ride("payg", 10, false);
        assertEquals("2.00", ride("payg", 10, false));
    }

    @Test
    void studentFourthRide() {
        // As it is today: the discount is the full €1, not €0.80.
        ride("student", 10, false);
        ride("student", 10, false);
        ride("student", 10, false);
        assertEquals("1.40", ride("student", 10, false));
    }

    @Test
    void receipt() {
        ride("payg", 10, false);
        assertEquals(List.of("ana@example.com: 10 min, €3.00"), rides.sent());
    }

    @Test
    void revenue() {
        ride("payg", 10, false);
        ride("student", 10, false);
        assertEquals("5.40", money(rides.day()));
    }
}

Result

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

The tests call the class only through the ride helper. When the shape of the class changes in the next steps, the helper and the calls will change, and the expected values never.

One of the tests is odd. A student pays 80% of the price, so their unlock costs €0.80. On the fourth ride, though, the class takes off the full €1, and the student pays €1.40 instead of €1.60. Is it a bug or a gift? You do not decide, and you do not decide now. The test pins what happens today, with a comment, and the question goes to the product owner. While you clean up, the behavior does not change, even when it looks wrong.

More precisely, if you are curious

The amounts are compared as text with two decimals ("2.40"), the way the customer sees them. Comparing numbers would trip over the small binary errors of double:

System.out.println((1.0 + 10 * 0.2) * 0.8);

Output

2.4000000000000004

And in step 3 the type of the amounts will change. The text with two decimals is the same before and after.

See it

Step 1: the names and the flag

t, m, f, p, c and s become words (names). The most important name is the method's: it does not calculate, it finishes the ride. The honest name is finishRide, and now the side effect shows in the name. The class becomes RideBilling.

class RideBilling {
    private final Map<String, Integer> ridesToday = new HashMap<>();
    private double revenue = 0;
    private final List<String> receipts = new ArrayList<>();

    // Price: €1 to unlock and €0.15 per minute.
    // At night the price goes up by 50%.
    public double finishRide(String email, String plan, int minutes, boolean night) {
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        double price = 0;
        if (plan.equals("payg")) {
            price = 1.0 + minutes * 0.2;
        } else if (plan.equals("student")) {
            price = (1.0 + minutes * 0.2) * 0.8;
        } else if (plan.equals("monthly")) {
            if (minutes > 45) {
                price = (minutes - 45) * 0.2;
            }
        }
        if (ridesToday.get(email) > 3 && !plan.equals("monthly")) {
            price = price - 1.0;
        }
        if (night) {
            price = price + 0.5;
        }
        revenue = revenue + price;
        receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
        return price;
    }

    public List<String> receipts() {
        return receipts;
    }

    public double revenue() {
        return revenue;
    }
}

The flag is left (functions). true in the call says nothing. Night is data about the ride, not a choice of a different job, so it does not become two functions. The call has to say what it means:

Java has no named arguments. In the tests the values are constants with names, DAY and NIGHT, and the helper passes them on.

Before

String ride(String plan, int minutes, boolean night) {
    return money(rides.calc(EMAIL, plan, minutes, night));
}

@Test
void nightSurcharge() {
    assertEquals("3.50", ride("payg", 10, true));
}

After

static final boolean DAY = false;
static final boolean NIGHT = true;

String ride(String plan, int minutes, boolean night) {
    return money(billing.finishRide(EMAIL, plan, minutes, night));
}

@Test
void nightSurcharge() {
    assertEquals("3.50", ride("payg", 10, NIGHT));
}

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

In IntelliJ IDEA, rename is Shift+F6: it changes the calls in the tests too.

See it

Step 2: the numbers and the comment that lies

1.0, 0.2, 0.8, 45, 3 and 0.5 become constants with names (magic number → constant).

The comment above the method says €0.15 per minute and a 50% night surcharge. The code charges €0.20 and adds €0.50. Which is true? The tests pinned what the code does, and customers have paid by it for years. The comment lies (comments) and is deleted. The constants say the same thing and cannot go stale on their own.

class RideBilling {
    private static final double UNLOCK_FEE = 1.00;
    private static final double PER_MINUTE = 0.20;
    private static final double STUDENT_SHARE = 0.80;
    private static final int MONTHLY_FREE_MINUTES = 45;
    private static final int PAID_UNLOCKS_PER_DAY = 3;
    private static final double NIGHT_SURCHARGE = 0.50;

    private final Map<String, Integer> ridesToday = new HashMap<>();
    private double revenue = 0;
    private final List<String> receipts = new ArrayList<>();

    public double finishRide(String email, String plan, int minutes, boolean night) {
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        double price = 0;
        if (plan.equals("payg")) {
            price = UNLOCK_FEE + minutes * PER_MINUTE;
        } else if (plan.equals("student")) {
            price = (UNLOCK_FEE + minutes * PER_MINUTE) * STUDENT_SHARE;
        } else if (plan.equals("monthly")) {
            if (minutes > MONTHLY_FREE_MINUTES) {
                price = (minutes - MONTHLY_FREE_MINUTES) * PER_MINUTE;
            }
        }
        if (ridesToday.get(email) > PAID_UNLOCKS_PER_DAY && !plan.equals("monthly")) {
            price = price - UNLOCK_FEE;
        }
        if (night) {
            price = price + NIGHT_SURCHARGE;
        }
        revenue = revenue + price;
        receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
        return price;
    }

    public List<String> receipts() {
        return receipts;
    }

    public double revenue() {
        return revenue;
    }
}

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

The tests are not touched: neither the name nor the calls have changed.

Try it

Find the bug

A colleague does the same step, but trusts the comment and keeps it:

Pick the line with the bug.

1// Price: €1 to unlock and €0.15 per minute.2// At night the price goes up by 50%.3private static final double UNLOCK_FEE = 1.00;4private static final double PER_MINUTE = 0.15;5private static final double STUDENT_SHARE = 0.80;6private static final int MONTHLY_FREE_MINUTES = 45;7private static final int PAID_UNLOCKS_PER_DAY = 3;8private static final double NIGHT_SURCHARGE = 0.50;

Check

More precisely, if you are curious

The tests

FAIL  fourthRideUnlockIsFree()
FAIL  monthlyOverFreeMinutes()
FAIL  nightSurcharge()
FAIL  payAsYouGoRide()
FAIL  receipt()
FAIL  revenue()
FAIL  studentFourthRide()
FAIL  studentRide()
PASS  monthlyWithinFreeMinutes()
Tests: 9, passed: 1, failed: 8

See it

Step 3: the money

Money in a double is primitive obsession (smells). Today it gives no wrong cent in any test, because every amount is rounded only when it is printed. That is why the step is a refactoring: the output is the same. But the revenue is a sum of thousands of such amounts, and that is where the small errors pile up.

double becomes BigDecimal, created from a string. BigDecimal has no operators: + and * become add and multiply. The repeated PER_MINUTE.multiply(BigDecimal.valueOf(minutes)) becomes a small perMinute method.

class RideBilling {
    private static final BigDecimal UNLOCK_FEE = new BigDecimal("1.00");
    private static final BigDecimal PER_MINUTE = new BigDecimal("0.20");
    private static final BigDecimal STUDENT_SHARE = new BigDecimal("0.80");
    private static final int MONTHLY_FREE_MINUTES = 45;
    private static final int PAID_UNLOCKS_PER_DAY = 3;
    private static final BigDecimal NIGHT_SURCHARGE = new BigDecimal("0.50");

    private final Map<String, Integer> ridesToday = new HashMap<>();
    private BigDecimal revenue = BigDecimal.ZERO;
    private final List<String> receipts = new ArrayList<>();

    private static BigDecimal perMinute(int minutes) {
        return PER_MINUTE.multiply(BigDecimal.valueOf(minutes));
    }

    public BigDecimal finishRide(String email, String plan, int minutes, boolean night) {
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        BigDecimal price = BigDecimal.ZERO;
        if (plan.equals("payg")) {
            price = UNLOCK_FEE.add(perMinute(minutes));
        } else if (plan.equals("student")) {
            price = UNLOCK_FEE.add(perMinute(minutes)).multiply(STUDENT_SHARE);
        } else if (plan.equals("monthly")) {
            if (minutes > MONTHLY_FREE_MINUTES) {
                price = perMinute(minutes - MONTHLY_FREE_MINUTES);
            }
        }
        if (ridesToday.get(email) > PAID_UNLOCKS_PER_DAY && !plan.equals("monthly")) {
            price = price.subtract(UNLOCK_FEE);
        }
        if (night) {
            price = price.add(NIGHT_SURCHARGE);
        }
        revenue = revenue.add(price);
        receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
        return price;
    }

    public List<String> receipts() {
        return receipts;
    }

    public BigDecimal revenue() {
        return revenue;
    }
}

In the tests only the helper that turns the amount into text changes:

String money(BigDecimal amount) {
    return String.format("%.2f", amount);
}

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

Try it

Fill in

The price per minute in the language's type for money:

private static final BigDecimal PER_MINUTE = new BigDecimal();

Check

See it

Step 4: the question apart from the command

Now the change that was waiting: the price before the end has to be a question that changes nothing (command or question). The calculation is extracted into a price method (Extract Method from the refactoring lesson). finishRide stays a command: it asks for the price, then counts, adds up and sends.

A colleague does it like this. The counter moves after the price, because a ride counts once it has finished:

What do the nine tests say?

public BigDecimal price(String email, String plan, int minutes, boolean night) {
    BigDecimal price = BigDecimal.ZERO;
    if (plan.equals("payg")) {
        price = UNLOCK_FEE.add(perMinute(minutes));
    } else if (plan.equals("student")) {
        price = UNLOCK_FEE.add(perMinute(minutes)).multiply(STUDENT_SHARE);
    } else if (plan.equals("monthly")) {
        if (minutes > MONTHLY_FREE_MINUTES) {
            price = perMinute(minutes - MONTHLY_FREE_MINUTES);
        }
    }
    if (ridesToday.getOrDefault(email, 0) > PAID_UNLOCKS_PER_DAY && !plan.equals("monthly")) {
        price = price.subtract(UNLOCK_FEE);
    }
    if (night) {
        price = price.add(NIGHT_SURCHARGE);
    }
    return price;
}

public BigDecimal finishRide(String email, String plan, int minutes, boolean night) {
    BigDecimal price = price(email, plan, minutes, night);
    ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
    revenue = revenue.add(price);
    receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
    return price;
}

Answer

All nine pass: the counter is the same, it is just somewhere else

fourthRideUnlockIsFree() and studentFourthRide() fail

Only fourthRideUnlockIsFree() fails

Check

More precisely, if you are curious

The tests

FAIL  fourthRideUnlockIsFree(): expected: <2.00> but was: <3.00>
FAIL  studentFourthRide(): expected: <1.40> but was: <2.40>
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentRide()
Tests: 9, passed: 7, failed: 2

See it

Step 4, second try

The red points at the last step. You do not look for the bug in the whole class: you undo the step and do it again. The order of the side effects was a hidden part of the behavior, and only the tests knew about it.

price works out which ride of the day this will be:

public BigDecimal price(String email, String plan, int minutes, boolean night) {
    int rideOfDay = ridesToday.getOrDefault(email, 0) + 1;
    BigDecimal price = BigDecimal.ZERO;
    if (plan.equals("payg")) {
        price = UNLOCK_FEE.add(perMinute(minutes));
    } else if (plan.equals("student")) {
        price = UNLOCK_FEE.add(perMinute(minutes)).multiply(STUDENT_SHARE);
    } else if (plan.equals("monthly")) {
        if (minutes > MONTHLY_FREE_MINUTES) {
            price = perMinute(minutes - MONTHLY_FREE_MINUTES);
        }
    }
    if (rideOfDay > PAID_UNLOCKS_PER_DAY && !plan.equals("monthly")) {
        price = price.subtract(UNLOCK_FEE);
    }
    if (night) {
        price = price.add(NIGHT_SURCHARGE);
    }
    return price;
}

public BigDecimal finishRide(String email, String plan, int minutes, boolean night) {
    BigDecimal price = price(email, plan, minutes, night);
    ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
    revenue = revenue.add(price);
    receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
    return price;
}

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

The tests are not touched again: they call finishRide, and it does the same as before.

The whole class after step 4

class RideBilling {
    private static final BigDecimal UNLOCK_FEE = new BigDecimal("1.00");
    private static final BigDecimal PER_MINUTE = new BigDecimal("0.20");
    private static final BigDecimal STUDENT_SHARE = new BigDecimal("0.80");
    private static final int MONTHLY_FREE_MINUTES = 45;
    private static final int PAID_UNLOCKS_PER_DAY = 3;
    private static final BigDecimal NIGHT_SURCHARGE = new BigDecimal("0.50");

    private final Map<String, Integer> ridesToday = new HashMap<>();
    private BigDecimal revenue = BigDecimal.ZERO;
    private final List<String> receipts = new ArrayList<>();

    private static BigDecimal perMinute(int minutes) {
        return PER_MINUTE.multiply(BigDecimal.valueOf(minutes));
    }

    public BigDecimal price(String email, String plan, int minutes, boolean night) {
        int rideOfDay = ridesToday.getOrDefault(email, 0) + 1;
        BigDecimal price = BigDecimal.ZERO;
        if (plan.equals("payg")) {
            price = UNLOCK_FEE.add(perMinute(minutes));
        } else if (plan.equals("student")) {
            price = UNLOCK_FEE.add(perMinute(minutes)).multiply(STUDENT_SHARE);
        } else if (plan.equals("monthly")) {
            if (minutes > MONTHLY_FREE_MINUTES) {
                price = perMinute(minutes - MONTHLY_FREE_MINUTES);
            }
        }
        if (rideOfDay > PAID_UNLOCKS_PER_DAY && !plan.equals("monthly")) {
            price = price.subtract(UNLOCK_FEE);
        }
        if (night) {
            price = price.add(NIGHT_SURCHARGE);
        }
        return price;
    }

    public BigDecimal finishRide(String email, String plan, int minutes, boolean night) {
        BigDecimal price = price(email, plan, minutes, night);
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        revenue = revenue.add(price);
        receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
        return price;
    }

    public List<String> receipts() {
        return receipts;
    }

    public BigDecimal revenue() {
        return revenue;
    }
}

See it

Step 5: the plans

What is left is the chain of if on the text of the plan (long if chain). Every new plan is a new branch, and "monthly" is checked in two places. Each plan becomes a class that knows its price and its discount for the fourth ride. This is Replace Conditional with Polymorphism from the refactoring lesson:

interface Plan {
    BigDecimal price(int minutes);

    BigDecimal loyaltyDiscount();
}

class PayAsYouGo implements Plan {
    @Override
    public BigDecimal price(int minutes) {
        return RideBilling.UNLOCK_FEE.add(RideBilling.perMinute(minutes));
    }

    @Override
    public BigDecimal loyaltyDiscount() {
        return RideBilling.UNLOCK_FEE;
    }
}

class Student implements Plan {
    private static final BigDecimal STUDENT_SHARE = new BigDecimal("0.80");

    @Override
    public BigDecimal price(int minutes) {
        return new PayAsYouGo().price(minutes).multiply(STUDENT_SHARE);
    }

    @Override
    public BigDecimal loyaltyDiscount() {
        // As it is today: the full €1, even though a student pays €0.80 to unlock.
        return RideBilling.UNLOCK_FEE;
    }
}

class Monthly implements Plan {
    private static final int FREE_MINUTES = 45;

    @Override
    public BigDecimal price(int minutes) {
        return RideBilling.perMinute(Math.max(0, minutes - FREE_MINUTES));
    }

    @Override
    public BigDecimal loyaltyDiscount() {
        return BigDecimal.ZERO;
    }
}

The check for the monthly pass disappears: it simply returns a discount of 0. The student's odd behavior now sits on one line, with a comment that says why it is so (the good comment from the lesson on comments).

class RideBilling {
    static final BigDecimal UNLOCK_FEE = new BigDecimal("1.00");
    static final BigDecimal PER_MINUTE = new BigDecimal("0.20");
    private static final int PAID_UNLOCKS_PER_DAY = 3;
    private static final BigDecimal NIGHT_SURCHARGE = new BigDecimal("0.50");

    private final Map<String, Integer> ridesToday = new HashMap<>();
    private BigDecimal revenue = BigDecimal.ZERO;
    private final List<String> receipts = new ArrayList<>();

    static BigDecimal perMinute(int minutes) {
        return PER_MINUTE.multiply(BigDecimal.valueOf(minutes));
    }

    public BigDecimal price(String email, Plan plan, int minutes, boolean night) {
        int rideOfDay = ridesToday.getOrDefault(email, 0) + 1;
        BigDecimal price = plan.price(minutes);
        if (rideOfDay > PAID_UNLOCKS_PER_DAY) {
            price = price.subtract(plan.loyaltyDiscount());
        }
        if (night) {
            price = price.add(NIGHT_SURCHARGE);
        }
        return price;
    }

    public BigDecimal finishRide(String email, Plan plan, int minutes, boolean night) {
        BigDecimal price = price(email, plan, minutes, night);
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        revenue = revenue.add(price);
        receipts.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
        return price;
    }

    public List<String> receipts() {
        return receipts;
    }

    public BigDecimal revenue() {
        return revenue;
    }
}

The student share and the free minutes move into the plan that uses them. The unlock fee and the price per minute are shared by several plans, so they stay in the billing class and are no longer private.

In the tests the calls pass an object instead of text:

assertEquals("3.50", ride("payg", 10, NIGHT));
assertEquals("3.50", ride(new PayAsYouGo(), 10, NIGHT));

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

This is Strategy from the patterns module: the rule is a separate object, and billing only asks.

See it

The two changes now

The business plan is a new class. The billing class is not touched (open for extension):

class Business implements Plan {
    private static final BigDecimal PER_MINUTE = new BigDecimal("0.18");

    @Override
    public BigDecimal price(int minutes) {
        return PER_MINUTE.multiply(BigDecimal.valueOf(minutes));
    }

    @Override
    public BigDecimal loyaltyDiscount() {
        return BigDecimal.ZERO;
    }
}

The price before the end is the price question. However many times you call it, nothing is counted and no receipt goes out:

public class Main {
    static final boolean DAY = false;
    static final boolean NIGHT = true;

    public static void main(String[] args) {
        RideBilling billing = new RideBilling();
        Plan payg = new PayAsYouGo();
        billing.finishRide("ana@example.com", payg, 10, DAY);
        billing.finishRide("ana@example.com", payg, 5, DAY);
        billing.finishRide("ana@example.com", payg, 12, NIGHT);
        System.out.println("Price before the end: " + String.format("€%.2f", billing.price("ana@example.com", payg, 8, DAY)));
        System.out.println("Price before the end: " + String.format("€%.2f", billing.price("ana@example.com", payg, 8, DAY)));
        System.out.println("Charged: " + String.format("€%.2f", billing.finishRide("ana@example.com", payg, 8, DAY)));
        System.out.println("Company, 12 min: " + String.format("€%.2f", billing.finishRide("office@example.com", new Business(), 12, DAY)));
        System.out.println("Receipts: " + billing.receipts().size());
    }
}

Output

Price before the end: €1.60
Price before the end: €1.60
Charged: €1.60
Company, 12 min: €2.16
Receipts: 5

Five receipts: three rides before the preview, the fourth one and the company's. The two previews leave no trace, and they show exactly what is charged afterwards.

The idea

Before and after

Smell	Step	Lesson
nothing guards the behavior	0: tests that pin today's behavior	the safety net
one-letter names, calc that does not say it sends	1: Rename	names
true in the call	1: an argument that says what it means	functions
magic numbers	2: constants with names	names
a comment that lies	2: deleted	comments
money in a double	3: a type for money	smells
a question with a hidden side effect, a long method	4: Extract Method, command and question	functions, refactoring
a chain of if on the plan as text	5: Replace Conditional with Polymorphism	smells, refactoring, SOLID

The class got longer, not shorter. But every rule is in one place and has a name, and each change from the start is one class.

The day of rides, run with the class from before step 1 and with the class after step 5, prints the same thing line by line. Here both programs run on every check of the site.

Before

RideMgr rides = new RideMgr();
rides.calc("ana@example.com", "payg", 10, false);
rides.calc("ivan@example.com", "student", 25, false);
rides.calc("ana@example.com", "payg", 5, false);
rides.calc("maria@example.com", "monthly", 60, true);
rides.calc("ana@example.com", "payg", 12, true);
rides.calc("ana@example.com", "payg", 8, false);

for (String receipt : rides.sent()) {
    System.out.println(receipt);
}
System.out.println("Revenue: " + String.format("€%.2f", rides.day()));

After

public class Main {
    static final boolean DAY = false;
    static final boolean NIGHT = true;

    public static void main(String[] args) {
        RideBilling billing = new RideBilling();
        Plan payg = new PayAsYouGo();
        billing.finishRide("ana@example.com", payg, 10, DAY);
        billing.finishRide("ivan@example.com", new Student(), 25, DAY);
        billing.finishRide("ana@example.com", payg, 5, DAY);
        billing.finishRide("maria@example.com", new Monthly(), 60, NIGHT);
        billing.finishRide("ana@example.com", payg, 12, NIGHT);
        billing.finishRide("ana@example.com", payg, 8, DAY);

        for (String receipt : billing.receipts()) {
            System.out.println(receipt);
        }
        System.out.println("Revenue: " + String.format("€%.2f", billing.revenue()));
    }
}

Output

ana@example.com: 10 min, €3.00
ivan@example.com: 25 min, €4.80
ana@example.com: 5 min, €2.00
maria@example.com: 60 min, €3.50
ana@example.com: 12 min, €3.90
ana@example.com: 8 min, €1.60
Revenue: €18.80

The idea

Where to stop

You stop when the changes that are coming become easy. Here the two changes are one class and one line. That is why some steps are deliberately not done:

• the receipt in its own class. The text of the receipt is a second reason for the class to change (one reason to change). Today it does not change. As soon as marketing starts editing it every month, it becomes a separate class (task 1);
• sending behind an interface. The receipts are a list, not a real email. When the real email arrives, the test must not send it, and that is when the abstraction comes (depending on an abstraction);
• the student's behavior. That is a change in behavior, not a cleanup. It is done separately, with its own test, when the product decides.

And the rule from the refactoring lesson: either you change the shape or the behavior, never both in one step.

Exercise

Carry on yourself

Solve the tasks in your editor, starting from the class after step 5. After each step, run the nine tests.

1. The receipt in its own class. Move the text of the receipt and the list of sent receipts into a ReceiptBook class. The billing class only tells it "send". The tests are not touched.

One possible solution

class ReceiptBook {
    private final List<String> sent = new ArrayList<>();

    public void send(String email, int minutes, BigDecimal price) {
        sent.add(email + ": " + minutes + " min, €" + String.format("%.2f", price));
    }

    public List<String> sent() {
        return sent;
    }
}

class RideBilling {
    static final BigDecimal UNLOCK_FEE = new BigDecimal("1.00");
    static final BigDecimal PER_MINUTE = new BigDecimal("0.20");
    private static final int PAID_UNLOCKS_PER_DAY = 3;
    private static final BigDecimal NIGHT_SURCHARGE = new BigDecimal("0.50");

    private final Map<String, Integer> ridesToday = new HashMap<>();
    private BigDecimal revenue = BigDecimal.ZERO;
    private final ReceiptBook receipts = new ReceiptBook();

    static BigDecimal perMinute(int minutes) {
        return PER_MINUTE.multiply(BigDecimal.valueOf(minutes));
    }

    public BigDecimal price(String email, Plan plan, int minutes, boolean night) {
        int rideOfDay = ridesToday.getOrDefault(email, 0) + 1;
        BigDecimal price = plan.price(minutes);
        if (rideOfDay > PAID_UNLOCKS_PER_DAY) {
            price = price.subtract(plan.loyaltyDiscount());
        }
        if (night) {
            price = price.add(NIGHT_SURCHARGE);
        }
        return price;
    }

    public BigDecimal finishRide(String email, Plan plan, int minutes, boolean night) {
        BigDecimal price = price(email, plan, minutes, night);
        ridesToday.put(email, ridesToday.getOrDefault(email, 0) + 1);
        revenue = revenue.add(price);
        receipts.send(email, minutes, price);
        return price;
    }

    public List<String> receipts() {
        return receipts.sent();
    }

    public BigDecimal revenue() {
        return revenue;
    }
}

The tests

PASS  fourthRideUnlockIsFree()
PASS  monthlyOverFreeMinutes()
PASS  monthlyWithinFreeMinutes()
PASS  nightSurcharge()
PASS  payAsYouGoRide()
PASS  receipt()
PASS  revenue()
PASS  studentFourthRide()
PASS  studentRide()
Tests: 9, passed: 9, failed: 0

The program for the day from the "Before and after" card prints the same output with this class.

2. The plan from text. The plan comes from the database as text ("payg"). Write the table that turns it into an object. What did the old class do with an unknown plan? Pin it with a test on the old class before you decide what the new one should do.

3. Night from the hour. The app no longer knows when it is night; it passes the start time. The rule "from 22:00 to 6:00" has to live in one place. Write the tests for 21:59, 22:00, 5:59 and 6:00 first.

4. The student. The product has decided: on the fourth ride a student gets €0.80, not €1. That is a change in behavior: first change the expected value in the test and see it red, then change one line in the plan.

Test

Check yourself

1. The test for a student on the fourth ride expects €1.40, and the "right" answer looks like €1.60. What do you do while you clean up the class?

Change the expected value to 1.60 and fix the code in the same stepLeave the test as it is, with a comment, and ask the product owner. If they decide otherwise, that is a separate change with its own testDelete the test, because it pins a bug

2. The comment says €0.15 per minute, and the code charges €0.20. Which one do you trust?

The comment: it is the documentationWhichever is newerThe code: the tests pinned what it does, and customers have paid by it for years

3. Why do the tests call the class through one helper and not directly?

When the shape of the class changes, one place changes, and the expected values stay the sameIt makes them fasterBecause the test framework requires it

4. The first try at step 4 turned two tests red. What do you do?

Change the expected values in the two testsUndo the step and do it again: the red points at it, because everything was green before itLook for the bug in the whole class

5. After the cleanup, the business plan is one new class, and the billing class is not touched. Which principle is this?

Depending on an abstractionOne reason to changeOpen for extension, closed for modification

Check

Start the lesson over

Related lessons

• Builds on: Code smells: long method, large class, duplication, primitives instead of types, long if chains · Tests as a safety net · Refactoring in small steps · SOLID without dogma
• Tasks on this topic: Clean code and refactoring