Low-level designCore9 min

Code smells

A code smell is a surface sign that usually points to a deeper design problem: a switch repeated across methods, a method more interested in another class than its own, values that always travel together. Learn to spot them, then decide which are worth fixing.

Watch it evolve1 / 10
The problem

A groomer's booking book

GroomingBook books and prices appointments and writes reminders. Appointment holds the details.

80
Start
Stage 1
Final

1 · The situation

A pet groomer's booking book

A pet groomer takes bookings by phone and texts a reminder the day before. GroomingBook books an appointment if the slot is free, prices it by the pet's size, and writes the reminder text. Appointment holds the details. It all works, and it's the kind of code most projects contain somewhere.

The first versionJava
import java.util.ArrayList;
import java.util.List;

enum PetSize { SMALL, MEDIUM, LARGE }

class Appointment {
    private final String ownerName;
    private final String ownerPhone;
    private final String petName;
    private final PetSize size;
    private final int startMinute;

    Appointment(String ownerName, String ownerPhone, String petName, PetSize size,
            int startMinute) {
        this.ownerName = ownerName;
        this.ownerPhone = ownerPhone;
        this.petName = petName;
        this.size = size;
        this.startMinute = startMinute;
    }

    String ownerName() { return ownerName; }
    String ownerPhone() { return ownerPhone; }
    String petName() { return petName; }
    PetSize size() { return size; }
    int startMinute() { return startMinute; }
}

class GroomingBook {
    private final List<Appointment> appointments = new ArrayList<>();

    String book(String ownerName, String ownerPhone, String petName, PetSize size,
            int startMinute) {
        if (ownerPhone.length() != 12) return "Bad phone number: " + ownerPhone;
        for (Appointment a : appointments) {
            int end = a.startMinute() + minutes(a.size());
            if (startMinute < end && a.startMinute() < startMinute + minutes(size)) {
                return "Slot taken for " + petName;
            }
        }
        appointments.add(new Appointment(ownerName, ownerPhone, petName, size, startMinute));
        return "Booked " + petName + " at " + clock(startMinute) + " for $" + priceDollars(size);
    }

    int priceDollars(PetSize size) {
        switch (size) {
            case SMALL: return 40;
            case MEDIUM: return 55;
            default: return 70;
        }
    }

    int minutes(PetSize size) {
        switch (size) {
            case SMALL: return 45;
            case MEDIUM: return 60;
            default: return 90;
        }
    }

    String reminder(Appointment a) {
        return "Text " + a.ownerPhone() + ": Hi " + a.ownerName() + ", " + a.petName()
                + "'s grooming is at " + clock(a.startMinute()) + ".";
    }

    String reminderFor(String petName) {
        for (Appointment a : appointments) {
            if (a.petName().equals(petName)) return reminder(a);
        }
        return "No appointment for " + petName;
    }

    private static String clock(int minute) {
        return String.format("%d:%02d", minute / 60, minute % 60);
    }
}

public class Main {
    public static void main(String[] args) {
        GroomingBook book = new GroomingBook();
        System.out.println(book.book("Ana", "555-555-0142", "Biscuit", PetSize.SMALL, 540));
        System.out.println(book.book("Raj", "555-555-0199", "Moose", PetSize.LARGE, 570));
        System.out.println(book.book("Raj", "555-555-0199", "Moose", PetSize.LARGE, 600));
        System.out.println(book.book("Lu", "555-01", "Pip", PetSize.MEDIUM, 720));
        System.out.println(book.reminderFor("Biscuit"));
    }
}
*«enum»PetSizeSMALLMEDIUMLARGEAppointment− ownerName: String− ownerPhone: String− petName: String− size: PetSize− startMinute: int~ ownerName(): String~ ownerPhone(): String~ petName(): String~ size(): PetSize~ startMinute(): intGroomingBook− appointments: List<Appointment>~ book(String, String, String, PetSize, int): Str…~ priceDollars(PetSize): int~ minutes(PetSize): int~ reminder(Appointment): String~ reminderFor(String): String− clock(int): String

The class diagram of this version, drawn from its code: the book does the work, and the appointment holds data.

Booked Biscuit at 9:00 for $40
Slot taken for Moose
Booked Moose at 10:00 for $70
Bad phone number: 555-01
Text 555-555-0142: Hi Ana, Biscuit's grooming is at 9:00.

2 · The smells

Four signs, each pointing somewhere

Nothing here is a bug. But read it the way a reviewer would, and several things stand out. The first is a switch on a type code: PetSize is switched on in two methods, so a new size, say giant breeds, means editing both and keeping them in step.

One enum, two switchesJava
class GroomingBook {
    private final List<Appointment> appointments = new ArrayList<>();

    String book(String ownerName, String ownerPhone, String petName, PetSize size,
            int startMinute) {
        if (ownerPhone.length() != 12) return "Bad phone number: " + ownerPhone;
        for (Appointment a : appointments) {
            int end = a.startMinute() + minutes(a.size());
            if (startMinute < end && a.startMinute() < startMinute + minutes(size)) {
                return "Slot taken for " + petName;
            }
        }
        appointments.add(new Appointment(ownerName, ownerPhone, petName, size, startMinute));
        return "Booked " + petName + " at " + clock(startMinute) + " for $" + priceDollars(size);
    }

    int priceDollars(PetSize size) {
        switch (size) {
            case SMALL: return 40;
            case MEDIUM: return 55;
            default: return 70;
        }
    }

    int minutes(PetSize size) {
        switch (size) {
            case SMALL: return 45;
            case MEDIUM: return 60;
            default: return 90;
        }
    }

    String reminder(Appointment a) {
        return "Text " + a.ownerPhone() + ": Hi " + a.ownerName() + ", " + a.petName()
                + "'s grooming is at " + clock(a.startMinute()) + ".";
    }

    String reminderFor(String petName) {
        for (Appointment a : appointments) {
            if (a.petName().equals(petName)) return reminder(a);
        }
        return "No appointment for " + petName;
    }

    private static String clock(int minute) {
        return String.format("%d:%02d", minute / 60, minute % 60);
    }
}

The second is feature envy: reminder() lives in GroomingBook but uses four of Appointment's getters and almost nothing of its own class. The third explains the second: Appointment is an anemic class, data with getters and no behavior, so the logic about appointments, including whether two overlap, lives elsewhere.

A method that envies AppointmentJava
class GroomingBook {
    private final List<Appointment> appointments = new ArrayList<>();

    String book(String ownerName, String ownerPhone, String petName, PetSize size,
            int startMinute) {
        if (ownerPhone.length() != 12) return "Bad phone number: " + ownerPhone;
        for (Appointment a : appointments) {
            int end = a.startMinute() + minutes(a.size());
            if (startMinute < end && a.startMinute() < startMinute + minutes(size)) {
                return "Slot taken for " + petName;
            }
        }
        appointments.add(new Appointment(ownerName, ownerPhone, petName, size, startMinute));
        return "Booked " + petName + " at " + clock(startMinute) + " for $" + priceDollars(size);
    }

    int priceDollars(PetSize size) {
        switch (size) {
            case SMALL: return 40;
            case MEDIUM: return 55;
            default: return 70;
        }
    }

    int minutes(PetSize size) {
        switch (size) {
            case SMALL: return 45;
            case MEDIUM: return 60;
            default: return 90;
        }
    }

    String reminder(Appointment a) {
        return "Text " + a.ownerPhone() + ": Hi " + a.ownerName() + ", " + a.petName()
                + "'s grooming is at " + clock(a.startMinute()) + ".";
    }

    String reminderFor(String petName) {
        for (Appointment a : appointments) {
            if (a.petName().equals(petName)) return reminder(a);
        }
        return "No appointment for " + petName;
    }

    private static String clock(int minute) {
        return String.format("%d:%02d", minute / 60, minute % 60);
    }
}

The fourth is a data clump with primitive obsession: an owner's name and phone travel together through every signature as two loose strings, and the phone number is a bare string whose only rule, twelve characters, is checked by whoever remembers. A text-message feature that needs the owner's country code would edit every signature the pair passes through.

Owner details, traveling looseJava
class GroomingBook {
    private final List<Appointment> appointments = new ArrayList<>();

    String book(String ownerName, String ownerPhone, String petName, PetSize size,
            int startMinute) {
        if (ownerPhone.length() != 12) return "Bad phone number: " + ownerPhone;
        for (Appointment a : appointments) {
            int end = a.startMinute() + minutes(a.size());
            if (startMinute < end && a.startMinute() < startMinute + minutes(size)) {
                return "Slot taken for " + petName;
            }
        }
        appointments.add(new Appointment(ownerName, ownerPhone, petName, size, startMinute));
        return "Booked " + petName + " at " + clock(startMinute) + " for $" + priceDollars(size);
    }

    int priceDollars(PetSize size) {
        switch (size) {
            case SMALL: return 40;
            case MEDIUM: return 55;
            default: return 70;
        }
    }

    int minutes(PetSize size) {
        switch (size) {
            case SMALL: return 45;
            case MEDIUM: return 60;
            default: return 90;
        }
    }

    String reminder(Appointment a) {
        return "Text " + a.ownerPhone() + ": Hi " + a.ownerName() + ", " + a.petName()
                + "'s grooming is at " + clock(a.startMinute()) + ".";
    }

    String reminderFor(String petName) {
        for (Appointment a : appointments) {
            if (a.petName().equals(petName)) return reminder(a);
        }
        return "No appointment for " + petName;
    }

    private static String clock(int minute) {
        return String.format("%d:%02d", minute / 60, minute % 60);
    }
}

The practice checks catch three of the four: the repeated switch as a warning, and the envy and the anemic class as hints. The data clump and the primitive phone they can't judge; whether two values belong together is a design question.

3 · The fix

Follow each smell to its cause

Each smell points to a change, and each change is small. Refactoring in small steps shows how to make them safely, one at a time; here is where they lead:

Phone checks its own format when it's made, and Owner holds a name with a phone. A bad number now fails where it's created, and every signature takes one Owner. JavaScript uses a plain object for the owner.

Java
record Phone(String number) {
    Phone {
        if (number.length() != 12) {
            throw new IllegalArgumentException("Bad phone number: " + number);
        }
    }
}
*«record»Phone− number: String«record»Owner− name: String− phone: Phone«enum»PetSize~ priceDollars: int~ minutes: intSMALLMEDIUMLARGE«record»Appointment− owner: Owner− petName: String− size: PetSize− startMinute: int~ overlaps(int, PetSize): boolean~ reminder(): StringClock~ format(int): StringGroomingBook− appointments: List<Appointment>~ book(Owner, String, PetSize, int): String~ reminderFor(String): String

The program prints exactly what it printed before, and the checks find nothing:

Booked Biscuit at 9:00 for $40
Slot taken for Moose
Booked Moose at 10:00 for $70
Bad phone number: 555-01
Text 555-555-0142: Hi Ana, Biscuit's grooming is at 9:00.

Smell

What it pointed to

The change

Switch on a type code

Behavior that belongs to each size

Size carries its own price and duration

Feature envy

A method in the wrong class

Move reminder() to Appointment

Anemic class

Data separated from its rules

Move overlaps() to Appointment

Data clump

A missing concept, the owner

Introduce Owner

Primitive obsession

A value with rules, the phone

Introduce Phone, which checks itself

4 · The catalog

Seven smells worth knowing

A code smell is a surface feature that's quick to spot and usually, not always, corresponds to a deeper problem. Smells are useful because they're easy to see in review, long before the deeper problem causes a bug. These seven come up most often:

Smell

What it looks like

The usual fix

Long method

A method that needs comments to mark its sections

Extract each section into a well-named method

God class

One class that many changes touch, with many methods and collaborators

Split by reason to change

Feature envy

A method that mostly uses another class's data

Move it to that class

Primitive obsession

Strings and numbers standing in for concepts with rules

A small value type

Data clumps

The same few values passed together again and again

A type that holds them

Switch on type

The same switch on a kind or a type code in several places

Polymorphism, or data on the type

Shotgun surgery

One change that edits many classes

Gather what changes together into one place

*«record»Phone− number: String«record»Owner− name: String− phone: Phone«enum»PetSize~ priceDollars: int~ minutes: intSMALLMEDIUMLARGE«record»Appointment− owner: Owner− petName: String− size: PetSize− startMinute: int~ overlaps(int, PetSize): boolean~ reminder(): StringClock~ format(int): StringGroomingBook− appointments: List<Appointment>~ book(Owner, String, PetSize, int): String~ reminderFor(String): String

After the fix: the owner and the phone are types, and the appointment carries its own rules.

Some smells are opposites, and pushing too far from one lands in the other. Divergent change, one class edited for many unrelated reasons, is fixed by splitting; split too far and you get shotgun surgery, one change spread across many classes. A middle man, a class that only forwards, is what overdoing Law of Demeter produces.

5 · When it's fine

Smells are prompts, not rules

Leave a smell alone when:

  • The code doesn't change. A switch over a fixed set of cases that nobody edits costs nothing.
  • The fix costs more than it saves. A one-off script doesn't need value types.
  • It's at a boundary. Data transfer objects, records read from a file or an API, are meant to be plain data; calling them anemic misreads their job.
  • The smell is a trade. A long method that reads top to bottom, like a recipe, can be clearer than six tiny ones.

Smell

Usually worth fixing when

Usually fine when

Switch on type

The cases grow, and the switch is repeated

One switch over cases that never change

Anemic class

Rules about the data live in other classes

It's a data transfer object at a boundary

Primitive obsession

The value has rules or a format

It's a plain count, a name or a label

6 · Getting it right

Spot, judge, then change one thing at a time

  • Name the smell before fixing it. "Feature envy in reminder()" points to a specific move; "this is messy" doesn't.
  • Look for the deeper cause. The envy and the anemic class here were one problem: data and behavior apart.
  • Fix the smells in code that changes. The history shows which files are edited often; a smell in a file nobody touches can wait.
  • Change in small steps, with tests running. Each step should leave the program working; see Refactoring in small steps.
  • Use tools as a first pass. Checks and linters find the mechanical smells; judging whether two values belong together is still yours.

The fix for a smell can look different by language:

Language

Data on a type code

A small value type

Java

An enum with fields and a constructor

A record, with checks in its compact constructor

C++

Constants of a small struct, since enums can't hold data

A class that checks its value in the constructor

JavaScript

A frozen object of frozen objects

A class with a private field, checked in the constructor

Python

An Enum whose values are tuples, with properties

A frozen dataclass, checked in __post_init__

7 · Look-alikes

Smells, bugs and anti-patterns

Smells sit between a bug and a principle. What separates them is how sure you can be that something's wrong.

Term

What it means

Bug

The program does the wrong thing

Code smell

A local sign, quick to spot, that a design problem may be underneath

Anti-pattern

A whole design that looks like a solution and causes more problems than it solves

Principle

A rule of thumb for good design; most smells are a principle being broken

Refactoring

The change that removes a smell without changing behavior

Many smells are principles seen from the other side. A switch on type breaks Open/closed; a god class breaks Single responsibility; feature envy breaks Encapsulation; shotgun surgery is DRY broken for a rule. Knowing the principle tells you why the smell matters; knowing the smell tells you where to look.

8 · In practice

What the checks look for

The practice checks report several smells: a god class (more than 15 methods or 8 collaborators), feature envy, an anemic class, a circular dependency, a two-way link, a hierarchy four or more levels deep, and an enum switched on in several methods. The god class, the circular dependency and the repeated switch are warnings; the rest are hints.

The vending-machine draft shows one at once: its Status enum is switched on in three methods. Fix the vending machine, and see which smells the checks report as you go.

Check yourself

4 questions

1. reminder() uses four of Appointment's getters and nothing of GroomingBook. What does that suggest?
2. Which is a data clump?
3. A switch over the four seasons appears once, in a class nobody has edited in two years. Should it be refactored?
4. Splitting a god class too far into many tiny classes produces which smell?

Takeaways

Remember this

  • A code smell is a quick-to-spot sign of a possible design problem, not a bug.
  • Name the smell, find the cause it points to, then fix the cause.
  • Feature envy and an anemic class are often one problem: data and behavior apart.
  • Data clumps and primitive obsession hide missing types; give them names and rules.
  • Fix smells in code that changes, in small steps; leave stable code alone.