Code Smells: Identifying Bad Design
Martin Fowler popularized the term 'code smell' in his 1999 book *Refactoring*. A code smell is a surface-level indication that something deeper is wrong with your code's design. The code works—it...
Key Insights
- Code smells aren’t bugs—they’re symptoms of design problems that make code harder to understand, modify, and maintain over time.
- Most smells fall into five categories: bloaters, OO abusers, change preventers, dispensables, and couplers—each pointing to specific structural issues.
- The best defense against code smells is continuous refactoring during development, not periodic “cleanup sprints” that never happen.
What Are Code Smells?
Martin Fowler popularized the term “code smell” in his 1999 book Refactoring. A code smell is a surface-level indication that something deeper is wrong with your code’s design. The code works—it passes tests, it ships features—but it’s structured in a way that will cause pain later.
Code smells aren’t bugs. Your application won’t crash because of a long method or duplicated code. But these structural problems compound. They make the next feature harder to add, the next bug harder to find, and the next developer (including future you) more likely to introduce regressions.
The key insight is that smells are heuristics, not rules. A 100-line method might be perfectly fine if it’s a straightforward data transformation. A switch statement isn’t automatically bad. Context matters. But when you notice a smell, you should pause and ask: is there a design problem here?
Bloaters: When Code Gets Too Big
Bloaters are the most obvious smells. They’re pieces of code that have grown so large they become difficult to work with. The three main bloaters are Long Methods, Large Classes, and Primitive Obsession.
Long Methods do too many things. They’re hard to understand, hard to test, and hard to reuse. When you need to add a comment explaining what the next section does, that section should probably be its own method.
Large Classes violate the Single Responsibility Principle. They become “god objects” that know too much and do too much. When a class has 50 methods and 20 fields, it’s doing the work of several classes.
Primitive Obsession means using primitives instead of small objects for simple tasks. Representing money as a float, phone numbers as strings, or date ranges as two separate fields.
Here’s a bloated method I’ve seen variations of in production code:
def process_order(order_data):
# Validate order
if not order_data.get('customer_id'):
raise ValueError("Missing customer ID")
if not order_data.get('items'):
raise ValueError("No items in order")
for item in order_data['items']:
if item['quantity'] <= 0:
raise ValueError(f"Invalid quantity for {item['product_id']}")
if item['price'] < 0:
raise ValueError(f"Invalid price for {item['product_id']}")
# Calculate totals
subtotal = 0
for item in order_data['items']:
subtotal += item['quantity'] * item['price']
# Apply discounts
discount = 0
if order_data.get('coupon_code'):
coupon = db.get_coupon(order_data['coupon_code'])
if coupon and coupon['valid']:
if coupon['type'] == 'percentage':
discount = subtotal * (coupon['value'] / 100)
else:
discount = coupon['value']
# Calculate tax
tax_rate = get_tax_rate(order_data['shipping_address']['state'])
tax = (subtotal - discount) * tax_rate
# ... another 150 lines for inventory, payment, shipping, notifications
The fix is extraction. Each comment block becomes its own method:
def process_order(order_data):
validate_order(order_data)
subtotal = calculate_subtotal(order_data['items'])
discount = apply_discounts(subtotal, order_data.get('coupon_code'))
tax = calculate_tax(subtotal - discount, order_data['shipping_address'])
order = create_order(order_data, subtotal, discount, tax)
reserve_inventory(order)
process_payment(order)
schedule_shipping(order)
send_confirmation(order)
return order
Each extracted method is now testable in isolation, reusable elsewhere, and named to communicate intent.
Object-Orientation Abusers
These smells occur when object-oriented principles are misapplied or ignored entirely. The most common are Switch Statements on type, Refused Bequest (inheriting methods you don’t want), and Temporary Fields (fields only used in certain situations).
Switch statements on type are particularly insidious because they tend to replicate. Add a new type, and you’re hunting through the codebase for every switch that needs updating.
// Smell: type-checking switch statement
public double calculateShippingCost(Order order) {
switch (order.getShippingMethod()) {
case "standard":
return order.getWeight() * 0.5;
case "express":
return order.getWeight() * 1.5 + 10.0;
case "overnight":
return order.getWeight() * 3.0 + 25.0;
default:
throw new IllegalArgumentException("Unknown shipping method");
}
}
The polymorphic solution pushes the behavior into the types themselves:
public interface ShippingMethod {
double calculateCost(double weight);
}
public class StandardShipping implements ShippingMethod {
public double calculateCost(double weight) {
return weight * 0.5;
}
}
public class ExpressShipping implements ShippingMethod {
public double calculateCost(double weight) {
return weight * 1.5 + 10.0;
}
}
// Usage
public double calculateShippingCost(Order order) {
return order.getShippingMethod().calculateCost(order.getWeight());
}
Now adding a new shipping method means adding a new class, not modifying existing code. The Open-Closed Principle in action.
Change Preventers: Rigidity in Code
Change preventers make modification painful. Divergent Change occurs when one class is modified for multiple unrelated reasons. Shotgun Surgery is the opposite—one change requires modifications across many classes.
I once worked on a codebase where adding a new field to a user profile required changes in 14 files: the database migration, the ORM model, the API serializer, the form validator, the frontend types, three different React components, two API endpoints, the search indexer, the export service, and the audit logger.
This is shotgun surgery. Here’s a simplified example:
// Smell: Adding a field requires changes everywhere
// user.model.ts
interface User { id: string; name: string; email: string; }
// user.api.ts
function createUser(name: string, email: string) { /* ... */ }
// user.validator.ts
function validateUser(data: any) {
if (!data.name || !data.email) throw new Error('Invalid');
}
// user.serializer.ts
function serializeUser(user: User) {
return { id: user.id, name: user.name, email: user.email };
}
The solution is consolidation—centralize related logic:
// user.ts - Single source of truth
const UserSchema = z.object({
id: z.string().uuid(),
name: z.string().min(1),
email: z.string().email(),
});
type User = z.infer<typeof UserSchema>;
function createUser(data: unknown): User {
return UserSchema.parse(data);
}
function serializeUser(user: User): Record<string, unknown> {
return UserSchema.parse(user);
}
Adding a field now means updating one schema. The validation, typing, and serialization all derive from the same source.
Dispensables: Code That Shouldn’t Exist
Dispensables are code that adds no value. Dead Code is never executed. Speculative Generality is abstraction built for future requirements that never materialize. Duplicate Code is the same logic in multiple places.
Dead code is surprisingly common. Features get removed but their supporting code remains. Parameters are added “just in case” and never used.
# Smell: YAGNI abstraction and unused parameters
class AbstractDataProcessorFactory:
def create_processor(self, config, legacy_mode=False, experimental=False):
# legacy_mode hasn't been True since 2019
# experimental was never implemented
return DataProcessor(config)
class DataProcessor:
def process(self, data, callback=None, retry_count=3, timeout=None):
# callback is always None
# retry_count is always 3
# timeout is never set
return self._do_process(data)
The fix is deletion. Remove what you don’t use:
class DataProcessor:
def process(self, data):
return self._do_process(data)
If you need retry logic later, add it later. Version control means nothing is truly gone.
Couplers: Unhealthy Relationships
Couplers indicate excessive coupling between classes. Feature Envy occurs when a method uses another object’s data more than its own. Inappropriate Intimacy means classes that know too much about each other’s internals. Message Chains are long sequences of method calls navigating object structures.
# Smell: Feature Envy - this method wants to be in Order
class OrderPrinter
def print_summary(order)
puts "Order ##{order.id}"
puts "Customer: #{order.customer.name}"
puts "Items: #{order.items.count}"
puts "Subtotal: #{order.items.sum(&:price)}"
puts "Tax: #{order.items.sum(&:price) * order.tax_rate}"
puts "Total: #{order.items.sum(&:price) * (1 + order.tax_rate)}"
end
end
The method accesses order constantly but never uses self. It belongs in Order:
class Order
def summary
<<~SUMMARY
Order ##{id}
Customer: #{customer.name}
Items: #{items.count}
Subtotal: #{subtotal}
Tax: #{tax}
Total: #{total}
SUMMARY
end
def subtotal = items.sum(&:price)
def tax = subtotal * tax_rate
def total = subtotal + tax
end
Now OrderPrinter can simply call order.summary if it still needs to exist at all.
Detection and Prevention Strategies
Knowing the smells is only half the battle. You need systems to catch them.
Static analysis tools catch many smells automatically. Configure your linter to flag long methods, deep nesting, and high complexity scores. Tools like SonarQube, CodeClimate, or language-specific analyzers (Rubocop, ESLint, Pylint) can be tuned to your team’s tolerances.
Code review checklists should include smell detection. Ask reviewers to specifically look for: methods over 20 lines, classes with more than one reason to change, duplicated logic, and unused parameters.
Refactor continuously. The Boy Scout Rule—leave code cleaner than you found it—prevents smell accumulation. Don’t wait for a refactoring sprint. When you touch code, improve it slightly.
Write tests first. TDD naturally produces smaller, focused methods because you’re designing for testability. Code that’s hard to test usually smells.
The goal isn’t perfection. It’s awareness. When you recognize a smell, you can make an informed decision: fix it now, note it for later, or accept it as a reasonable tradeoff. That conscious choice is what separates professional software engineering from just making it work.