Recognizing Code Smells
I was debugging a checkout bug last year. The test passed. The code looked correct. But production was rejecting valid orders.
The issue wasn't a logic error. It was a 200-line function that validated, calculated, charged, and notified—all in one place. The bug was a subtle interaction between validation and calculation that only surfaced under specific conditions. I couldn't add a targeted test because I couldn't isolate the behavior.
That function had a code smell. Not a bug—a structural problem that made bugs inevitable.
Code Smell vs. Bug
A bug is incorrect behavior. The code does the wrong thing. Fix it.
A code smell is a structural problem that suggests something might be wrong. The code might work today, but it's hard to understand, hard to change, and prone to future bugs.
The distinction matters. You fix bugs. You refactor smells.
Some smells are acceptable. A small script that runs once doesn't need the same structure as a production service. Context matters. The goal is to recognize smells so you can decide when to act.
Long Method
A method that does too much. You scroll. You lose the thread. You can't test one piece in isolation.
// Smell: One method doing validation, calculation, formatting, and persistence
function processOrder(order) {
if (!order) throw new Error('Order required');
if (!order.items || order.items.length === 0) throw new Error('No items');
if (!order.customer) throw new Error('Customer required');
if (!order.customer.email) throw new Error('Email required');
let subtotal = 0;
for (const item of order.items) {
subtotal += item.price * item.quantity;
}
const tax = subtotal * 0.08;
const shipping = subtotal > 100 ? 0 : 9.99;
const total = subtotal + tax + shipping;
const receipt = `Order #${order.id}\n`;
receipt += `Subtotal: $${subtotal.toFixed(2)}\n`;
receipt += `Tax: $${tax.toFixed(2)}\n`;
receipt += `Shipping: $${shipping.toFixed(2)}\n`;
receipt += `Total: $${total.toFixed(2)}\n`;
await db.orders.insert({ ...order, total, status: 'completed' });
await emailService.send(order.customer.email, 'Order Confirmation', receipt);
return { orderId: order.id, total };
}Refactoring trigger: Extract validation, calculation, formatting, and persistence into separate functions. Each does one thing.
Large Class
A class that knows and does too much. It has many fields, many methods, and unclear responsibilities.
// Smell: OrderProcessor handles validation, pricing, persistence, notifications, and analytics
class OrderProcessor {
validateOrder(order) { /* ... */ }
calculateTotal(order) { /* ... */ }
applyDiscounts(order, user) { /* ... */ }
chargePayment(order, paymentInfo) { /* ... */ }
updateInventory(items) { /* ... */ }
sendConfirmationEmail(order) { /* ... */ }
logAnalytics(event, data) { /* ... */ }
generateInvoice(order) { /* ... */ }
scheduleShipment(order) { /* ... */ }
// 15 more methods...
}Refactoring trigger: Split into focused classes: OrderValidator, OrderPricer, PaymentProcessor, OrderNotifier. Each owns one concern.
Feature Envy
A method that uses another class's data more than its own. It's in the wrong place.
// Smell: ReportGenerator is obsessed with Order's internals
class ReportGenerator {
formatOrderSummary(order) {
return `${order.customer.name} - ${order.items.length} items - $${order.total}`;
}
}
// Better: Order knows how to summarize itself
class Order {
getSummary() {
return `${this.customer.name} - ${this.items.length} items - $${this.total}`;
}
}Refactoring trigger: Move the method to the class that owns the data. If that creates coupling issues, consider whether the data should be restructured.
Data Clumps
The same group of parameters appears together everywhere. They travel as a pack because they belong together.
// Smell: street, city, zip always appear together
function validateAddress(street, city, zip) { /* ... */ }
function formatAddress(street, city, zip) { /* ... */ }
function saveAddress(street, city, zip) { /* ... */ }
function displayAddress(street, city, zip) { /* ... */ }Refactoring trigger: Extract a value object. Address with street, city, zip. One parameter instead of three.
interface Address {
street: string;
city: string;
zip: string;
}
function validateAddress(address: Address) { /* ... */ }
function formatAddress(address: Address) { /* ... */ }Primitive Obsession
Using primitives (strings, numbers) where a small object would add meaning and behavior.
// Smell: What is this string? What values are valid?
function setUserRole(userId, role) {
if (role === 'admin' || role === 'user' || role === 'guest') {
// ...
}
}
// Better: Role is a concept with validation
class Role {
static ADMIN = new Role('admin');
static USER = new Role('user');
static GUEST = new Role('guest');
constructor(value) {
if (!['admin', 'user', 'guest'].includes(value)) {
throw new Error(`Invalid role: ${value}`);
}
this.value = value;
}
}Refactoring trigger: When you have validation logic, formatting logic, or magic values around a primitive, consider a value object or enum.
Switch Statements
Repeated switch/if-else on the same type. Adding a new case requires finding and updating every switch.
// Smell: Switch on notification type everywhere
function sendNotification(notification) {
switch (notification.type) {
case 'email': return sendEmail(notification);
case 'sms': return sendSms(notification);
case 'push': return sendPush(notification);
default: throw new Error('Unknown type');
}
}
function formatNotification(notification) {
switch (notification.type) {
case 'email': return formatEmail(notification);
case 'sms': return formatSms(notification);
case 'push': return formatPush(notification);
default: throw new Error('Unknown type');
}
}Refactoring trigger: Polymorphism. Each notification type becomes a class with send() and format() methods. New types = new classes, no switch updates.
Parallel Hierarchies
Two class hierarchies that change together. Add a shape, you must add a shape drawer. Add a format, you must add a format handler.
// Smell: Circle and CircleDrawer, Rectangle and RectangleDrawer—always in sync
class Circle { }
class Rectangle { }
class CircleDrawer { draw(circle: Circle) { } }
class RectangleDrawer { draw(rect: Rectangle) { } }Refactoring trigger: Collapse or use a single hierarchy with a polymorphic draw() method on each shape.
How to Systematically Identify Smells
Don't rely on intuition alone. Use a checklist when reviewing code:
- Read the function/class name. Does it describe one thing? If not, it's probably doing too much.
- Count the lines. Long methods (>30 lines) and large classes (>10 methods) warrant a look.
- Trace the parameters. Do the same 3+ parameters appear together in multiple places? Data clump.
- Follow the data. Does this method touch another object's internals more than its own? Feature envy.
- Search for primitives. Are there magic strings, numbers, or repeated validation? Primitive obsession.
- Find the switches. Is there a switch that would grow when you add a type? Consider polymorphism.
Run this checklist during code review. Add one smell to your mental catalog each week. Over time, recognition becomes automatic.
When a Smell Is Acceptable
Not every smell demands immediate refactoring. Consider:
- Throwaway code: A migration script that runs once. Don't over-invest.
- Stable code: Code that hasn't changed in two years and isn't planned to change. Low priority.
- Time pressure: You're fixing a critical bug. Fix the bug. Add a TODO. Refactor when you're in that area next.
- Team capacity: Refactoring has a cost. If the team is underwater, address the highest-impact smells first.
The goal is awareness, not perfection. Recognize the smell. Assess the risk. Refactor when the cost of not refactoring exceeds the cost of refactoring.
Key insight: A code smell is a structural problem that suggests future trouble—not a bug, but a risk. Learn the catalog: Long Method, Large Class, Feature Envy, Data Clumps, Primitive Obsession, Switch Statements, Parallel Hierarchies. Use a systematic checklist to identify them. Refactor when the cost of inaction exceeds the cost of change.