A Code Smell Catalog: Recognizing Smells in C# Code

Version note: The code in this post was built and run with C# 14 and .NET 10.

Guess First: How Many Smells in This Method?

A 25-line method from the legacy OrderService.cs we've been cleaning up throughout the series. It cancels an order:

public bool CancelOrder(int id, bool refund, bool notify)
{
    try
    {
        Order o = GetOrder(id);
        if (o == null) return false;
        if (o.Status == 3 || o.Status == 4) return false;
        if (o.Status == 9) return false;
        if (refund)
        {
            var r = pc.Refund(o.PaymentRef, (long)(o.Total * 100));
            if (r.Code != "00") { LastError = r.Message; return false; }
        }
        foreach (var oi in o.Items) oi.Book.Stock += oi.Qty;
        o.Status = 4;
        o.Customer.Points -= (int)(o.Total / 10);
        if (o.Customer.Points < 0) o.Customer.Points = 0;
        if (notify) Mailer.Send(o.Customer.Email, "Order #" + o.Id + " cancelled", "Your order has been cancelled.", false);
        return true;
    }
    catch (Exception)
    {
    }
    return false;
}

Before reading on, count: how many separate problems do you see here, and can you give each one a name? Saying "bad code" is easy; saying "two flag arguments, a swallowed exception, status as a magic number, a static error field, a calculation that changes another object's data" is a list a team can discuss, prioritize and fix. The code smell catalog gives you exactly these names. Note your count; we'll come back to the same method with a table at the end of the post.

What a Code Smell Is and Isn't

The term comes from Kent Beck. According to Martin Fowler, Beck coined it while helping with Fowler's Refactoring book (Refactoring: Improving the Design of Existing Code, 1999). The definition in Fowler's 2006 bliki post is short: a code smell is a surface indication that usually corresponds to a deeper problem in the system. Both parts of the definition matter. "Surface", because a smell is quick to spot: you don't need to know the domain to see a 127-line method. "Usually", because a smell doesn't always mean a problem; in Fowler's own example, some long methods are just fine. A smell is not a diagnosis, it's a sign pointing at where to look.

In practice this distinction says the smell list is not a linter rule set. The "saw a Long Function, split it" reflex leads to the over-decomposition problem we'll discuss two posts from now. The right question is "which change does this smell make expensive here?"

The first edition of Refactoring in 1999 listed 22 smells. The second edition in 2018 grew the list to 24 and renamed some: Long Method is now Long Function, Switch Statements is now Repeated Switches, Lazy Class is now Lazy Element, Inappropriate Intimacy is now Insider Trading. Mysterious Name, Global Data, Mutable Data and Loops are new. This post uses the second-edition names; you'll still see the old ones in many sources.

Five Groups: Mapping the Smells

Fowler doesn't group smells; he gives a flat list. The most widely used grouping today is Mika Mäntylä's: first in his 2003 master's thesis, then in the study he published with Casper Lassenius in the journal Empirical Software Engineering in 2006, they sorted the 22 smells into five groups by their common effects. In this post I didn't put the four new second-edition smells and Comments in a group; they are in the "Other" box on the map below.

The value of the groups: smells in the same group usually charge the same kind of cost and are solved with similar refactorings. Bloaters raise the cost of reading, OO (object-oriented) Abusers put inheritance and polymorphism in the wrong places, Change Preventers the cost of change, Couplers make changing one class force changes in others. The table below looks for each of the 24 smells in OrderService.cs:

There are two honest "none"s in the table. Not every legacy file contains every smell, and trying to fill the catalog like a checklist is a trap of its own. The refactoring names match those in Fowler's catalog on refactoring.com; the steps themselves are the topic of the upcoming Refactoring series.

Bloaters: Code That Has Grown Too Big

Bloaters don't appear overnight; like the changelog comments in the opening, every addition is "a small change". The most visible one is Long Function: ProcessData is 127 lines and carries four levels of nested if. We opened it up with guard clauses and Extract Function in the functions post.

Long Parameter List and Data Clumps usually come together:

public int CreateCustomer(string name, string email, string street, string city, string zip, string country, string phone, string type, bool newsletter)

Four of the nine parameters (street, city, zip, country) always travel together. Data that travels together is a concept nobody has named. If a caller swaps city and zip, the compiler won't warn you, both are string. The fix is to name that concept; in the clean version in post 14 it became two records:

public sealed record Address(string Street, string Zip, string City, string Country);

public sealed record NewCustomer(string Name, string Email, Address Address, CustomerTier Tier = CustomerTier.Normal, string Phone = "", bool Newsletter = false);

Primitive Obsession is the same problem at the type level: deciding whether Customer.Type is "vip" with a string comparison produced the Turkish-I bug that opened the series. Under the tr-TR culture, "vip".ToUpper() returns "VİP", so the VIP discount silently disappeared. With an enum or a value object that comparison goes away; details in the objects and data structures post.

OO Abusers: Using OOP Halfway

The OO Abusers group collects places where the tools of OOP (Object-Oriented Programming) are used wrongly or halfway. Repeated Switches looks like this in the file:

// inside ProcessData
if (b.Type == "ebook")
    oi.Price = b.Price * 0.8m;
else if (b.Type == "audio")
    oi.Price = b.Price * 0.9m;
else
    oi.Price = b.Price;

// inside GetQuote
decimal p = b.Price;
if (b.Type == "ebook") p = b.Price * 0.8m;
tmp += p * kv.Value;

The decision based on the book type is made in two places, and the two have already drifted apart: GetQuote forgot the audiobook discount. That's why Fowler renamed "Switch Statements" to "Repeated Switches" in the second edition: a single switch isn't the problem, repeating the same condition in several places is. The catalog's Replace Conditional with Polymorphism is the right path when there is a lot of behavior per type. Here the behavior is a single price factor, so in post 14 a smaller step was enough: putting the decision in one place.

public static decimal UnitPrice(Book book) => book.Format switch
{
    BookFormat.Ebook => book.Price * PricingRules.EbookPriceFactor,
    BookFormat.Audio => book.Price * PricingRules.AudioPriceFactor,
    _ => book.Price,
};

Refused Bequest isn't in this file, because the file has no inheritance. But .NET's own library has a textbook example. ReadOnlyCollection<T> implements IList<T> and refuses part of the contract it inherits:

using System.Collections.ObjectModel;

IList<string> isbns = new ReadOnlyCollection<string>(["978-0441013593", "978-0141439518"]);

Console.WriteLine(isbns.IsReadOnly);
try
{
    isbns.Add("978-0062316097");
}
catch (NotSupportedException ex)
{
    Console.WriteLine($"{ex.GetType().Name}: {ex.Message}");
}

True
NotSupportedException: Collection is read-only.

A method working through IList<T> assumes it can add to the list it receives and runs into a NotSupportedException at runtime. The BCL (Base Class Library) half-documents this with the IsReadOnly property; the Liskov Substitution post in the upcoming SOLID (Single Responsibility, Open/Closed, Liskov Substitution, Interface Segregation and Dependency Inversion principles) and Design Principles series is about exactly these NotSupportedExceptions.

Change Preventers and Dispensables

The two members of Change Preventers are mirror images. In Divergent Change one module changes for many different reasons: the four changelog lines at the top of ProcessData record that the same method was changed for coupons, express shipping, the VIP discount and the weekend discount. In Shotgun Surgery one change touches many modules: adding a new coupon requires changing ProcessData and GetQuote separately, and forgetting one produces the "one price on the cart page, another at checkout" bug we saw in post 1. The common cure is to separate the reasons for change and put each reason in one place; that's why pricing moved to the OrderPricing class in post 14.

The Dispensables group collects code nobody would miss if it were deleted:

// old discount calculation - not used but do not delete!
private decimal OldCalcDiscount(decimal total, int itemCount)
{
    if (itemCount > 3) return total * 0.07m;
    return 0;
}

//public int ProcessDataV2(Order o)
//{
//    var e = new PricingEngine();
//    return e.Run(o);
//}

// for different order types in the future
public interface IOrderProcessor<T> where T : class
{
    int Process(T item, bool flag);
}

Three different smells on one screen. OldCalcDiscount is dead code; the "do not delete!" warning doesn't say why, and Git keeps the history anyway. The commented-out ProcessDataV2 is a worse form of dead code, because nobody notices when it breaks, it doesn't compile. Dead Code isn't a separate item among the 24 second-edition smells, but the catalog has a matching refactoring: Remove Dead Code. IOrderProcessor<T> is Speculative Generality: an abstraction written "for the future" that doesn't even have one implementation. The fourth rule in the simple design post was asking about elements like this.

Couplers and the Rest

Couplers are places where a class knows too much about another class's internals. The shortest description of Feature Envy: a method is more interested in another class's data than its own. The CancelOrder in the opening calculates and clamps the customer's points itself; Customer could do that job. Insider Trading is in the same spot: OrderService decreases another object's field from the outside. In the clean version this rule moved into Customer:

public void ReturnPointsFor(Order order) =>
    Points = Math.Max(0, Points - (int)(order.Total / PricingRules.CurrencyPerPoint));

Message Chains are the smell form of a Law of Demeter violation:

string label = o.Customer.Name + "\n" + o.Customer.Address.Street + "\n" + o.Customer.Address.Zip + " " + o.Customer.Address.City.Name + "\n" + o.Customer.Address.City.Country;

The shipping label reaches all the way down to the name of the city of the customer's address. If the Address structure changes, the code that produces the label has to change too. Once Hide Delegate shortened the chain and the City class became a property of the address, the label moved to its own place:

public static class ShippingLabel
{
    public static string For(Customer customer, bool express)
    {
        Address address = customer.Address;
        string label = $"{customer.Name}\n{address.Street}\n{address.Zip} {address.City}\n{address.Country}";
        return express ? "*** EXPRESS ***\n" + label : label;
    }
}

Middle Man is the opposite problem: a class whose only job is to delegate to another class. OrderService.SendMail only calls Mailer.Send and swallows the exception on top; nothing would be lost if callers used Mailer directly.

The ones in the "Other" box are all over this file. Mysterious Name was the topic of the naming post, Comments of the comments post. Global Data turned out to be the most expensive smell of the series: because of the static Db and LastError, the characterization tests in post 14 couldn't run in parallel. The sneakiest example of Mutable Data is IsValid: its name reads like a query, but it changes the order's Status, breaking Command-Query Separation.

Which Smells Can Tools See?

Some smells can be caught at the compiler level. I put the same legacy file into a console project, added the SonarAnalyzer.CSharp NuGet package and turned on a few rules that are off by default:

root = true

[*.cs]
dotnet_diagnostic.CA1502.severity = warning
dotnet_diagnostic.CA1505.severity = warning
dotnet_diagnostic.CA1506.severity = warning
dotnet_diagnostic.S3776.severity = warning
dotnet_diagnostic.S107.severity = warning
dotnet_diagnostic.S138.severity = warning

The build reported 72 warnings. The ones that match smells:

OrderService.cs(31,20): warning CA1502: 'ProcessData' has a cyclomatic complexity of '27'. Rewrite or refactor the code to decrease its complexity below '26'.
OrderService.cs(31,20): warning S138: This method 'ProcessData' has 127 lines, which is greater than the 80 lines authorized. Split it into smaller methods.
OrderService.cs(31,20): warning S3776: Refactor this method to reduce its Cognitive Complexity from 92 to the 15 allowed.
OrderService.cs(252,41): warning S1168: Return an empty collection instead of null.
OrderService.cs(324,34): warning S107: Method has 9 parameters, which is greater than the 7 authorized.
OrderService.cs(439,25): warning S1144: Remove the unused private method 'OldCalcDiscount'.
OrderService.cs(446,9): warning S125: Remove this commented out code.
OrderService.cs(473,38): warning S3246: Add the 'in' keyword to parameter 'T' to make it 'contravariant'.

S3776 (Cognitive Complexity), S138 (method length) and CA1502 (cyclomatic complexity) found Long Function, S107 (parameter count) found Long Parameter List, and S1144 (unused private member) and S125 (commented-out code) found dead code. S1168 (returning null instead of a collection) isn't a smell, but it's the topic of the error handling post. Most of the remaining warnings are about public fields and static state: S1104 (public field) alone fired 21 times, so Data Class and Global Data show up indirectly.

Look at what they don't see: not a single warning for Feature Envy, Shotgun Surgery, Divergent Change, Message Chains or Duplicated Code. The most instructive one is S3246 (a suggestion to make a generic type parameter contravariant): instead of deleting IOrderProcessor<T>, which has no implementation at all, the analyzer suggests adding the in keyword to make it a better interface. Tools are good at local smells, the ones visible inside a single method or line; design-level smells require looking across several files and the change history. Setting up the rules, their thresholds and team-level use with SonarQube are in the upcoming Static Analysis series.

Common Pitfalls

Mistaking the smell for a diagnosis. A "Long Function" warning doesn't tell you to split the method, it tells you to look at it. Some long methods are a flat, readable algorithm; splitting them makes the reader jump between methods.

Trying to fill the catalog. The "no clear example" rows in the table are deliberate. Forcing the code to find a smell that isn't there steals time from the real ones.

Making the warning count a goal. Bringing 72 warnings down to zero doesn't fix Shotgun Surgery. Warnings are a good indicator of local quality, not of design.

Knowing the name but not the reason. The question behind every smell is the same: which future change does this structure make expensive or risky? If the answer is "none", the smell may be harmless.

Finding a smell and jumping straight into fixing it. In code without tests, the safest first step is often not the fix but pinning the current behavior with tests. Post 14 starts with exactly that.

A Small Experiment

★☆☆ Go back to CancelOrder in the opening and list the smells using the names in the table. How many did you find? Which ones did your first guess miss?

★★☆ Put LEGACY_ORDERSERVICE.cs into your own project, add the .editorconfig above and count the warnings by rule ID. Which smells got no warning at all?

★★★ Find at least three smells in the Report(DateTime from, DateTime to, bool csv) method and pick a refactoring from the catalog for each. Which would you apply first, and why?

In this post we gave the smells names. In the next post we question the rules behind those names: John Ousterhout's 2024–2025 debate with Martin, Casey Muratori's performance critique, and where the rules end.