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:

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Checkout: €{checkout_total(100, True):.2f}")
print(f"Invoice: €{invoice_total(100, True):.2f}")

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: invoice_total 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

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


print(f"Checkout: €{checkout_total(100, True):.2f}")
print(f"Invoice: €{invoice_total(100, True):.2f}")

Step by step, in words

1. invoice_total started as a copy-paste of checkout_total: 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 checkout_total, 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:

def order_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Checkout: €{order_total(100, True):.2f}")
print(f"Invoice: €{order_total(100, True):.2f}")

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 function takes an order and does everything:

def place_order(email, subtotal, city):
    # check
    if "@" not in email:
        print("Invalid email: " + email)
        return
    if subtotal <= 0:
        print("Empty cart")
        return
    # total
    delivery = 3.90 if city == "Sofia" else 5.90
    if subtotal >= 50:
        delivery = 0
    total = subtotal + delivery
    # notification
    text = f"Your order for €{total:.2f} is accepted."
    print("To " + email + ": " + text)
    # log
    print(f"[log] order {email} {total:.2f}")

place_order("ana@example.com", 34, "Plovdiv")
place_order("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:

def problem_with(email, subtotal):
    if "@" not in email:
        return "Invalid email: " + email
    if subtotal <= 0:
        return "Empty cart"
    return None


def delivery_fee(subtotal, city):
    if subtotal >= 50:
        return 0
    return 3.90 if city == "Sofia" else 5.90


def notify_customer(email, total):
    text = f"Your order for €{total:.2f} is accepted."
    print("To " + email + ": " + text)


def log(email, total):
    print(f"[log] order {email} {total:.2f}")

def place_order(email, subtotal, city):
    problem = problem_with(email, subtotal)
    if problem is not None:
        print(problem)
        return
    total = subtotal + delivery_fee(subtotal, city)
    notify_customer(email, total)
    log(email, total)

place_order("ana@example.com", 34, "Plovdiv")
place_order("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 function 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:

@dataclass
class Order:
    customer_name: str = ""
    customer_email: str = ""
    customer_phone: str = ""
    delivery_city: str = ""
    delivery_street: str = ""
    delivery_postcode: str = ""
    card_number: str = ""
    card_expiry: str = ""
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery_city = "Varna"
print("Delivery to " + order.delivery_city)

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:

@dataclass
class Customer:
    name: str
    email: str
    phone: str


@dataclass
class Address:
    city: str
    street: str
    postcode: str


@dataclass
class Card:
    number: str
    expiry: str


@dataclass
class Order:
    customer: Customer | None = None
    delivery: Address | None = None
    card: Card | None = None
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery = Address("Varna", "5 Morska St", "9000")
print("Delivery to " + order.delivery.city)

Output

Delivery to Varna

@dataclass writes __init__, __repr__ and __eq__ from the fields. The address is now one value: it is passed, compared and checked in one place. Address | None says there may be no address at all.

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?

total = 0
for _ in range(3):
    total += 0.10
print(total == 0.30)
print(total)

Answer

False / 0.30000000000000004

True / 0.30

True / 0.3

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 float to keep things short. In real code:

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal("0.10")
print(total == Decimal("0.30"))
print(total)

Output

True
0.30

Decimal from the decimal module keeps decimal digits exactly. It is created from a string: Decimal(0.10) takes the already wrong float and keeps the error.

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal()
print(total == Decimal("0.30"))
print(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:

import re
from dataclasses import dataclass


@dataclass(frozen=True)
class Phone:
    value: str

    def __post_init__(self):
        digits = re.sub(r"[ ()-]", "", self.value)
        if digits.startswith("0"):
            digits = "+359" + digits[1:]
        if not re.fullmatch(r"\+359\d{9}", digits):
            raise ValueError("Invalid phone: " + self.value)
        object.__setattr__(self, "value", digits)

a = Phone("0888 123 456")
b = Phone("+359 888 123 456")
print(a.value)
print("same number" if a == b else "different numbers")
try:
    Phone("0888 12")
except ValueError as e:
    print(e)

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.

frozen=True forbids changes after creation. That is why the clean number is stored with object.__setattr__ in __post_init__, the one place where this is acceptable. == comes from @dataclass.

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

1. if digits.startswith("0"):↑↓
2. object.__setattr__(self, "value", digits)↑↓
3. digits = re.sub(r"[ ()-]", "", self.value)↑↓
4. raise ValueError("Invalid phone: " + self.value)↑↓
5. if not re.fullmatch(r"\+359\d{9}", digits):↑↓
6. digits = "+359" + digits[1:]↑↓

Check

Try it

Find the bug

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

opted_out = ["+359888123456"]
notify_shipped("0888 123 456", opted_out)
notify_shipped("0877 555 010", opted_out)

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.

1def notify_shipped(phone, opted_out):2 if phone in opted_out:3 return4 print("SMS to " + phone + ": Your order is on its way.")

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:

def price(kind):
    if kind == "courier":
        return 5.90
    elif kind == "office":
        return 3.90
    elif kind == "locker":
        return 2.90
    raise ValueError("Unknown delivery type: " + kind)


def days(kind):
    if kind == "courier":
        return 1
    elif kind == "office":
        return 2
    elif kind == "locker":
        return 3
    raise ValueError("Unknown delivery type: " + kind)

for kind in ["courier", "office", "locker"]:
    print(f"{kind}: €{price(kind):.2f}, days: {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:

@dataclass(frozen=True)
class Delivery:
    price: float
    days: int


DELIVERY = {
    "courier": Delivery(5.90, 1),
    "office": Delivery(3.90, 2),
    "locker": Delivery(2.90, 3),
}

for kind in ["courier", "office", "locker"]:
    d = DELIVERY[kind]
    print(f"{kind}: €{d.price:.2f}, days: {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 or function: 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:

def send_receipt(channel, contact, amount):
    if channel == "sms":
        text = f"You paid €{amount:.2f}. Thank you!"
        print("SMS to " + contact.replace(" ", "") + ": " + text)
    elif channel == "email":
        text = f"You paid €{amount:.2f}. Thank you!"
        print("Email to " + contact.strip().lower() + ": " + text)
    elif channel == "viber":
        text = f"You paid €{amount:.2f}. Thank you!"
        print("Viber to " + contact.replace(" ", "") + ": " + text)

send_receipt("sms", "0888 123 456", 24.5)
send_receipt("email", " Ana@Example.com ", 24.5)
send_receipt("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 printDecimal, 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