Mahdi SHamlou | مهدی شاملو

Posted on Jul 10

Mahdi Shamlou | 24 Code Smells and How to Fix Them: A Complete Refactoring Guide

#ai #webdev #programming #productivity

A while back I read Refactoring: Improving the Design of Existing Code (2nd Edition) by Martin Fowler, with Kent Beck. I'll say it plainly: it's one of the best software books I've read. Not because it teaches you a new language or framework, but because it gives you vocabulary — a name for the thing that's bugging you about a piece of code, and a precise, mechanical answer for what to do about it.

If you've followed this series, you already know the shape of the problem from the SOLID article: giant classes, endless elif chains, subclasses that throw NotImplementedError, fat interfaces, hardcoded infrastructure. Fowler's book is where most of that vocabulary actually comes from. This article walks through his full list of 24 code smells — what each one looks like in Python, and which named refactoring from his catalog fixes it.

One thing worth understanding before the list: in the book, "the catalog" isn't a single tool — it's the reference section of the book itself, roughly 60 individually named refactorings (Extract Function, Move Field, Replace Conditional with Polymorphism...), each written up the same way: a name, a motivation, and mechanics — the actual steps to do it safely. Chapter 3 diagnoses the smell. The catalog is where the treatment lives. That's the pairing this whole article is built around: smell → catalog entry → fixed.

Before I Read This Book

Before I actually sat down and read this book, my process looked like this: I'd open a file, feel that something was off — a function too long, a class doing too much, a chain of .get_x().get_y().get_z() calls — and I'd fix it. Not badly, usually. I'd split the function, move the logic somewhere that made more sense, flatten the nested ifs. It worked. But it was all intuition. I had no name for what I was looking at, no reason to expect a specific fix would work beyond "this feels better now," and no way to tell someone else "this is a Feature Envy problem" instead of just showing them the diff and hoping they saw what I saw.

What this book actually gives you isn't a new way to write code — it's the realization that the thing you were already sensing has a name, has been seen thousands of times before, and has an exact, repeatable answer. "This function is too long" becomes Long Function, and the fix isn't "break it up somehow," it's Extract Function, with defined steps. "This code reaches into another object's data too much" becomes Feature Envy, fixed with Move Function. It's the difference between a doctor saying "you don't look well" and a doctor saying "this is strep, here's the treatment." Same instinct that something's wrong — but one of them actually tells you what to do next, and lets you say it to someone else in one word instead of pointing at a screen.

That's the real value of turning intuition into a catalog: it's teachable, it's shareable across a team, and it stops the fix from depending on how much experience you personally happen to have that day.

Here's the full list, in the order the book presents them:

Mysterious Name
Duplicated Code
Long Function
Long Parameter List
Global Data
Mutable Data
Divergent Change
Shotgun Surgery
Feature Envy
Data Clumps
Primitive Obsession
Repeated Switches
Loops
Lazy Element
Speculative Generality
Temporary Field
Message Chains
Middle Man
Insider Trading
Large Class
Alternative Classes with Different Interfaces
Data Class
Refused Bequest
Comments

Let's go through them one at a time.

1. Mysterious Name

A function, variable, or class whose name tells you nothing about what it actually does. You end up opening the body every single time just to remember what it's for.

def proc(d, f):
    if f:
        return d * 0.9
    return d

What does proc process? What's d? What's f? You can't tell without reading the body — and every caller has to re-derive that same context.

Catalog fix: Rename Variable / Change Function Declaration. These two refactorings do exactly what they sound like — rename a variable, or rename (and reshape, if needed) a function's signature — but Fowler treats naming as an ongoing, first-class refactoring activity, not a one-time nicety you do at the start and never revisit. Every time you understand a piece of code a little better, that's the moment to rename it to reflect what you now know.

def apply_discount(price, is_member):
    if is_member:
        return price * 0.9
    return price

2. Duplicated Code

The exact same code structure showing up in more than one place. It's the most common smell in real codebases, and the most dangerous — because when the rule changes, you have to remember to update every copy, and you never do.

def invoice_total(order):
    tax = order.amount * 0.08
    return order.amount + tax

def report_total(sale):
    tax = sale.amount * 0.08
    return sale.amount + tax

Catalog fix: Extract Function. Pull the duplicated logic into one named function, and have every call site use that function instead of its own copy.

def with_tax(amount):
    return amount * 1.08

def invoice_total(order):
    return with_tax(order.amount)

def report_total(sale):
    return with_tax(sale.amount)

If the duplication is between sibling subclasses instead of free functions, the matching catalog entry is Pull Up Method — move the shared method up into the common parent class instead.

3. Long Function

def process_order(order):
    if not order.items:
        raise ValueError("Order has no items")

    discount = 0
    if order.customer.is_vip:
        discount = order.total * 0.1

    tax = (order.total - discount) * 0.08
    shipping = 5.99 if order.total < 50 else 0

    final_total = order.total - discount + tax + shipping
    order.final_total = final_total
    db.save(order)
    print(f"Order confirmed. Total: {final_total}")
    return final_total

Five unrelated jobs — validation, discount, tax, shipping, persistence, notification — all crammed into one function you have to read top to bottom to understand any single piece of.

Catalog fix: Extract Function. The same refactoring as Duplicated Code, applied for a different reason: instead of pulling out something that's repeated elsewhere, you pull out a self-contained chunk of a long function and give it its own name, purely so the outer function reads like a short list of steps.

def validate_order(order):
    if not order.items:
        raise ValueError("Order has no items")

def calculate_discount(order):
    return order.total * 0.1 if order.customer.is_vip else 0

def calculate_tax(amount):
    return amount * 0.08

def calculate_shipping(total):
    return 5.99 if total < 50 else 0

def process_order(order):
    validate_order(order)
    discount = calculate_discount(order)
    taxable = order.total - discount
    order.final_total = taxable + calculate_tax(taxable) + calculate_shipping(order.total)
    db.save(order)
    print(f"Order confirmed. Total: {order.final_total}")
    return order.final_total

4. Long Parameter List

def create_user(name, email, phone, street, city, state, zip_code, country):
    ...

create_user("Ana", "ana@x.com", "555-1234", "1 Main St", "Metropolis", "NY", "10001", "US")

At the call site, you have no idea which string is which without checking the signature.

Catalog fix: Introduce Parameter Object. Group the parameters that always travel together into one object, and pass that object instead of the individual values.

from dataclasses import dataclass

@dataclass
class Address:
    street: str
    city: str
    state: str
    zip_code: str
    country: str

def create_user(name, email, phone, address: Address):
    ...

If the caller already has the whole object and is needlessly pulling fields out of it to pass in, Fowler's related entry is Preserve Whole Object — just pass the object itself instead of pulling values out of it first.

5. Global Data

Mutable data reachable and changeable from anywhere in the program — module-level variables, singletons, anything any function can quietly reach in and modify.

current_discount = 0.1

def apply_discount(price):
    return price * (1 - current_discount)

# somewhere else, in a completely unrelated file:
current_discount = 0.5  # anyone can change this, from anywhere, at any time

Catalog fix: Encapsulate Variable. Wrap access to the global behind a function (a getter, and a setter if writes are genuinely needed), so every read and write goes through one place you can control, log, or validate.

_discount = 0.1

def get_discount():
    return _discount

def set_discount(value):
    global _discount
    _discount = value

def apply_discount(price):
    return price * (1 - get_discount())

6. Mutable Data

Data that changes in ways that are hard to trace — one part of the code mutates a value, and somewhere far away, unrelated code breaks because it expected the old value.

def apply_bonus(scores):
    scores[0] += 10  # mutates the caller's list in place
    return scores

original = [80, 90, 70]
result = apply_bonus(original)
# original is now also changed — surprising if the caller didn't expect that

Catalog fix: Encapsulate Variable, together with a general shift toward computing values through query methods instead of storing and mutating them directly. When you must have mutation, Fowler's advice is to keep the surface where it can happen as small and explicit as possible.

def apply_bonus(scores):
    return [scores[0] + 10] + scores[1:]  # returns a new list, doesn't mutate the input

original = [80, 90, 70]
result = apply_bonus(original)
# original is untouched

7. Divergent Change

One class that has to change for several unrelated reasons — a new report format, a new tax rule, and a new persistence detail, all touching the same file.

class OrderService:
    def save(self, order): ...          # changes if the database changes
    def calculate_tax(self, order): ...  # changes if tax law changes
    def generate_report(self, order): ...  # changes if reporting needs change

Catalog fix: Extract Class. Split the class along the different reasons it changes — one class per responsibility — so a tax-law change only ever touches the tax class.

class OrderRepository:
    def save(self, order): ...

class TaxCalculator:
    def calculate(self, order): ...

class OrderReportGenerator:
    def generate(self, order): ...

This is the same smell SRP addresses in the SOLID article — Fowler names it explicitly in this chapter.

8. Shotgun Surgery

The mirror image of Divergent Change: one conceptual change forces you to make small edits across many different files.

# pricing.py
TAX_RATE = 0.08

# invoice.py
tax = amount * 0.08  # duplicated rate

# report.py
total = sales * 1.08  # same rate, third form

Change the tax rate, and you have to hunt across three files, hoping you found every copy.

Catalog fix: Move Function / Move Field. Pull the scattered, related pieces of logic back together into one place, so the next change touches one file instead of three.

# tax.py
class TaxPolicy:
    RATE = 0.08

    @classmethod
    def apply(cls, amount):
        return amount * (1 + cls.RATE)

9. Feature Envy

class InvoicePrinter:
    def print_invoice(self, order):
        name = order.customer.name
        discount = order.customer.loyalty_tier * 0.02
        address = order.customer.address.format()
        print(f"{name} ({discount}% off)\n{address}")

print_invoice reaches into order.customer three times — it's more interested in Customer's data than its own.

Catalog fix: Move Function. Relocate the method to live on the class whose data it actually uses.

class Customer:
    def invoice_summary(self):
        discount = self.loyalty_tier * 0.02
        return f"{self.name} ({discount}% off)\n{self.address.format()}"

class InvoicePrinter:
    def print_invoice(self, order):
        print(order.customer.invoice_summary())

10. Data Clumps

def create_user(name, email, street, city, state, zip_code): ...
def update_address(user, street, city, state, zip_code): ...

street, city, state, zip_code show up together, everywhere. That's the tell: if you'd delete one, the others stop making sense alone — they're one concept wearing four names.

Catalog fix: Extract Class (or Introduce Parameter Object if it's specifically a parameter list problem, as in the example above).

from dataclasses import dataclass

@dataclass
class Address:
    street: str
    city: str
    state: str
    zip_code: str

def create_user(name, email, address: Address): ...
def update_address(user, address: Address): ...

11. Primitive Obsession

Using raw strings, ints, and floats for things that have their own rules and behavior — money, percentages, phone numbers, date ranges — instead of small dedicated types.

def apply_discount(price: float, percent: float) -> float:
    return price * (1 - percent / 100)

apply_discount(100, 150)  # no validation — silently returns a negative price

Catalog fix: Replace Primitive with Object. Wrap the primitive in a small class that owns its own validation and behavior.

class Percentage:
    def __init__(self, value: float):
        if not 0 <= value <= 100:
            raise ValueError("Invalid percentage")
        self.value = value

    def apply_to(self, amount: float) -> float:
        return amount * (1 - self.value / 100)

Percentage(150)  # raises immediately, instead of failing silently downstream

12. Repeated Switches

The same if/elif (or switch) chain, checking the same condition, copy-pasted in multiple places.

def calculate_pay(employee):
    if employee.type == "engineer":
        return employee.base_salary
    elif employee.type == "manager":
        return employee.base_salary * 1.2

def calculate_bonus(employee):
    if employee.type == "engineer":
        return 500
    elif employee.type == "manager":
        return 1500
# add a new employee type, and you must find and update every one of these chains

Catalog fix: Replace Conditional with Polymorphism. Turn each branch into a subclass method, so a new type means one new class instead of N updated chains.

from abc import ABC, abstractmethod

class Employee(ABC):
    def __init__(self, base_salary):
        self.base_salary = base_salary

    @abstractmethod
    def calculate_pay(self): ...
    @abstractmethod
    def calculate_bonus(self): ...

class Engineer(Employee):
    def calculate_pay(self):
        return self.base_salary
    def calculate_bonus(self):
        return 500

class Manager(Employee):
    def calculate_pay(self):
        return self.base_salary * 1.2
    def calculate_bonus(self):
        return 1500

13. Loops

Old-style manual iteration, where a pipeline (map/filter/reduce style) would say the intent more directly.

result = []
for user in users:
    if user.active:
        result.append(user.email)

Catalog fix: Replace Loop with Pipeline. Express the same operation as a chain of filter/map-style steps.

result = [user.email for user in users if user.active]

14. Lazy Element

A function or class that no longer earns its keep — created for a reason that no longer applies, now just forwarding to something else with no logic of its own.

class UserValidator:
    def validate(self, user):
        return basic_email_check(user.email)  # the whole class does nothing but this

Catalog fix: Inline Function / Inline Class / Collapse Hierarchy. Remove the unnecessary layer and let callers use the underlying thing directly.

def validate_user(user):
    return basic_email_check(user.email)

15. Speculative Generality

Abstraction, hooks, and parameters built for a future requirement that hasn't arrived — and might never.

class ReportGenerator:
    def generate(self, data, format="pdf", plugin_hooks=None, future_export_target=None):
        # only format="pdf" has ever actually been called, in two years
        ...

Catalog fix: Collapse Hierarchy, Inline Function, Remove Dead Code, and simply deleting unused parameters — pruning the code back down to what's actually used today, not what might be needed someday.

class ReportGenerator:
    def generate(self, data):
        return render_pdf(data)

16. Temporary Field

An object field that's only ever set and used inside one specific method, and sits empty the rest of the time — confusing to anyone reading the class from outside.

class OrderProcessor:
    def __init__(self):
        self.temp_discount = None  # only meaningful during process()

    def process(self, order):
        self.temp_discount = order.total * 0.1
        return order.total - self.temp_discount

Catalog fix: Extract Class. Pull the field and the method that uses it into their own small object, so the temporary state doesn't leak into the rest of the class's lifetime.

class DiscountCalculation:
    def __init__(self, order):
        self.discount = order.total * 0.1

    def final_total(self, order):
        return order.total - self.discount

class OrderProcessor:
    def process(self, order):
        return DiscountCalculation(order).final_total(order)

17. Message Chains

color = order.get_customer().get_profile().get_settings().get_theme().get_color()

Four dots deep — if any link in that chain ever changes, every caller like this one breaks.

Catalog fix: Hide Delegate. Wrap the chain behind one method on the object you actually start from.

class Customer:
    def get_theme_color(self):
        return self.profile.settings.theme.get_color()

color = order.get_customer().get_theme_color()

18. Middle Man

The opposite extreme: a class where most methods just forward, one-for-one, to another object, adding no logic of their own.

class OrderFacade:
    def __init__(self, order_service):
        self.order_service = order_service

    def create_order(self, data):
        return self.order_service.create_order(data)
    # ...every other method, forwarded identically

Catalog fix: Remove Middle Man. Let callers talk to the real object directly instead of going through a layer that adds nothing.

order_service.create_order(data)

19. Insider Trading

Two modules that reach into each other's internals too much, trading data back and forth in ways that create tight, hidden coupling — beyond ordinary collaboration.

class Warehouse:
    def __init__(self):
        self._stock = {}

class ShippingService:
    def ship(self, warehouse: Warehouse, item):
        warehouse._stock[item] -= 1  # reaching directly into Warehouse's private data

Catalog fix: Move Function / Move Field, reducing how much crosses the boundary, or Hide Delegate if the interaction should be mediated through a proper method instead of direct access.

class Warehouse:
    def __init__(self):
        self._stock = {}

    def remove_stock(self, item):
        self._stock[item] -= 1

class ShippingService:
    def ship(self, warehouse: Warehouse, item):
        warehouse.remove_stock(item)

20. Large Class

A class trying to do too much — recognizable by an unreasonably long list of fields and methods that don't obviously belong together.

class UserService:
    def create_user(self): ...
    def validate_email(self): ...
    def send_welcome_email(self): ...
    def log_activity(self): ...
    def generate_invoice(self): ...
    def calculate_tax(self): ...

Catalog fix: Extract Class, splitting the fields and methods that change together into their own smaller classes (or Extract Superclass / Replace Type Code with Subclasses if the bloat comes from handling several variants inside one class).

class UserValidator: ...
class WelcomeEmailSender: ...
class ActivityLogger: ...
class InvoiceService: ...

21. Alternative Classes with Different Interfaces

Two classes doing the same job, but shaped differently enough that you can't swap one for the other without extra glue code at every call site.

class MySQLClient:
    def fetch_row(self, query): ...

class MongoClient:
    def find_document(self, query): ...
# functionally the same job, but callers can't treat them interchangeably

Catalog fix: Change Function Declaration and Move Function, aligning the two interfaces until they can share one abstraction — often followed by Extract Superclass.

class MySQLClient:
    def fetch(self, query): ...

class MongoClient:
    def fetch(self, query): ...
# both now expose the same shape, and can be swapped behind a common interface

22. Data Class

A class that's all getters/setters and public fields, with none of the behavior that actually operates on that data living nearby.

class Order:
    def __init__(self, total, discount):
        self.total = total
        self.discount = discount
# meanwhile, the discount math lives scattered across three unrelated files

Catalog fix: Move Function, bringing the relevant behavior back onto the class, plus Encapsulate Record / Remove Setting Method to stop letting every caller mutate it freely from the outside.

class Order:
    def __init__(self, total, discount):
        self._total = total
        self._discount = discount

    def final_total(self):
        return self._total - self._discount

23. Refused Bequest

A subclass that inherits from a parent but only wants part of what it inherits — overriding methods to throw, or quietly breaking the parent's contract.

class Rectangle:
    def set_width(self, width):
        self.width = width
    def set_height(self, height):
        self.height = height
    def area(self):
        return self.width * self.height

class Square(Rectangle):
    def set_width(self, width):
        self.width = self.height = width  # silently breaks Rectangle's contract

This is exactly the Square/Rectangle example from the SOLID article's LSP section.

Catalog fix: Push Down Method / Push Down Field (move the part the subclass doesn't want out of the shared parent), or Replace Subclass with Delegate if inheritance was the wrong relationship to begin with.

class Shape:
    def area(self): ...

class Rectangle(Shape):
    def __init__(self, width, height):
        self.width, self.height = width, height
    def area(self):
        return self.width * self.height

class Square(Shape):
    def __init__(self, side):
        self.side = side
    def area(self):
        return self.side ** 2

24. Comments

The last smell in the chapter, and the most misunderstood one. Fowler is explicit that comments themselves aren't the problem — a comment used as a deodorant, covering up confusing code instead of fixing it, is.

def calc(o):
    # check if order total is over 1000 and customer is vip, then apply 10% off
    if o.total > 1000 and o.customer.is_vip:
        return o.total * 0.9
    return o.total

If you need a comment to explain what a block of code does, that's usually a sign the code itself should say it.

Catalog fix: Extract Function. Turn the commented block into a well-named function, and let the name carry the explanation the comment used to.

def is_eligible_for_vip_discount(order):
    return order.total > 1000 and order.customer.is_vip

def calc(order):
    if is_eligible_for_vip_discount(order):
        return order.total * 0.9
    return order.total

A comment explaining why — a business reason, a workaround for someone else's bug, a warning about something non-obvious — is worth keeping. Fowler isn't anti-comment; he's anti-comment-as-substitute-for-clarity.

Smell → Catalog Quick Reference

Smell	Catalog Fix
Mysterious Name	Rename Variable / Change Function Declaration
Duplicated Code	Extract Function / Pull Up Method
Long Function	Extract Function
Long Parameter List	Introduce Parameter Object / Preserve Whole Object
Global Data	Encapsulate Variable
Mutable Data	Encapsulate Variable
Divergent Change	Extract Class
Shotgun Surgery	Move Function / Move Field
Feature Envy	Move Function
Data Clumps	Extract Class / Introduce Parameter Object
Primitive Obsession	Replace Primitive with Object
Repeated Switches	Replace Conditional with Polymorphism
Loops	Replace Loop with Pipeline
Lazy Element	Inline Function / Inline Class / Collapse Hierarchy
Speculative Generality	Collapse Hierarchy / Inline Function / Remove Dead Code
Temporary Field	Extract Class
Message Chains	Hide Delegate
Middle Man	Remove Middle Man
Insider Trading	Move Function / Move Field / Hide Delegate
Large Class	Extract Class / Extract Superclass
Alternative Classes, Different Interfaces	Change Function Declaration / Move Function
Data Class	Move Function / Encapsulate Record
Refused Bequest	Push Down Method / Push Down Field / Replace Subclass with Delegate
Comments	Extract Function

Key Takeaways

Every smell in this list is a symptom, never the disease itself — it just tells you where to look
Every fix is a named, mechanical refactoring from the book's catalog, not a vague "clean it up"
Most of these smells map directly onto a SOLID violation — SRP shows up as Divergent Change and Large Class, LSP shows up as Refused Bequest, OCP shows up as Repeated Switches
The single most-used fix across the whole list is Extract Function — if in doubt, that's usually the first thing to reach for
Not every fix requires a class — several of these (Loops, Comments, Mutable Data) are fixed with nothing more than a better function

If you take one thing from this book, take this: you don't need to memorize the whole catalog before you start. You just need to notice the smell, look up its entry, and apply it — one small, safe step at a time.

Design Patterns & Architecture Series

Creational Patterns — How to create objects properly
Structural Patterns - Part 1 — Adapter, Bridge, Composite, Decorator
Structural Patterns - Part 2 — Facade, Flyweight, Proxy
Behavioral Patterns — How objects communicate
OOP in Python — The foundations everything else builds on
SOLID Principles — The reasoning behind every pattern above
24 Code Smells and How to Fix Them (This Article) — Fowler's Refactoring, smell by smell

Want More Deep Dives?

If you enjoyed this article, check out my other production-focused guides:

Message Brokers in 2026: Kafka, RabbitMQ, NATS — Architecture decisions
OWASP Top 10 for Developers (2026 Edition) — Security patterns
Injection Attacks Are Not Dead — Attack patterns and solutions
Durable Workflow Engines: Temporal vs dbt OS — System patterns

🔗 LinkedIn: https://www.linkedin.com/in/mahdi-shamlou-3b52b8278
📱 Telegram: https://telegram.me/mahdi0shamlou
📸 Instagram: https://www.instagram.com/mahdi0shamlou/

Author: Mahdi Shamlou | مهدی شاملو

DEV Community