2. The vocabulary of what's wrong: code smells

Description

By the end of this lesson you'll have three things. First, a precise definition of what a code smell is and, above all, what it is not: it isn't an error, it isn't an opinion, and it isn't an order to refactor. Second, you'll understand the distinction that makes all the practical difference: a smell gets investigated, not reflexively corrected. That sentence sounds like a nuance and is actually the line that separates someone who diagnoses from someone who hands out refactors. And third, you'll walk away with the short, useful catalog — the twelve smells that genuinely show up — with their concrete Boletia example, ready to use in the next lesson.

This matters because the vocabulary of what's wrong carries a risk pattern vocabulary doesn't. When you learn "Strategy," the worst that can happen is you apply it where it wasn't needed — bad, but visible and reversible. When you learn "God object," the worst that can happen is you start handing the word out like a verdict, over code that might be perfectly fine. Smells are probabilistic signals, not sentences, and whoever uses them as sentences does more harm than someone who doesn't know them. That's why this lesson spends as much space on the word probably as on the catalog itself.

And there's a positive reason, more important than the defensive one. With the catalog in your head, reading someone else's code changes texture. Without it, you open a three-hundred-line file and feel a diffuse discomfort you don't know where to place. With it, you open the same file and see countable things: "here's a switch on kind I've already seen in two other files," "this method touches seven attributes of Order and none of its own," "this class imports fourteen modules." The diffuse discomfort turns into a list. And a list can be prioritized, discussed, and decided.

Connection to the module: lesson 1 showed the difference between a vague comment and a named one, but it hadn't given you the names yet. This lesson installs the concept that organizes them all — the smell as a signal — and delivers the catalog. Lesson 3 takes the three most frequent, most expensive ones and opens them up under a magnifying glass, one by one. Lesson 4 turns the name into an actionable comment. And lesson 5 goes up a level: from smells, which are local signals in the code, to anti-patterns, which are named design decisions.

What a smell does in a kitchen

You're at home and you smell gas.

Ask yourself the important question: is the smell the problem? No. The smell doesn't hurt you; it's the molecule they add to gas — thiol, a compound made to smell awful on purpose — precisely so you notice it, because natural gas has no smell at all. The problem is the leak, and the leak is invisible.

Now notice what you do with that smell. You don't replace the stove. You go check whether a burner was left open, you smell near the hose, you open a window. You investigate. And there are three possible outcomes, all three normal:

• You find the leak. The smell saved you. The repair is what fixes the problem, not the smell.
• You find something else. Someone lit the water heater and the flame went out. It wasn't a leak, but it was something worth looking at.
• You find nothing. It was the neighbor, or it was organic trash. The smell was a false positive and nothing happens: the cost of investigating was low and the cost of ignoring it would have been high.

A code smell works exactly the same way. It's an observable trait in the code that usually accompanies a structural problem, but that isn't the problem itself. Code with a smell normally works: it passes tests, serves users, charges money. The smell doesn't break anything today. What it indicates is that probably something is badly arranged that's going to cost dearly the next time someone has to change that area.

And like the smell of gas, it has legitimate false positives. An eighty-line method is normally a problem; an eighty-line method that's a flat, branchless country-code conversion table isn't. Someone who corrects reflexively splits it into four twenty-line methods and leaves the code worse. Someone who investigates looks at it, understands what it is, and goes on with their day.

There's a third parallel with gas I want you to keep, because it's the hardest to accept: the smell doesn't tell you the size of the leak. A tiny leak in a connection smells the same as a serious one. It's the same in code: a three-branch if/elif and a twenty-branch one smell the same smell — repeated switch — and have completely different consequences. Detecting the smell is the first step; measuring its cost is a separate step, and it's the one that turns a finding into a priority. Lesson 4 is going to insist on that when we talk about a comment's "consequence" part.

What a smell exactly is (and its three parts)

A code smell is a superficial, observable trait of the code that, with high statistical frequency, indicates a deeper structural problem.

That definition has three loaded words, and it's worth unpacking them one by one, because each one rules out a common misunderstanding.

Superficial. The smell shows without understanding the domain. You don't need to know what a complimentary ticket is to notice calculate_price has an if for every value of one field, nor to count that ReconciliationReport.notify_finance uses seven attributes of Order and none of its own. This is what makes the vocabulary teachable and shareable: two people looking at the same file arrive at the same finding even if one's been on the team for three years and the other for three weeks. Compare that to "this code is poorly thought out," which requires knowing the business and can't be verified.

Observable. A smell can be pointed at and, almost always, counted. It isn't "I feel like this class does a lot": it's "this class has fourteen methods, imports eleven modules, and got touched by thirteen of the last twenty PRs." That property — being countable — is what pulls it out of matter-of-taste territory. When two people argue about whether something's ugly, the argument doesn't end. When they argue about whether thirteen of twenty PRs touched the same file, the argument ends in two minutes: you open the history and count.

High frequency, not always. Here's the lesson's heart. A smell is a correlation, not an implication. It says: "in most cases where this shows up, there's a problem underneath." It doesn't say: "there's a problem here." That distinction is what authorizes — and requires — investigating before acting.

Now, the practical anatomy. Every time you use a smell in a review, you're going to be handling three pieces, and it's worth keeping them separate in your head because lesson 4 is going to explicitly ask for them:

Piece	What it is	Example at Boletia
The symptom	What's visible and countable. Verifiable by anyone	calculate_price has an if for every value of ticket.kind, and the same question gets answered again in tickets/transfer.py and in refunds/policy.py
The hypothesis	What structural problem is usually underneath	Behavior that depends on ticket type is scattered across three files instead of living alongside the type
The cost	What's going to hurt, when, and for whom	Adding a fifth ticket type forces finding the three places. Whoever forgets one produces a silent bug: the new ticket behaves like general in refunds

Notice the symptom is a fact, the hypothesis is an interpretation, and the cost is a prediction. All three can be argued, but they get argued differently. The symptom gets argued by counting. The hypothesis gets argued with design arguments. The cost gets argued with the team's experience and with what the roadmap has planned. Mixing them up is the source of half the arguments that go nowhere in a review.

Worked example: an investigated smell, not a corrected one

Let's run the full cycle on a Boletia case, and let's do it with the outcome almost never shown in a course: one where part of the finding gets fixed and part gets decided to be left alone.

You open pricing/calculator.py because you have to review a PR that adds the student ticket type.

# File: pricing/calculator.py

def calculate_price(ticket, order_date):
    if ticket.kind == "general":
        return ticket.base_price
    elif ticket.kind == "vip":
        return ticket.base_price * 1.40
    elif ticket.kind == "early_bird":
        cutoff = get_early_bird_cutoff(ticket.event_id)
        return ticket.base_price * 0.75 if order_date < cutoff else ticket.base_price
    elif ticket.kind == "courtesy":
        if courtesy_count(ticket.event_id) > COURTESY_LIMIT:
            raise CourtesyLimitExceeded(ticket.event_id)
        return 0.0
    else:
        raise ValueError(f"Unknown ticket type: {ticket.kind}")

Step 1 — the symptom, stated with no interpretation. There's an if/elif branching on a free-text field's value, with one branch per possible value. The smell has a name: switch on a type (Fowler calls it switch statements, and its close cousin is primitive obsession, because kind is a str and not a type of its own).

Notice what I didn't say: I didn't say "this is wrong." A four-branch if in one place is perfectly defensible. What I said is that the signal shows.

Step 2 — the investigation. This is where the diagnosis gets won or lost, and where most people skip a step. The question to answer is: is this same question answered anywhere else? Because a switch in one place is a function; the same switch in five places is a structural problem.

# We search every place in the code that asks about ticket type.
# It's not sophisticated and doesn't need to be: the goal is to count.
$ grep -rn "\.kind" --include="*.py" .

And this comes out:

pricing/calculator.py:4     if ticket.kind == "general":
pricing/calculator.py:6     elif ticket.kind == "vip":
pricing/calculator.py:8     elif ticket.kind == "early_bird":
pricing/calculator.py:11    elif ticket.kind == "courtesy":
tickets/transfer.py:22      if ticket.kind in ("courtesy",):
refunds/policy.py:15        if ticket.kind == "early_bird":
reports/attendees.py:41     "type": TICKET_LABELS.get(ticket.kind, ticket.kind),
api/routes.py:88            if payload["kind"] not in ALLOWED_KINDS:

Step 3 — reading each occurrence. This is what separates investigating from counting. Five files show up, but they don't all say the same thing:

• pricing/calculator.py — decides behavior based on type. Four branches, real logic in each.
• tickets/transfer.py — decides behavior: complimentary tickets don't get transferred.
• refunds/policy.py — decides behavior: early-bird tickets have a three-day refund window, everyone else fourteen.
• reports/attendees.py — decides nothing: translates the value into a label to display. It's a presentation table.
• api/routes.py — decides nothing: validates that the input value is on the allowed list.

Here's where the real finding shows up, and it isn't the one that showed at first. Three files make behavior decisions based on ticket type, and none of the three knows about the other two. The other two uses are harmless: presentation and input validation aren't domain behavior.

Step 4 — the cost, in a concrete scenario. The PR you're reviewing adds student. Let's walk through what has to happen for that type to work fully:

1. pricing/calculator.py — new branch with the discount. The PR does it.
2. api/routes.py — add "student" to ALLOWED_KINDS. The PR does it.
3. reports/attendees.py — add the label. The PR does it.
4. refunds/policy.py — decide the refund window for a student ticket. The PR doesn't touch it.
5. tickets/transfer.py — decide whether a student ticket can be transferred. The PR doesn't touch it.

Points 4 and 5 don't fail. No exception, no red test, nothing breaks. The student ticket simply falls into each one's else and behaves like a general ticket: fourteen-day refund window and transferable with no restriction. That might be exactly what the business wants. It might not be. Nobody decided it: it decided itself, by omission.

That's the smell's cost, stated in its most useful form: it isn't that the code is ugly, it's that the system makes decisions nobody made.

Step 5 — the decision, which is what almost never gets taught. You have the diagnosis. Now, what do you do?

The reflexive answer would be: "this calls for a Strategy, per-type behavior needs to move into TicketKind classes." And it could be right in the medium term. But module 2 left a pocket question that applies exactly here: does the redesign earn its place today? Let's think it through out loud.

In favor of refactoring: it's three real places, they've already answered differently, and the failure mode is silent — which is the worst kind. Against: the PR you're reviewing is about one feature, not a redesign; asking that person to restructure the ticket-type model just to add student multiplies the change's size by ten, and whoever wrote it has probably been on the team for two weeks.

The sensible way out is splitting the finding into two comments of different weight:

Blocking — refunds/policy.py and tickets/transfer.py (outside the diff, but a direct consequence of this change): these two files also decide based on ticket.kind, and with this PR student is going to fall into their else. That means a fourteen-day refund window and unrestricted transfer, decided by omission. Is that what product wants? If so, let's add the explicit branch anyway, so the decision gets written down. If not, it needs to change here.

Note, not blocking — the underlying pattern: ticket.kind is a str three independent files branch on today (pricing, refunds, transfer). It's a repeated switch with primitive obsession underneath, and the failure mode is the one we just saw: a new type behaves like general with nobody deciding it. When the sixth ticket type comes in, it's worth moving per-type behavior into one place (a Strategy per kind, with the three questions — price, refund window, transferable — as methods). Not work for this PR; I'm leaving it written down so it exists.

What to expect from this walkthrough. The first thing: the finding changed during the investigation. You started seeing "a long if in calculator.py" and ended up with something completely different and much more serious: "there are three files that decide by type and don't know about each other, and this PR is going to leave two decisions made by omission." If you'd corrected reflexively — splitting the if into small functions, or dropping a Strategy right there — you'd have fixed calculator.py's aesthetics and the refunds bug would have gone in just the same. Investigating isn't slowness: it's what makes you aim at the problem and not the symptom.

The second: the outcome has two different weights. One part is blocking and small — two explicit branches — the other is structural and blocks nothing. Putting them in the same comment would have produced the classic effect: the author reads a paragraph about Strategy, feels like they're being asked to redesign the system to add a ticket type, and gets stuck. Separating them lets them do the urgent thing today and leaves the important thing on record.

The third, and the hardest to accept: the smell got investigated and most of it got decided to stay. calculator.py's code leaves this review exactly as it entered, with one more branch. And that's fine. Lesson 1 said it and here you see it working: the difference between an ignored smell and a decided smell is enormous, even though the code looks the same.

The short catalog

Fowler cataloged more than twenty smells; refactoring.guru lists a couple dozen grouped into five families. The full list is a useful reference, not a study plan. These twelve are the ones that genuinely show up in real code reviews, the ones you'll be able to use without feeling like you're forcing the label.

I group them by the question each one answers, because that's easier to remember than alphabetical order.

Size smells: "this is too big"

Long method — the long function. A function that does so many things that understanding it requires reading it whole and keeping mental count. The signal isn't the line count but the number of mixed abstraction levels: if one function calculates a total, builds an HTTP client, and formats a message, it's jumping between three different heights. At Boletia: checkout(), which orchestrates pricing, seats, charging, and notices in a single body. Typical false positive: a long, flat data table, or a configuration function that only assigns values.

Large class / God object — the class that knows everything. Many responsibilities, many attributes, many collaborators. It's the most expensive size smell because it attracts more code: since everything's already there, dropping one more thing always seems easiest. At Boletia: checkout/checkout.py. Lesson 3 opens it up under a magnifying glass.

Long parameter list — the long argument list. A function that asks for six, eight, ten arguments. It usually signals that several of those parameters are actually a concept that doesn't have a name yet. If send_confirmation(email, name, order_id, total, event_name, venue, starts_at) bothers you, it's because the last three are an Event and the middle ones are an Order. Its cousin is data clump: the same group of three or four values traveling together throughout the system.

Change smells: "this is going to hurt when you touch it"

Divergent change — the file that changes for many reasons. A file that gets modified for reasons that have nothing to do with each other: today because a fee changed, tomorrow because an email's format changed, the day after because the payment provider changed. A sign that several responsibilities that should be separate coexist inside. At Boletia: checkout.py, again.

Shotgun surgery — the change that scatters. The exact opposite of the previous one: a single conceptual change forces touching many files. At Boletia: adding a payment provider touches five places. Lesson 3 opens it up under a magnifying glass.

It's worth seeing these two together, because they're symmetric and the symmetry helps you remember them:

	One change	Many changes
One file	Normal	Divergent change
Many files	Shotgun surgery	Normal in a large system

Duplicated code — repeated code. The most well-known and worst-understood one. Repeating three lines isn't always a problem; repeating a decision almost always is. The useful question isn't "do they look alike?" but "if one changes, do they all have to change?" At Boletia: the three exporters in reports/ repeat the same skeleton — fetch, sort, format, write — and only one step differs. That points to Template Method (module 3). Watch the false positive: two pieces that look the same today but change for different reasons aren't duplication, and unifying them couples them. Module 2 devoted half a lesson to that trap.

Responsibility smells: "this doesn't live where it should"

Feature envy — data envy. A method that uses more data from another class than from its own. At Boletia: a notify_finance(order) that touches seven attributes of Order and none of its own class. Lesson 3 opens it up under a magnifying glass.

Message chain — the message chain. Code that navigates other objects' internal structure: order.customer.event.organizer.email. The problem isn't the length, it's that whoever wrote that line depends on four structures to get one piece of data. If any of the four links changes, this line breaks. The pocket rule that describes it is known as the Law of Demeter: talk to your friends, not to your friends' friends.

Middle man — the empty middleman. A class whose methods only delegate to another one, adding nothing. It's the smell opposite to missing indirection, and it's how a well-intentioned pattern turns into dead weight. Watch the false positive: an Adapter (module 5) delegates almost everything on purpose, and there the delegation is the value — it's translating between two interfaces. The question that tells them apart: does this middleman change anything, or does it just pass the ball along?

Type smells: "a concept is missing here"

Primitive obsession — obsession with primitives. Business concepts traveling as a loose str, int, or dict instead of having a type of their own. At Boletia there are three textbook examples: Ticket.kind is a str, Order.provider is a str, and Order.status is a str. All three are actually closed sets of values with associated behavior, and since they're free text, the compiler — or the editor, or the test — can't help you: a "vip " with a trailing space falls into the else without saying anything.

Repeated switch on a type. The one we just investigated. The signal is the same if on the same field, answered in more than one file. It almost always comes paired with primitive obsession, because the switch exists precisely because the type has no behavior of its own.

Excess smells: "this is unnecessary"

Speculative generality — speculative generality. Extension structure built for needs that never arrived: an interface with one implementer, a parameter that always receives the same value, an abstract class with a single child, a plugin mechanism with one plugin. At Boletia: plugins/, which module 2 dismantled step by step in its lesson 6. This is the only smell in the catalog whose treatment is removing, not adding.

Comments as deodorant — the comment that masks. A long comment explaining what an enigmatic block does. It's not that commenting is bad; it's that a comment explaining what a piece of code does is usually a sign that piece was asking for a name. # Here we calculate the fee based on the amount and the provider almost always means a function called calculate_commission(amount, provider) was missing. The distinction: a comment explaining why is valuable and smells of nothing; one explaining what is usually substituting for a name.

The pocket table

Smell	Signal in one line	At Boletia
Long method	A function with several mixed abstraction levels	checkout()
God object	Many responsibilities, many collaborators, everyone touches it	checkout/checkout.py
Long parameter list	Six or more arguments; several form an unnamed concept	send_confirmation(...)
Divergent change	A file that changes for unrelated reasons	checkout/checkout.py
Shotgun surgery	One conceptual change that touches many files	Adding a payment provider: 5 places
Duplicated code	If one copy changes, they all have to	The three exporters in reports/
Feature envy	A method uses more foreign data than its own	notify_finance(order)
Message chain	a.b.c.d — walking through others' structure	order.customer.event.organizer.email
Middle man	Delegates everything and adds nothing	A manager that only forwards
Primitive obsession	A business concept travels as str or int	Ticket.kind, Order.provider, Order.status
Repeated switch	The same if on the same field, across several files	ticket.kind in pricing, refunds, transfer
Speculative generality	Extension built for something that never arrived	plugins/

Print it mentally, don't memorize it. The way to learn this catalog isn't reviewing it: it's opening any file from your own work, going through the table, and noting which ones you see. The first time you'll see four where there's one. The fifth time you'll see the one that matters.

Why a smell gets investigated and not reflexively corrected

You already saw the investigation in action. Now I want to leave the rule explicit, because it's the idea most often lost when someone learns this vocabulary.

A detected smell doesn't authorize a change. It authorizes a question. And the question has three parts worth asking in order:

First: is it real? That is, is the symptom what it looks like? False positives exist and are frequent. The long method that turned out to be a table. The duplication that turned out to be two things that look alike today and change for different reasons. The middleman that turned out to be an Adapter doing its job. This first question gets answered by reading, and it's the one most people skip.

Second: how much does it cost? A smell with no cost isn't a problem, it's a curiosity. And the cost is measured in concrete future work, not principles. The estimation method that works best is the change scenario: pick a plausible change the team's going to need in the coming months — adding a provider, adding a ticket type, changing the email's format — and walk through which files need touching and what can go wrong. If the walkthrough is short and no-surprise, the smell is cosmetic. If it goes through five files and one of the failure modes is silent, there's your case.

There's a second, free, and much undervalued source of evidence: history. If a file shows up in thirteen of the last twenty changes, that isn't an opinion about its design; it's a fact about how the team behaves around it.

# The files touched the most over the last 200 commits.
# It doesn't prove they're badly designed, but it does say where the pain lives.
$ git log --format=format: --name-only -n 200 | sort | uniq -c | sort -rg | head -10

Third: now? This is the judgment question, and it's the one that most resembles module 2. A real, expensive smell can still be the wrong thing to fix today, for three legitimate reasons: because the PR you're on is about something else; because information is missing — two more ticket types are coming and the right design still isn't visible; or because there's something more expensive pending. Deciding "not yet" with the diagnosis written down is a perfectly respectable engineering decision. What isn't respectable is not having looked.

A note about this third question that connects to the whole guide. "Yes, now, and with a pattern" is one of several answers, and not the most frequent one. Often the right fix involves no pattern at all: extract a function, rename a variable, move a method to another class. Patterns are the answer when the problem is structure that varies; for everything else there are simple refactors with unglamorous names. Confusing "I detected a smell" with "a pattern needs to go in" is exactly the reflex this module is trying to switch off.

Common mistakes

Treating the smell as the problem (conceptual). What happens: someone detects an eighty-line method and splits it into four twenty-line methods, with names like _step_one, _step_two. The "long method" smell disappeared and nothing got fixed: now you have to read four functions instead of one, and none of them makes sense on its own. Why it happens: the smell is what's visible and measurable, and making it disappear produces an immediate sense of progress. How to spot it: if after your refactor you can't name what structural problem you solved — only which signal you turned off — you fixed nothing. The most direct test: if the new methods don't have names that mean something in the domain (calculate_service_fee, assign_seats) but positional names (_part_two), you split by size and not by responsibility. How to fix it: before touching anything, write in one sentence the hypothesis — what's badly arranged — and the cost — what's going to hurt. If you can't write them, you haven't investigated enough yet.

Handing out labels without reading (method). What happens: someone goes through a diff with the catalog table next to them and starts marking: "long method," "primitive obsession," "feature envy." Eight comments in four minutes. Half are false positives and the author knows it, so they start discounting all your comments, including the good ones. Why it happens: the catalog can be applied superficially, and that's exactly what makes it dangerous. Recognizing a smell's shape doesn't require understanding the code; telling a real smell from a false positive does. How to spot it: if your comments contain no countable evidence — a number, a list of files, a scenario — they're labels. How to fix it: the worked example's rule. A detected smell requires a search before the comment: grep for the field, check how many uses there are, read whether they decide or just display. That search takes two minutes and turns a label into a diagnosis.

Believing the catalog is a list of prohibitions (judgment). What happens: someone leaves a lesson like this convinced an if over a type is bad, that long methods are bad, that duplication is bad, and starts writing code avoiding smells instead of solving problems. The result is usually worse than the code it avoided: class hierarchies for two cases, chained three-line functions, premature abstractions everywhere. Why it happens: following a list of prohibitions is easier than exercising judgment, and a list gives the reassuring feeling of doing the right thing. How to spot it: if you're introducing indirection before having a real second case, you're programming against the list. How to fix it: all of module 2, and the rule of three in particular. Smells are tools for reading existing code, not rules for writing new code. Writing new code while avoiding imaginary smells produces the smell this catalog closes with: speculative generality.

Exercises

Exercise 1 — Separate the symptom from the hypothesis and the cost. Here are three observations about Boletia, written the way they're normally said. For each one, break it into the three pieces: what's the observable symptom, what's the hypothesis about the structure, and what's the concrete cost. If any of the three pieces is missing from the original, fill it in yourself.

(a) "checkout.py is a mess." (b) "The three exporters in reports/ are copy-pasted." (c) "order.provider being a string gives me a bad feeling."

See solution

(a) Symptom: the original has none; "it's a mess" is a value judgment. A real symptom would be: checkout() is about 300 lines, orchestrates four unrelated things (pricing, seats, charging, notices), imports eleven modules, and shows up in thirteen of the last twenty commits. Hypothesis: several responsibilities that should be separate coexist — it's a God object with divergent change. Cost: any change in any of the four areas forces touching the system's most delicate file, and two people working on different things collide on the same file. Notice how much work it took to turn the original sentence into something arguable: that's the difference between a complaint and a diagnosis.

(b) Symptom: the three exporters run the same sequence of four steps in the same order (fetch → sort → format → write) and only differ in the third one. It's countable: you can lay the three files side by side. Hypothesis: the algorithm's skeleton is duplicated; only the variable step should live in each class. Points to Template Method. Cost: a change to the skeleton — paginating the query, changing the sort criterion, adding a header column — has to be done three times, and whoever forgets one produces an inconsistency that only shows up in production. Note the original had the symptom but was missing the hypothesis and the cost, and with no cost there's no case.

(c) Symptom: Order.provider is an unrestricted str, and there are at least two whitelists of allowed values in different files (api/routes.py and the if/elif in checkout.py). Hypothesis: primitive obsession — a closed domain concept traveling as free text, with its associated behavior scattered around. Cost: a typo or case error ("Stripe", "stripe ") falls into the else and produces a ValueError in checkout's core; and the two lists can drift out of sync, letting the API accept a provider checkout can't charge, or the reverse. "Bad feeling" is a legitimate intuition, but it can't be argued; this can.

Why it works: the three pieces are exactly what lesson 4 is going to ask you for to write an actionable comment. Practicing the separation here, on observations you already had, makes the formula feel natural there instead of bureaucratic.

Exercise 2 — Investigate before deciding. You have to review this new Boletia file. Name the smells you see, and for each one say what you'd search for before writing a comment. Don't propose any solution yet.

# File: notifications/manager.py

class NotificationManager:
    def __init__(self, db, mailer, sms_client, push_client, analytics, settings):
        self.db = db
        self.mailer = mailer
        self.sms_client = sms_client
        self.push_client = push_client
        self.analytics = analytics
        self.settings = settings

    def send_order_confirmation(self, order):
        customer = self.db.get_customer(order.customer_id)
        event = self.db.get_event(self.db.get_ticket(order.ticket_ids[0]).event_id)
        body = (
            f"Hi {customer.name}, your purchase for ${order.total} "
            f"for {event.name} at {event.venue} on {event.starts_at} is ready. "
            f"Order #{order.id}, {len(order.ticket_ids)} tickets."
        )
        if order.provider == "cash":
            body += " Remember to pay in store within 48 hours."
        self.mailer.send(customer.email, "Your Boletia purchase", body)
        if customer.phone:
            self.sms_client.send(customer.phone, body[:140])
        if customer.push_token:
            self.push_client.send(customer.push_token, body[:80])
        self.analytics.track("confirmation_sent", order_id=order.id)

See solution

Four smells, and a concrete search for each one.

Long parameter list in the constructor — six dependencies. What to search before commenting: how many of the six each method of the class uses. If send_order_confirmation uses five and another method uses two different ones, it isn't a long list: it's two classes stuck together, and the real smell is God object. If every method uses almost everything, the long list is honest and the comment would be a different one.

Feature envy toward Order, Customer, and Event — the method builds a text using purely foreign data: customer.name, order.total, event.name, event.venue, event.starts_at, order.id, order.ticket_ids. Seven external accesses, zero own attributes (except the injected collaborators). What to search: whether some build_confirmation(order) already exists elsewhere in the system. At Boletia it does, and checkout() calls it — so there's also duplication of the message-building logic.

Switch on order.provider — the if order.provider == "cash" dropped in the middle of building the text. What to search: grep -rn "provider ==" . to see how many files decide by provider. If there are several, this is the same shotgun surgery we already know, now peeking out in notifications.

Message chain — self.db.get_event(self.db.get_ticket(order.ticket_ids[0]).event_id). It isn't the classic a.b.c.d form, but the problem is identical: this line depends on an order having at least one ticket, on the ticket having event_id, and on two repository calls to get one piece of data. What to search: whether Order has, or could have, a direct way to give its event; and what happens today if ticket_ids comes in empty — most likely an IndexError in the middle of sending an email, which is a terrible place to blow up.

Notice what's not on this list: no proposed solution. The exercise asked to investigate, and in a real review the investigation changes the comment. If the search for build_confirmation finds nothing, the feature-envy comment is a minor note. If it finds the same text getting built in three places with slightly different formats, the comment becomes about duplication and is probably blocking.

Why it works: the habit I want to install is that between "I see something" and "I write something" there's a step, and that step is usually a two-minute grep. That step is what turns someone who hands out labels into someone whose comments the team reads carefully.

Exercise 3 — Find the false positive. Here are three Boletia snippets. All three show a catalog smell. In one of the three, the smell is a false positive and the code is fine as it is. Identify which one and explain why the other two are real problems.

# (a) utils/currency.py
def format_money(amount, currency):
    if currency == "MXN":  return f"${amount:,.2f} MXN"
    elif currency == "USD": return f"US${amount:,.2f}"
    elif currency == "EUR": return f"{amount:,.2f} €"
    elif currency == "COP": return f"${amount:,.0f} COP"
    elif currency == "ARS": return f"${amount:,.2f} ARS"
    else: return f"{amount:,.2f} {currency}"

# (b) payments/stripe_provider.py
class StripeProvider(PaymentProvider):
    def __init__(self, client):
        self.client = client
    def charge(self, order):
        r = self.client.create_charge(amount=int(order.total * 100), currency="MXN")
        return ChargeResult(ok=r["status"] == "succeeded", reference=r["id"])
    def refund(self, order):
        return self.client.create_refund(charge_id=order.external_ref)

# (c) reports/attendees.py
def attendee_row(ticket, order, customer, event):
    return {
        "name": customer.name,
        "email": customer.email,
        "ticket": ticket.id,
        "type": ticket.kind,
        "seat": ticket.seat or "-",
        "event": event.name,
        "paid": order.total,
        "status": order.status,
    }

See solution

The false positive is (a).

Yes, it's a five-branch if/elif on a type, and at first glance it's the same smell we investigated in calculate_price. But look at the three differences that save it. First, there's no behavior: each branch is a text template, not a business decision. Second, it isn't repeated anywhere else: the question "how does an amount get written in this currency" gets answered in exactly one place in the system, so there's no risk of drift. And third, the else is a correct default, not a hole: an unknown currency gets formatted sensibly instead of a decision being made by omission. Turning this into a Currency class hierarchy with a format() method would be exactly what the lesson called correcting reflexively: adding five classes to eliminate five lines.

(b) is a real problem, but a small one and of a different kind. What jumps out is that "MXN" is hardcoded inside charge — a business value buried in an infrastructure detail. Concrete consequence: the day Boletia sells an event in another currency, this provider is going to charge in pesos with no warning. That said, the class itself is fine: it's an Adapter doing its job — translating between Stripe's SDK and Boletia's shared shape — and the fact that almost everything it does is delegate isn't a middle man. That's the false positive worth knowing to rule out: delegation with translation has value.

(c) is a real problem, and it's a data clump. The four parameters — ticket, order, customer, event — travel together, and they almost certainly travel together in several other places in the reporting module. Consequence: when a fifth piece of data is needed in the row — the payment provider, say — this function's signature and every caller's has to change. On top of that, the caller has to gather all four things before it can ask for a row, which tends to produce the query chain we saw in exercise 2. What's missing here is a concept with no name yet: something like AttendeeRecord, which knows how to gather the four pieces once.

Why it works: the exercise trains the skill that separates useful vocabulary from harmful vocabulary. Recognizing a smell's shape is easy and gets learned in an afternoon; deciding whether that shape corresponds to a real problem requires reading, counting, and thinking about the cost. If in your next review you consciously rule out a smell you saw — and say so, even just to yourself — you're already using the catalog the right way.

Summary and next step

In this lesson you defined what a code smell is: a superficial, observable trait of the code that, with high frequency, indicates a deeper structural problem. You saw that the definition's three loaded words rule out three misunderstandings: superficial means it gets detected without knowing the domain, which is why it's teachable; observable means it's countable, which is why it leaves matter-of-taste territory; and high frequency, not always means a smell is a correlation, not an implication — which is why legitimate false positives exist.

You saw the three-piece anatomy you'll use for the rest of the module: the symptom (a countable fact), the hypothesis (an interpretation about the structure), and the cost (a prediction about future work). And you saw the full cycle working on pricing/calculator.py, with an outcome worth remembering: the investigation changed the finding — from "a long if" to "two decisions that are about to get made by omission" — the result got split into two comments of different weight, and most of the code left exactly as it entered.

You're walking away with the short catalog — twelve smells grouped by the question they answer — with its Boletia example, and the rule that organizes the whole module: a detected smell doesn't authorize a change, it authorizes a question, and that question has three parts in order: is it real? how much does it cost? now?

Before moving on you should be able to: explain why a smell isn't an error; give an example of a false positive and say what saves it; name the catalog's five groups; and describe the symmetry between divergent change and shotgun surgery.

What's next is the magnifying glass. Three of the twelve smells concentrate most of the real pain and show up in nearly every review: God object, feature envy, and shotgun surgery. Lesson 3 opens them one by one with their Boletia example, and for each one gives you something this lesson only hinted at: an almost-mechanical detection method — what to count, with what command, what number is a signal — and the direction they usually resolve toward.

Resources

• Refactoring, chapter 3 — Bad Smells in Code (Martin Fowler and Kent Beck) — the foundational chapter, written in 1999 and still current. Worth it for a reason this summary can't convey: the authors insist over and over that there are no rules, only signals.
• Refactoring Guru — Code Smells — the full catalog grouped into five families, and for each smell the refactorings usually applied. A reference, not a straight read.
• Wiki Wiki Web — Code Smell — the original community discussion where the term was born, with Kent Beck explaining where the smell metaphor came from. It's a historical document and reads in ten minutes.
• Your Code as a Crime Scene (Adam Tornhill) — how to use version control history as evidence about design. It's the source of the idea that the files that change the most are a fact about the structure, not just about activity.