Code smells: a working catalog of six, with C# examples

You have heard the term for as long as you have been reviewing code. Kent Beck coined it, the "Bad Smells in Code" chapter he and Martin Fowler wrote for Refactoring made it canon (Fowler's short definition is still the cleanest), and somewhere along the way "code smell" became a thing people say in pull requests without anyone agreeing on what it commits them to. So here is the working definition, and then a working catalog: a smell is a surface symptom that usually, not always, indicates a deeper problem.

The two words doing the work in that sentence are surface and usually.

A symptom, not a verdict

A smell is cheap to detect and expensive to misread. That is the whole design. Beck and Fowler picked symptoms you can spot in seconds (this method is long, this call chain is nosy) precisely because the deeper problems they point at (tangled responsibilities, misplaced behavior) take real reading to confirm. The smell buys you a place to point the flashlight. It does not buy you a conclusion.

A code smell tells you where to look, not what to do. Half the skill is the catalog; the other half is knowing when the answer is "leave it alone."

That framing matters because the failure mode of learning smells is turning them into lint rules for humans: every long method flagged in review, every comment treated as a confession. Applied that way the catalog makes codebases blander and colleagues angrier without making anything more correct. Applied as a vocabulary for suspicion, it is one of the highest-leverage things a working programmer can learn.

The smells a machine can see

A few smells are honest-to-goodness measurable. Method length is a number. Nesting depth is a number. Cyclomatic complexity is a number with fifty years of literature behind it. When a tool puts limits on those three, the conversation stops being about taste:

method length

limit 25 lines

the starter: 78 ✗ yours: 18 ✓

cyclomatic complexity

limit 8

the starter: 14 ✗ yours: 6 ✓

nesting depth

limit 3 levels

the starter: 5 ✗ yours: 2 ✓

✓ 9 / 9 tests still passing (behavior never changed)

Most of the catalog is not like this. No analyzer reliably knows whose job a computation is, or whether a parameter list is begging to become a type. The measurable trio is the machine's share of the work; the rest below needs a human nose. Take them in order of how often they bite.

The catalog: six smells worth knowing

1. Long method

The tell: you scroll to read it. Twice.

Why it hurts: a method is a promise that one name covers everything inside it. At 78 lines the promise is broken; the method is doing several jobs, and every reader must reverse-engineer the boundaries between them before touching anything. Bugs hide in exactly those unmarked seams.

The move: extract method, until what remains reads like a table of contents:

public async Task PlaceOrderAsync(Cart cart, PaymentMethod payment)
{
    var order = BuildOrder(cart);
    ApplyPromotions(order);
    await ReserveStockAsync(order);
    await ChargeAsync(order, payment);
    await PublishOrderPlacedAsync(order);
}

Each extracted step gets a name, a small scope, and a test you can actually write.

2. Long parameter list

The tell: parameters that travel in a pack. Wherever street goes, city, postalCode, and countryCode follow:

public Shipment CreateShipment(
    string street, string city, string postalCode, string countryCode,
    decimal weightKg, bool isFragile, bool isPriority)

Why it hurts: the pack is a concept without a name, so every call site re-assembles it from loose parts, argument order becomes a trap (bool, bool at the end is a bug plot), and validation gets copy-pasted or skipped.

The move: introduce parameter object. The data clump is begging to be a type:

public Shipment CreateShipment(Address destination, Parcel parcel)

Now Address can validate itself once, and the signature says what the method actually needs.

3. Feature envy

The tell: a method that interrogates another object's fields to do that object's job:

public decimal DiscountFor(Customer customer)
{
    var years = (DateTime.UtcNow - customer.FirstOrderAt).Days / 365;
    if (customer.LifetimeSpend > 10_000m && years >= 2)
    {
        return 0.10m;
    }
    return customer.Orders.Count > 50 ? 0.05m : 0m;
}

Every input is customer data. The invoicing service this method lives on contributes nothing but a place to stand.

Why it hurts: the logic lives far from the data it depends on, so when Customer changes, this method breaks in a file nobody thought to check. Multiply by every envious method and the class's real behavior is scattered across the codebase.

The move: move method. Let customer.LoyaltyDiscount() own its own rules, and the next change happens in one obvious place.

4. Primitive obsession

The tell: domain concepts passed around as string and decimal. An email address, a currency amount, and a country code all typed the same, so the compiler cannot tell them apart even when you swap them by accident.

Why it hurts and what to do about it: this one deserves its own treatment, and it has one. Primitive obsession and value objects covers the smell, the wrapper types that fix it, and where to stop before you have wrapped every int in the building. The short version: make the type system carry the rules you are currently enforcing by vigilance.

5. Shotgun surgery

The tell: one conceptual change, many files:

$ git diff --stat   # the change: "totals round to two decimals, always"
 src/Billing/InvoiceCalculator.cs  | 4 +-
 src/Billing/RefundCalculator.cs   | 4 +-
 src/Cart/CartSummary.cs           | 3 +-
 src/Reporting/RevenueReport.cs    | 5 +-
 src/Api/Dto/OrderTotalsDto.cs     | 2 +-
 tests/Billing/InvoiceTests.cs     | 6 +-
 tests/Cart/CartSummaryTests.cs    | 6 +-
 7 files changed

Why it hurts: the diff is the smell. One decision (how totals round) was never given a home, so it lives as seven copies that must now be found and changed in lockstep. Miss one and you have two rounding policies in production, which is the kind of bug that surfaces as a support ticket about three cents.

The move: consolidate. Pull the scattered decision into one class or one function, and let the seven call sites depend on it. The next rounding change is a one-file diff.

6. Comments as deodorant

The tell: a comment explaining what clearer code would say itself:

// active, verified, not opted out of marketing
var eligible = users.Where(u =>
    u.Status == UserStatus.Active &&
    u.EmailVerifiedAt != null &&
    !u.Preferences.HasFlag(Preferences.NoMarketing));

Why it hurts: the comment and the predicate will drift apart, and when they disagree the reader has no way to know which one is lying. Comments that restate logic are maintenance debt wearing a helpful expression.

The move: extract the logic under the name the comment was trying to be:

var eligible = users.Where(u => u.CanReceiveMarketingEmail());

To be clear, this is about comments that paraphrase code. Comments that explain why (the workaround for the vendor API, the regulatory reason behind the odd rounding) are some of the most valuable lines in a codebase. Deodorant hides; documentation informs.

When a smell is fine

The catalog has a failure mode, so here is the counterweight. A long method that is a flat, linear script of steps with no branching can be the honest shape of the work: a data migration, a report builder, a setup sequence. Slicing it into eight two-line methods adds indirection and removes the reading order. The scroll test flags it; the read acquits it.

The same goes elsewhere. A parameter list that mirrors an external contract may be the truthful representation of that contract. Test arrange blocks repeat themselves because each test is supposed to stand alone. In every case the question is the same: does the symptom trace back to a real problem, or does it just pattern-match one? The smell starts the investigation. The investigation is still your job.

Making it a rep, not a lecture

The gap between knowing this catalog and using it is reps: seeing a smell in code you did not write, forming a hypothesis, making the move, and finding out whether the code got better or just different. Katabench's refactoring track is built as exactly that rep. You inherit a working mess with a green test suite, and the grader measures your rewrite structurally, so "better" is not a feeling: the tests must stay green while length, complexity, and nesting come down under real limits.

The machine-checkable smells are the scaffolding; the judgment calls (whose job is this computation, is this comment deodorant or documentation) are what the practice actually trains. For the full-scale version of that judgment applied to a legacy codebase, see refactoring legacy C#. And the next time someone writes "code smell" on your pull request, you can ask the only question that matters: which one, and what did you find when you looked?