Code smells: long method, large class, duplication, primitives instead of types, long if chains

Code in:JavaC#PythonJavaScript

In short: A code smell is a sign that the next change will breed a bug: a long method, a large class, duplication, primitives instead of types and a long chain of ifs.

Problem

The checkout says €90, the invoice says €95

The order total is computed in two places: at the checkout and in the invoice. A month ago the VIP discount became 10%. The developer fixed the checkout:

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Checkout: €%.2f%n", checkoutTotal(100, true));
System.out.printf("Invoice: €%.2f%n", invoiceTotal(100, true));

Output

Checkout: €90.00
Invoice: €95.00

The customer pays €90, and the invoice says €95. Accounting finds a €5 difference in every VIP order for the past month. No line here is "wrong" by itself: invoiceTotal does exactly what it was written to do. The problem is that one rule lives in two places.

See it

How two copies drift apart

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Checkout: €%.2f%n", checkoutTotal(100, true));
System.out.printf("Invoice: €%.2f%n", invoiceTotal(100, true));

Step by step, in words

1. invoiceTotal started as a copy-paste of checkoutTotal: the same three lines, both with a 5% VIP discount.
2. The VIP discount becomes 10%. The bug report came from the checkout, so the fix goes into checkoutTotal, line 2.
3. Nobody remembers that the invoice is a copy. Line 8 keeps 0.05: the two copies now say different things.
4. A €100 order of a VIP customer goes through the checkout: 100 − 10 + 0 = €90.
5. The same order goes into the invoice: 100 − 5 + 0 = €95.
6. The customer pays €90, and the invoice says €95. One rule in two places: you fix one, the other stays behind.

On the day of the copy-paste both functions were right. The bug was born later, with a fix that was made honestly, but in one place. Code that is right today but will cause a bug with the next change has a code smell. The term comes from Kent Beck and Martin Fowler: a smell is not an error, but a sign that something near it will soon go wrong.

This lesson is about the five most common smells and how to notice them before they cost you €5 an order.

The idea

Duplication: one rule, one place

The rule for the total becomes one function, and the checkout and the invoice both call it:

static double orderTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Checkout: €%.2f%n", orderTotal(100, true));
System.out.printf("Invoice: €%.2f%n", orderTotal(100, true));

Output

Checkout: €90.00
Invoice: €90.00

Now you cannot fix one and forget the other: there is no other.

How you smell it. You fix a bug and ask yourself: "Was this copied somewhere?". You search for a distinctive number or expression (0.05) and find it in two places. Two functions with almost the same names (checkout…, invoice…) and almost the same bodies.

Careful. Not everything that looks the same is duplication. Free delivery from €50 and returns within 50 days are two rules with one number. Merge them into one constant, and changing one changes the other. Duplication is one rule in two places. Many teams wait for the third copy before they pull out the common part: two can be a coincidence, three rarely are.

See it

Long method

The method takes an order and does everything:

static void placeOrder(String email, double subtotal, String city) {
    // check
    if (!email.contains("@")) {
        System.out.println("Invalid email: " + email);
        return;
    }
    if (subtotal <= 0) {
        System.out.println("Empty cart");
        return;
    }
    // total
    double delivery = city.equals("Sofia") ? 3.90 : 5.90;
    if (subtotal >= 50) {
        delivery = 0;
    }
    double total = subtotal + delivery;
    // notification
    String text = String.format("Your order for €%.2f is accepted.", total);
    System.out.println("To " + email + ": " + text);
    // log
    System.out.printf("[log] order %s %.2f%n", email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Output

To ana@example.com: Your order for €39.90 is accepted.
[log] order ana@example.com 39.90
Invalid email: ana.example.com

How you smell it: you scroll to see the end; comments work as headings for parts ("check", "total"); you cannot say what it does without saying "and"; a variable declared at the top is used only at the bottom. Every heading comment is the name of a function waiting to happen (see the lesson Comments that lie).

Each part goes into its own function, named after its comment:

static String problemWith(String email, double subtotal) {
    if (!email.contains("@")) {
        return "Invalid email: " + email;
    }
    if (subtotal <= 0) {
        return "Empty cart";
    }
    return null;
}

static double deliveryFee(double subtotal, String city) {
    if (subtotal >= 50) {
        return 0;
    }
    return city.equals("Sofia") ? 3.90 : 5.90;
}

static void notifyCustomer(String email, double total) {
    String text = String.format("Your order for €%.2f is accepted.", total);
    System.out.println("To " + email + ": " + text);
}

static void log(String email, double total) {
    System.out.printf("[log] order %s %.2f%n", email, total);
}

static void placeOrder(String email, double subtotal, String city) {
    String problem = problemWith(email, subtotal);
    if (problem != null) {
        System.out.println(problem);
        return;
    }
    double total = subtotal + deliveryFee(subtotal, city);
    notifyCustomer(email, total);
    log(email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Output

To ana@example.com: Your order for €39.90 is accepted.
[log] order ana@example.com 39.90
Invalid email: ana.example.com

The output is the same, letter for letter. The main method now reads like a list of steps, and the delivery fee can be tested on its own. This step is called Extract Method, and you will do it in small steps in the lesson on refactoring.

See it

Large class

The order has collected everything anyone ever needed:

class Order {
    String customerName;
    String customerEmail;
    String customerPhone;
    String deliveryCity;
    String deliveryStreet;
    String deliveryPostcode;
    String cardNumber;
    String cardExpiry;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.deliveryCity = "Varna";
System.out.println("Delivery to " + order.deliveryCity);

Output

Delivery to Varna

How you smell it: groups of fields with a common prefix (customer…, delivery…, card…); methods that use only one group; a name like OrderManager or Utils that says nothing; every task on the team touches this file, and the git merge conflicts are always in it.

Every group with a common prefix is a type waiting for a name:

record Customer(String name, String email, String phone) {}

record Address(String city, String street, String postcode) {}

record Card(String number, String expiry) {}

class Order {
    Customer customer;
    Address delivery;
    Card card;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.delivery = new Address("Varna", "5 Morska St", "9000");
System.out.println("Delivery to " + order.delivery.city());

Output

Delivery to Varna

A record writes the fields, the constructor and the accessors. The address is now one value: it is passed, compared and checked in one place.

The rest (smsSent, emailSent) is the next group. You do not have to fix everything at once.

Try it

Predict the output

Three products at €0.10 each. What does the program print?

double total = 0;
for (int i = 0; i < 3; i++) {
    total += 0.10;
}
System.out.println(total == 0.30);
System.out.println(total);

Answer

true / 0.3

false / 0.30000000000000004

true / 0.30

Check

0.10 has no exact binary form, just as 1/3 has no exact decimal form. A double keeps the nearest binary number, and the small errors add up.

See it

Primitives instead of types: money

Money in a double is the most common form of the smell primitive obsession: a value with its own rules lives in a type that does not know those rules. So far in the course the amounts were double to keep things short. In real code:

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal("0.10"));
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Output

true
0.30

BigDecimal keeps decimal digits exactly. It is created from a string: new BigDecimal(0.10) takes the already wrong double and keeps the error. It is compared with compareTo, because equals treats 0.30 and 0.3 as different.

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal());
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Check

More precisely, if you are curious

Better still is a Money type that keeps the amount and the currency together and does not let you add euros to dollars by mistake. It is the same idea as the phone on the next card.

See it

Primitives instead of types: the phone

A phone number as a string is another form of the same smell. "0888 123 456", "+359 888 123 456" and "+359888123456" are one number, but three different strings. Every place that compares phones has to remember on its own how to clean them. The Phone type cleans and checks once, when it is created:

record Phone(String value) {
    Phone {
        String digits = value.replaceAll("[ ()-]", "");
        if (digits.startsWith("0")) {
            digits = "+359" + digits.substring(1);
        }
        if (!digits.matches("\\+359\\d{9}")) {
            throw new IllegalArgumentException("Invalid phone: " + value);
        }
        value = digits;
    }
}

Phone a = new Phone("0888 123 456");
Phone b = new Phone("+359 888 123 456");
System.out.println(a.value());
System.out.println(a.equals(b) ? "same number" : "different numbers");
try {
    new Phone("0888 12");
} catch (IllegalArgumentException e) {
    System.out.println(e.getMessage());
}

Output

+359888123456
same number
Invalid phone: 0888 12

The regular expression [ ()-] removes spaces, brackets and dashes, and \+359\d{9} checks that +359 is followed by exactly nine digits.

An invalid phone cannot exist: the constructor does not let it through. Every Phone in the program is already clean.

The compact constructor of a record gets value before it is stored and can change it. equals comes from the record and compares the clean values.

Put the lines of the constructor in order so that "0888 123 456" becomes "+359888123456":

1. if (digits.startsWith("0")) {↑↓
2. value = digits;↑↓
3. String digits = value.replaceAll("[ ()-]", "");↑↓
4. }↑↓
5. throw new IllegalArgumentException("Invalid phone: " + value);↑↓
6. if (!digits.matches("\\+359\\d{9}")) {↑↓
7. digits = "+359" + digits.substring(1);↑↓
8. }↑↓

Check

Try it

Find the bug

The customer has unsubscribed from SMS messages, and the shop still texts them about every parcel:

List<String> optedOut = List.of("+359888123456");
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

Output

SMS to 0888 123 456: Your order is on its way.
SMS to 0877 555 010: Your order is on its way.

Which line has the bug?

Pick the line with the bug.

1static void notifyShipped(String phone, List<String> optedOut) {2 if (optedOut.contains(phone)) {3 return;4 }5 System.out.println("SMS to " + phone + ": Your order is on its way.");6}

Check

See it

Long if chain on a type

The price and the delivery time depend on the delivery type. The chain is in both functions:

static double price(String kind) {
    if (kind.equals("courier")) {
        return 5.90;
    } else if (kind.equals("office")) {
        return 3.90;
    } else if (kind.equals("locker")) {
        return 2.90;
    }
    throw new IllegalArgumentException("Unknown delivery type: " + kind);
}

static int days(String kind) {
    if (kind.equals("courier")) {
        return 1;
    } else if (kind.equals("office")) {
        return 2;
    } else if (kind.equals("locker")) {
        return 3;
    }
    throw new IllegalArgumentException("Unknown delivery type: " + kind);
}

for (String kind : List.of("courier", "office", "locker")) {
    System.out.printf("%s: €%.2f, days: %d%n", kind, price(kind), days(kind));
}

Output

courier: €5.90, days: 1
office: €3.90, days: 2
locker: €2.90, days: 3

How you smell it: the same chain on the same field (kind, type, status) in several places; a new type means finding every chain; a forgotten branch shows up only when a customer picks it.

Here the types differ only by numbers. Then the chain becomes a table: type → data:

record Delivery(double price, int days) {}

static final Map<String, Delivery> DELIVERY = Map.of(
    "courier", new Delivery(5.90, 1),
    "office", new Delivery(3.90, 2),
    "locker", new Delivery(2.90, 3));

for (String kind : List.of("courier", "office", "locker")) {
    Delivery d = DELIVERY.get(kind);
    System.out.printf("%s: €%.2f, days: %d%n", kind, d.price(), d.days());
}

Output

courier: €5.90, days: 1
office: €3.90, days: 2
locker: €2.90, days: 3

A new type is a new row in the table. When the types differ by behavior, not just by numbers, each type becomes its own class: that is Strategy. If the type changes over time (the order is new, then paid, then shipped), that is State. How a chain turns into classes step by step (Replace Conditional with Polymorphism) is in the lesson on refactoring.

The idea

A smell is not a bug: when to leave it

The five smells in short:

• duplication: one rule in several places → one function or constant;
• long method: you scroll and read heading comments → Extract Method;
• large class: groups of fields with a common prefix → a class for each group;
• primitives instead of types: money in a double, a phone in a string → a type that keeps its own rules;
• long if chain on a type: the same chain in many places → a table, Strategy or State.

A smell is not a bug. It tells you what the next change will cost. So you leave it when:

• the code does not change. The report nobody has touched in three years costs nothing until you touch it;
• there are no tests. Without them, fixing the smell can break something that worked. Tests first (the next lesson), then small steps;
• two things only look the same. Two rules with one number are not duplication;
• the code is for one use: a migration script you will delete tomorrow.

The scout rule: when you are touching a file anyway, leave it a bit cleaner than you found it. Do not rewrite whole modules "just in case".

Exercise

Smell it and fix it

Solve it in your own editor.

The receipt. The function sends a receipt by SMS, email or Viber:

static void sendReceipt(String channel, String contact, double amount) {
    if (channel.equals("sms")) {
        String text = String.format("You paid €%.2f. Thank you!", amount);
        System.out.println("SMS to " + contact.replace(" ", "") + ": " + text);
    } else if (channel.equals("email")) {
        String text = String.format("You paid €%.2f. Thank you!", amount);
        System.out.println("Email to " + contact.trim().toLowerCase() + ": " + text);
    } else if (channel.equals("viber")) {
        String text = String.format("You paid €%.2f. Thank you!", amount);
        System.out.println("Viber to " + contact.replace(" ", "") + ": " + text);
    }
}

sendReceipt("sms", "0888 123 456", 24.5);
sendReceipt("email", " Ana@Example.com ", 24.5);
sendReceipt("viber", "0877 555 010", 9.9);

Output

SMS to 0888123456: You paid €24.50. Thank you!
Email to ana@example.com: You paid €24.50. Thank you!
Viber to 0877555010: You paid €9.90. Thank you!

Find the smells in it (at least three) and name them. Then fix them one at a time, running the program after each step: the output must stay the same, letter for letter.

More tasks

• A new channel. Add Telegram, with a contact like the SMS one. In how many places do you make changes before the fix, and in how many after it?
• Money in cents. Change the receipt amount to whole cents (2450), and turn it into euros only when printing.
• The large class. Finish Order from the card "Large class": move smsSent and emailSent into their own type and think about what to call it.

Test

Check yourself

1. The code has a smell. Does that mean it has a bug?

Yes, a smell is a bug by another nameYes, if the compiler gives a warningNo. The code may work today; the smell says the next change is risky

2. Free delivery starts at €50, and returns are allowed for up to 50 days. Should you make them one constant to avoid duplication?

No. These are two different rules that happen to have the same numberYes, one number should live in one placeYes, but call it FIFTY

3. Totals add up in a double, and 0.10 three times is not 0.30. What helps?

A bigger type: float instead of doubleRounding every time you printBigDecimal, created from a string

4. The same if chain on the delivery type is in the price, in the delivery time and in the label. A new type arrives. What helps?

A comment above each chain listing the typesOne more else if in all three placesA table type → data, or a class per type if the behavior differs

5. The yearly report module: 300 lines in one method, nobody has touched it in three years, no tests. Tomorrow you have to ship something else. What do you do?

Split it into functions todayDelete itLeave it. You will fix it when you need to change it, and you will write tests for it first

Check

Start the lesson over

Related lessons

• Builds on: State · Names in code · Functions: one job, few parameters, no hidden side effects · Comments that lie and comments that help
• Continues in: Tests as a safety net · Refactoring in small steps · SOLID without dogma · Exercise: a messy class, cleaned up step by step
• Tasks on this topic: Clean code and refactoring