Code Smell

Before you begin: Functions, Boolean expressions, and return values.

A code smell is a noticeable feature of code that suggests a design problem worth investigating. A long method, a repeated calculation, or a class full of unrelated operations might be a smell. None of those observations proves that the program is wrong. 1

Think of “this smells” as the beginning of a question: what difficulty is this code causing? Without that question, the term becomes a fancy way of saying “I would have written it differently.”

Two copies of one shipping rule

A shop ships an order when it is paid, contains at least one item, and has not been cancelled. One function displays a status label. Another decides whether to queue the order for shipping.

Original version

public static class Shipping
{
    public static string Label(bool paid, int itemCount, bool cancelled)
    {
        if (paid && itemCount > 0 && !cancelled)
        {
            return "Ready";
        }
        return "Hold";
    }

    public static bool ShouldQueue(bool paid, int itemCount, bool cancelled)
    {
        return paid && itemCount > 0 && !cancelled;
    }
}

Console.WriteLine(Shipping.Label(true, 2, false));       // Ready
Console.WriteLine(Shipping.ShouldQueue(true, 2, false)); // True

The same expression appears in Label and ShouldQueue. That repetition is the clue. Both methods need the same answer to “may this order ship?”

def shipping_label(paid, item_count, cancelled):
    if paid and item_count > 0 and not cancelled:
        return "Ready"
    return "Hold"


def should_queue(paid, item_count, cancelled):
    return paid and item_count > 0 and not cancelled

print(shipping_label(True, 2, False))  # Ready
print(should_queue(True, 2, False))    # True

The same expression appears in shipping_label and should_queue. That repetition is the clue. Both functions need the same answer to “may this order ship?”

Now suppose orders awaiting an age check must be held. There are two expressions to update. Forget one, and the screen could say “Ready” while the queue refuses the order—or the queue could send an order the screen correctly says to hold.

That is the concrete concern. The problem is not the existence of a Boolean expression; it is having two separately maintained copies of one decision.

One possible correction

We can give the decision a name and let both functions ask for it. For now, keep the existing rule unchanged; adding an age check would be a separate feature change.

public static class Shipping
{
    public static bool CanShip(bool paid, int itemCount, bool cancelled)
    {
        return paid && itemCount > 0 && !cancelled;
    }

    public static string Label(bool paid, int itemCount, bool cancelled)
    {
        if (CanShip(paid, itemCount, cancelled))
        {
            return "Ready";
        }
        return "Hold";
    }

    public static bool ShouldQueue(bool paid, int itemCount, bool cancelled)
    {
        return CanShip(paid, itemCount, cancelled);
    }
}

CanShip now contains the condition. The other methods still produce the same labels and queue decisions, but a change to eligibility has one place to go.

def can_ship(paid, item_count, cancelled):
    return paid and item_count > 0 and not cancelled


def shipping_label(paid, item_count, cancelled):
    if can_ship(paid, item_count, cancelled):
        return "Ready"
    return "Hold"


def should_queue(paid, item_count, cancelled):
    return can_ship(paid, item_count, cancelled)

can_ship now contains the condition. The other functions still produce the same labels and queue decisions, but a change to eligibility has one place to go.

A clue still needs interpretation

The repeated rule gives us a reason for this extraction. A condition used once, already obvious, and unlikely to change might be clearer left where it is.

The same distinction applies to other smells. A long function might be one coherent algorithm. A short function might combine three unrelated jobs. A class containing only data might be a perfectly good message passed between programs. Investigate what the shape means in its actual setting.

In a review, “these two copies can disagree when we change shipping eligibility” is useful feedback. “This violates a smell” is not. The first statement identifies a failure you can discuss, test, and prevent; the second merely invokes a label.

Sources

Martin Fowler — Code Smell, on visible clues that require further investigation.
Martin Fowler — Extract Function, the correction used in this original shipping example.