What the term actually means, why your code can work perfectly and still stink and how to start recognising it before it becomes someone else’s problem to fix
Most developers have heard the term. Few can explain it without stumbling. Even fewer apply it consistently during code reviews. Code smell sits in that awkward space between “everyone nods when it comes up” and “almost nobody was explicitly taught what it means.”
Think of it like a car that drives fine but makes a strange noise every time you brake. Nothing is broken. You get to work. But a mechanic would hear that noise and know something underneath is worn, misaligned or about to fail. The car is not broken yet. It is telling you it will be.
Code smell is the same idea. The code runs. The tests pass. But something about the way it is written tells an experienced developer that it is going to cause pain. Maintenance will be slow. The next feature will be harder to add. A bug will hide there for three months before anyone finds it.
That is what code smell means. Not broken. Just quietly heading in the wrong direction.
Where the term came from
The term was coined by Kent Beck and later popularised by Martin Fowler in his 1999 book Refactoring: Improving the Design of Existing Code. Fowler defined a code smell as a surface indication that usually corresponds to a deeper problem in the system.
That framing is important. A smell is a signal, not a verdict. It tells you something is worth looking at. It does not tell you the code is broken.
What a code smell actually is
A code smell is any characteristic in your source code that hints at a deeper structural problem. The code compiles. It runs. Tests pass. Users are not complaining. And yet something about the code makes it harder to read, harder to change and harder to extend than it should be.
That last part is what matters. Code smell is not about whether the code works today. It is about whether the code will be painful to work with tomorrow.
A function that is 400 lines long is not broken. But it is suspicious. A class that imports from fifteen other modules is not incorrect. But it suggests something is wrong with how responsibilities are distributed. A variable named data2 in a codebase with data already defined is not a bug. But it signals that the person writing it was not thinking clearly about what they were trying to say.
None of these are errors. All of them are smells.
Why developers create smelly code
The reasons are ordinary. Deadline pressure means shortcuts get taken and never revisited. A feature that started small grows without anyone stepping back to redesign it. A junior developer copies a pattern they saw elsewhere without understanding what it was doing. A senior developer writes something clever and forgets that clever code is the hardest kind to maintain.
Smelly code is not the result of bad developers. It is the result of normal development pressure applied to a codebase without regular cleanup. Most teams have it. Most teams do not talk about it until the maintenance cost becomes impossible to ignore.
The most common code smells
Duplicated code
The same logic appears in more than one place. When requirements change, you update one copy and miss the other. Bugs get fixed in one location and silently persist everywhere else the same code was pasted. Duplication is the most common smell and the most straightforward to address: extract the shared logic into a single function or class and reference it from everywhere it is needed.
// Smelly
function getActiveAdmins(): array
{
return User::where('status', 'active')
->where('role', 'admin')
->orderBy('name')
->get()
->toArray();
}
function getActiveModerators(): array
{
return User::where('status', 'active')
->where('role', 'moderator')
->orderBy('name')
->get()
->toArray();
}
// Clean
function getActiveUsersByRole(string $role): array
{
return User::where('status', 'active')
->where('role', $role)
->orderBy('name')
->get()
->toArray();
}Long method
A function that does too much. If you need to scroll to read a function, or if you find yourself adding section comments inside it to make it navigable, the function is too long. The fix is to extract smaller functions with names that describe what each piece does. A well-named 10-line function is easier to understand than an unnamed 80-line block.
// Smelly
class OrderProcessor
{
public function processOrder(array $data): void
{
// Validate
if (empty($data['user_id'])) {
throw new \Exception('Missing user');
}
if (empty($data['items'])) {
throw new \Exception('No items');
}
// Calculate total
$total = 0;
foreach ($data['items'] as $item) {
$total += $item['price'] * $item['quantity'];
}
if ($data['coupon'] ?? null) {
$total -= $total * 0.1;
}
// Save order
$order = Order::create([
'user_id' => $data['user_id'],
'total' => $total,
'status' => 'pending',
]);
// Notify
Mail::to($data['email'])->send(new OrderConfirmation($order));
$order->update(['status' => 'confirmed']);
}
}
// Clean
class OrderProcessor
{
public function processOrder(array $data): void
{
$this->validateOrderData($data);
$total = $this->calculateTotal($data['items'], $data['coupon'] ?? null);
$order = $this->saveOrder($data['user_id'], $total);
$this->confirmOrder($order, $data['email']);
}
private function validateOrderData(array $data): void { /* ... */ }
private function calculateTotal(array $items, ?string $coupon): float { /* ... */ }
private function saveOrder(int $userId, float $total): Order { /* ... */ }
private function confirmOrder(Order $order, string $email): void { /* ... */ }
}God class
A class that knows too much and does too much. It has grown to contain logic that belongs in several other classes. Changing one part of it risks breaking something apparently unrelated. God classes often form around nouns like Manager, Handler, Processor or Service, which are vague enough to attract unrelated responsibilities over time.
// Smelly: one class doing everything
class UserManager
{
public function register(array $data): User { /* ... */ }
public function login(string $email, string $password): string { /* ... */ }
public function sendWelcomeEmail(User $user): void { /* ... */ }
public function generateInvoice(User $user): string { /* ... */ }
public function updateSubscription(User $user, string $plan): void { /* ... */ }
public function logActivity(User $user, string $action): void { /* ... */ }
public function exportUsersToCsv(): string { /* ... */ }
}
// Clean: responsibilities split across focused classes
class UserRegistrar
{
public function register(array $data): User { /* ... */ }
}
class AuthService
{
public function login(string $email, string $password): string { /* ... */ }
}
class UserMailer
{
public function sendWelcomeEmail(User $user): void { /* ... */ }
}
class BillingService
{
public function generateInvoice(User $user): string { /* ... */ }
public function updateSubscription(User $user, string $plan): void { /* ... */ }
}Dead code
Variables, parameters, methods or classes that are never called or referenced. Dead code accumulates over time as features are removed or replaced but the old code is not cleaned up. It clutters the codebase, confuses new developers and adds surface area that has to be mentally filtered out when reading. If it is not used, delete it. Version control exists precisely so you can recover something you removed.
// Smelly
class OrderService
{
public function placeOrder(array $items): Order
{
$order = Order::create(['items' => $items]);
return $order;
}
// This was the old flow before the payment gateway switch in 2022.
// Keeping it just in case.
public function placeOrderLegacy(array $items, string $gateway): Order
{
$fee = $this->calculateLegacyFee($gateway);
$order = Order::create(['items' => $items, 'fee' => $fee]);
return $order;
}
private function calculateLegacyFee(string $gateway): float
{
return $gateway === 'ipay88' ? 1.5 : 2.0;
}
}
// Clean: remove it. Git blame exists for a reason.
class OrderService
{
public function placeOrder(array $items): Order
{
return Order::create(['items' => $items]);
}
}Long parameter list
A function that takes five, seven or ten parameters. At that point the caller has to remember the order of every argument and the function is almost certainly doing too many distinct things. The fix is to group related parameters into an object that carries them together, or to split the function into smaller ones that each take fewer arguments.
// Smelly
function createUser(
string $name,
string $email,
string $password,
string $phone,
string $address,
string $city,
string $country,
string $role
): User {
// ...
}
// Called like this. No one can read this without looking up the signature
createUser('Ahmad', 'ahmad@example.com', 'secret', '+60123456789', 'Jalan Utama 1', 'Kuala Lumpur', 'Malaysia', 'admin');
// Clean: group into a DTO
class CreateUserData
{
public function __construct(
public readonly string $name,
public readonly string $email,
public readonly string $password,
public readonly string $phone,
public readonly string $address,
public readonly string $city,
public readonly string $country,
public readonly string $role,
) {}
}
function createUser(CreateUserData $data): User
{
// ...
}Primitive obsession
Using raw strings, integers or booleans to represent domain concepts that deserve their own types. An email address stored as a plain string, a currency amount stored as a float, a status stored as an integer with magic values like 0, 1 and 2. These are technically fine but they push validation logic everywhere the value is used and make the code’s intent harder to read. Creating a dedicated type for the concept centralises both the representation and the rules around it.
// Smelly: what does status 2 mean? where is this documented?
function updateOrder(int $orderId, int $status): void
{
if (!in_array($status, [0, 1, 2, 3])) {
throw new \Exception('Invalid status');
}
Order::find($orderId)->update(['status' => $status]);
}
updateOrder(99, 2); // what is 2?
// Clean: use an enum
enum OrderStatus: string
{
case Pending = 'pending';
case Confirmed = 'confirmed';
case Shipped = 'shipped';
case Cancelled = 'cancelled';
}
function updateOrder(int $orderId, OrderStatus $status): void
{
Order::find($orderId)->update(['status' => $status->value]);
}
updateOrder(99, OrderStatus::Shipped); // readable at the call siteData clumps
Groups of variables that always travel together but are never formalised into a single structure. If you see the same three or four parameters appearing together in multiple function signatures, that grouping is trying to tell you it should be an object.
// Smelly: street, city, postcode always appear together
function createShipment(string $street, string $city, string $postcode): void { /* ... */ }
function validateAddress(string $street, string $city, string $postcode): bool { /* ... */ }
function calculateDeliveryFee(string $street, string $city, string $postcode): float { /* ... */ }
// Clean: the group is telling you it wants to be a class
class Address
{
public function __construct(
public readonly string $street,
public readonly string $city,
public readonly string $postcode,
) {}
}
function createShipment(Address $address): void { /* ... */ }
function validateAddress(Address $address): bool { /* ... */ }
function calculateDeliveryFee(Address $address): float { /* ... */ }Comments that explain what the code does
A comment that says // loop through users and check their status above a loop that loops through users and checks their status is not documentation. It is a sign that the code itself is not clear enough to be read without translation. The better fix is code that expresses its intent directly through naming. Comments that explain why a decision was made are valuable. Comments that narrate what the code does are a smell.
// Smelly: the comment is just repeating the code in English
public function handle(array $users): void
{
// loop through users and check if active
foreach ($users as $user) {
// check if user is active
if ($user['status'] === 'active') {
// send email to user
Mail::to($user['email'])->send(new WelcomeMail());
}
}
}
// Clean: name the method and the condition so the code reads itself
public function notifyActiveUsers(array $users): void
{
foreach ($users as $user) {
if ($this->isActive($user)) {
$this->sendWelcomeEmail($user);
}
}
}
// This is what a useful comment looks like: it explains WHY not WHAT
// FPX requires amount in cents, not ringgit. Do not change this without
// confirming with the payment team first
$amount = (int) round($order->total * 100);Feature envy
A method that spends more time working with the data of another class than with the data of its own class. If a method is constantly calling getters on some other object to do its work, that logic probably belongs in the other object.
// Smelly: Invoice is obsessed with Order's internals
class Invoice
{
public function calculateTotal(Order $order): float
{
$subtotal = 0;
foreach ($order->getItems() as $item) {
$subtotal += $item->getPrice() * $item->getQuantity();
}
if ($order->getCoupon()) {
$subtotal -= $subtotal * $order->getCoupon()->getDiscountRate();
}
return $subtotal + $order->getShippingFee();
}
}
// Clean: the logic belongs on Order, Invoice just asks for the result
class Order
{
public function calculateTotal(): float
{
$subtotal = collect($this->items)
->sum(fn($item) => $item->price * $item->quantity);
if ($this->coupon) {
$subtotal -= $subtotal * $this->coupon->discount_rate;
}
return $subtotal + $this->shipping_fee;
}
}
class Invoice
{
public function calculateTotal(Order $order): float
{
return $order->calculateTotal();
}
}Shotgun surgery
The opposite of the god class problem. You need to make one logical change and it requires edits scattered across a dozen different files. When a single change forces you to touch many classes, the codebase’s responsibilities are fragmented in a way that makes changes expensive and risky.
// Smelly: discount logic is scattered everywhere
// In CartController.php
$discount = $user->is_member ? $subtotal * 0.1 : 0;
// In CheckoutService.php
$discount = $order->user->is_member ? $order->subtotal * 0.1 : 0;
// In InvoiceGenerator.php
$discount = $invoice->user->is_member ? $invoice->amount * 0.1 : 0;
// In OrderExport.php
$discount = $row->user->is_member ? $row->total * 0.1 : 0;
// Clean: one place owns the rule
class DiscountCalculator
{
public function forUser(User $user, float $amount): float
{
return $user->is_member ? $amount * 0.1 : 0;
}
}
// Every class now calls DiscountCalculator instead of repeating the logic.
// When the rule changes, you change it in exactly one place.Code smell is not the same as technical debt
These two terms get conflated. Technical debt is the broader category: the accumulated cost of shortcuts, deferred work and design decisions that made sense at the time but now slow you down. Code smell is a specific type of signal within that broader category.
Not all technical debt produces visible smells. A database schema that was designed for a product that no longer exists may not show up in any individual function but still costs you every time you build a new feature on top of it.
Not all code smells represent significant debt. A slightly long method in rarely touched code may not be worth the risk of refactoring. The cost-benefit calculation always depends on how often the code is changed, how critical the path is and how much the smell is actively slowing the team down.
How to find them
Code review is the most practical detection point. When a reviewer finds themselves rereading a block of code multiple times to understand what it does, something smells. When they notice a function appearing to do three distinct things, something smells. Naming that observation explicitly during code review builds shared vocabulary in the team.
Static analysis tools automate a portion of this. They do not replace a human reviewer but they catch the mechanical patterns consistently, which frees reviewers to focus on the harder judgements that require context and experience.
Tools that help you find them
Think of these tools like a spell checker for code quality. A spell checker does not write your article for you. It catches the obvious errors so your editor can focus on whether the argument actually makes sense. These tools do the same: they catch the patterns a machine can see so your reviewer can focus on the patterns only a human can.
For PHP developers
PHPStan is a static analysis tool that reads your PHP code without running it and tells you about problems it finds. The higher the rule level (0 to 9), the stricter it gets. Most teams start at level 5 and work up over time. It catches things like calling a method that does not exist, passing the wrong type and functions that are too complex to follow. Free and open source.
Psalm does similar work to PHPStan but with a stronger focus on type safety. If your codebase uses a lot of PHP 8 type declarations and you want something to enforce them rigorously, Psalm handles that well. Also free and open source.
PHP Mess Detector (PHPMD) is specifically built for code smells. It flags long methods, excessive class complexity, unused parameters and similar structural problems. You can configure which rules to apply and how strict the thresholds should be. Good starting point if you want a focused smell detector rather than a general type checker.
Larastan is PHPStan configured specifically for Laravel. It understands Eloquent models, facades, relationships and the rest of the framework so it gives you useful analysis instead of false alarms about Laravel magic. If you are on a Laravel codebase, use this instead of plain PHPStan.
For JavaScript and TypeScript developers
ESLint is the standard linting tool for JavaScript and TypeScript. Beyond catching syntax errors it can be configured with rules that flag structural smells: functions that are too long, too deeply nested, or doing too many things at once. Free, open source and already installed in most projects.
For teams and CI pipelines
SonarQube (self-hosted) and SonarCloud (cloud version) scan your entire codebase and produce a dashboard showing code smells, bugs, security issues and test coverage in one place. It integrates with GitHub, GitLab and Bitbucket so it can comment on pull requests automatically. The community edition is free. The paid tiers add branch analysis and security rules.
The practical setup for most teams is SonarQube or SonarCloud running on every pull request, blocking merges that introduce new smells above a configurable threshold, while PHPStan or Psalm runs locally before the developer even pushes.
What to do when you find one
Not every smell demands immediate action. The question is whether fixing it now costs less than living with it will.
If the code is on a hot path that gets touched every sprint, the smell is actively slowing the team and the fix should go on the backlog with real priority. If the code is stable, rarely changed and well-tested, the smell may be worth noting and leaving alone.
When you do refactor, do it with test coverage in place. Refactoring is restructuring code without changing its observable behaviour. Tests are how you verify the behaviour was preserved. Refactoring without tests is just editing with optimism.
The skill underneath the term
Recognising code smell is a learned perceptual skill. It develops the same way any technical taste does: by reading a lot of code, writing a lot of code and getting feedback from people who have done both longer than you have.
Junior developers often cannot see smells in code they wrote because they are still focused on making the code work at all. Senior developers see them immediately because they have learned to read code for its future maintenance cost, not just its current correctness.
The goal is not a perfectly clean codebase, which does not exist in any production system that has been around for more than a year. The goal is a team with enough shared vocabulary to name what they see, enough discipline to keep the worst smells from accumulating and enough judgment to know when to fix something and when to move on.
That starts with knowing what the term means.
