ch17-smells-and-heuristics

Chapter 17: Smells and Heuristics

clean-code code-smells refactoring

Status: Notes complete
Difficulty: Medium
Time to complete: ~60 min read (reference chapter — use during code review)

---

Overview

Chapter 17 is the index at the back of a dictionary: you do not read it cover-to-cover on day one, but you reach for it constantly once you know it exists. Martin compiles every smell and heuristic from the book — and several new ones — into a single, numbered, categorized reference. There are 61 items across 7 categories: Comments (C), Environment (E), Functions (F), General (G), Java (J), Names (N), and Tests (T).

The chapter originated from a refactoring exercise (the TestableHtmlBuilder case study covered in Chapters 14–16). Every time Martin or a colleague identified a reason to change something, they catalogued it. Over many years of practice and teaching, the list stabilized into the 61 items here.

Why this matters: Every item is a named, shared vocabulary entry. When a reviewer says “G5” or “Feature Envy”, the entire team knows exactly what is meant without a paragraph of explanation. Naming smells turns vague discomfort into actionable diagnosis.

---

How to Use This Chapter

• During code review: Scan the Quick Reference Table. If you spot something, cite the code (e.g., “G5 — this logic is duplicated in OrderService”).
• During refactoring: Use it as a checklist. Walk a file from top to bottom looking for each category in turn.
• During design: G-smells in particular signal structural problems that are cheaper to fix before writing tests.
• During retrospectives: If a bug escaped to production, ask which smell enabled it. This converts post-mortems into learning.

The smells are not rules. They are heuristics — signals that something may be wrong. Judgment is always required. A flag argument (F3) in a private helper function called from one place is usually fine. A flag argument on a public API surface is almost always wrong.

---

Category C: Comments (5 smells)

C1 — Inappropriate Information

Comments should contain only information that a human reader needs and that cannot live anywhere else. Change logs, author names, ticket numbers, and edit histories belong in source control, not in comments. Comments stuffed with metadata age badly, clutter every file, and are almost never read.

// BAD
// Author: jsmith
// Modified: 2021-03-15 (JIRA-4421)
// Modified: 2022-07-02 (JIRA-8813)
// See also: https://wiki.corp.example.com/blah
public void processOrder(Order order) { ... }
 
// GOOD — the method stands alone; history lives in git log
public void processOrder(Order order) { ... }

C2 — Obsolete Comment

A comment that no longer describes the code it annotates is worse than no comment. It misleads the reader into believing something false. When you change code, update or delete its associated comments. If the comment can’t be kept accurate, delete it.

# BAD
# Returns the discount as a percentage (0-100)
def get_discount(customer):
    return customer.discount_factor  # now returns a decimal 0.0-1.0
 
# GOOD
# Returns the discount factor as a decimal (0.0–1.0)
def get_discount(customer):
    return customer.discount_factor

C3 — Redundant Comment

A comment that says exactly what the code already says adds noise and trains readers to ignore all comments. The comment i++; // increment i is the canonical example — it cannot possibly add meaning. Redundant comments increase maintenance burden: now both the code and the comment must be updated when things change.

// BAD
/** The name of the customer. */
private String customerName;
 
/** Sets the name. @param name the new name */
public void setName(String name) { this.name = name; }
 
// GOOD — the names are self-explanatory; no comment needed
private String customerName;
public void setName(String name) { this.name = name; }

C4 — Poorly Written Comment

If a comment is worth writing, it is worth writing well. Use correct grammar. Be concise. Do not ramble. Do not state the obvious. A comment that requires re-reading to understand is a comment that should be rewritten or deleted.

// BAD
// this function basically does the thing where it figures out if the
// user should get a discount or not based on stuff
 
// GOOD
// Returns true if this customer qualifies for the seasonal discount.
// Eligibility requires: active account, >= 3 prior purchases, and
// no outstanding balance.
bool isEligibleForSeasonalDiscount(const Customer& c);

C5 — Commented-Out Code

Commented-out code festers. Other developers see it and hesitate to delete it — “maybe it’s there for a reason.” It accumulates. It misleads. It references APIs and variables that no longer exist. Source control remembers every line ever deleted; you never need to comment out code as a backup. Delete it.

// BAD
public void sendNotification(User user) {
    // emailService.sendLegacyEmail(user.getEmail());
    // if (user.hasSmsPreference()) { smsService.send(user.getPhone()); }
    notificationRouter.dispatch(user);
}
 
// GOOD
public void sendNotification(User user) {
    notificationRouter.dispatch(user);
}

---

Category E: Environment (4 smells)

E1 — Build Requires More Than One Step

Building a project should be a single command. Nobody should need to check out 3 repositories, set 5 environment variables, and run 4 commands in a specific order to produce a runnable artifact. One command: mvn install, gradle build, make, npm run build. Every extra step is a friction point that slows onboarding and creates room for error.

# BAD — documented in a 12-step wiki page
$ svn co http://repo/project/trunk
$ cd trunk/lib && ant build
$ cd ../core && mvn compile
$ cp lib/foo.jar core/lib/
$ mvn package
 
# GOOD
$ git clone https://github.com/org/project
$ ./gradlew build

E2 — Tests Require More Than One Step

You should be able to run every test in a system with a single command, or a single button click in an IDE. If running tests requires manual setup, starting external services, or running multiple commands in sequence, tests will be run less often — and the feedback loop degrades.

# BAD
$ docker-compose up -d
$ export TEST_DB_URL=postgres://localhost/testdb
$ mvn test -Pintegration
$ docker-compose down
 
# GOOD
$ mvn test   # embedded DB spins up automatically; everything is self-contained

E3 — Build Is Fragile

A build that works on one developer’s machine but fails in CI, or that depends on system-level state (specific JDK version in PATH, a globally installed tool, a particular timezone) is a fragile build. Fragile builds erode trust and slow teams. Builds should be reproducible and hermetic — the same source at the same commit should produce the same artifact everywhere.

# BAD — depends on a specific tool version present globally
$ eslint --fix src/   # breaks if eslint version differs between machines
 
# GOOD — version pinned in package.json, run via npm script
$ npm run lint        # always uses the project-local eslint version

E4 — Dependency on External Resources

Tests that call real networks, real databases, or real third-party APIs are not repeatable. They fail when the network is slow, the external service is down, or the test data changes. Mock or stub all external dependencies. Tests must be hermetic — they should run identically with no internet access.

# BAD
def test_get_user():
    response = requests.get("https://api.example.com/users/1")
    assert response.json()["name"] == "Alice"
 
# GOOD
def test_get_user(requests_mock):
    requests_mock.get("https://api.example.com/users/1",
                      json={"name": "Alice"})
    user = UserClient().get(1)
    assert user.name == "Alice"

---

Category F: Functions (4 smells)

F1 — Too Many Arguments

The ideal number of function arguments is zero (niladic). One (monadic) is good. Two (dyadic) is acceptable. Three (triadic) is to be avoided wherever possible. Four or more requires special justification — and almost never has it. Many arguments indicate that the function is doing too much, or that arguments should be collected into a parameter object.

// BAD — four arguments; callers must check documentation for order
public Circle makeCircle(double x, double y, double radius, String color) { ... }
 
// GOOD — arguments grouped into a cohesive object
public Circle makeCircle(Point center, double radius, Color color) { ... }

F2 — Output Arguments

Arguments are inputs. Using an argument as an output (passing in an object and mutating it instead of returning a new value) is surprising and counterintuitive. Readers must stop, check the method signature, and track the mutation mentally. Use return values for outputs. If the function must mutate state, make it a method on the object being mutated.

// BAD — appendFooter mutates the input; caller must know this
void appendFooter(StringBuffer report);
 
// GOOD — mutation is a method on the object, or a new object is returned
report.appendFooter();

F3 — Flag Arguments

A boolean argument is a declaration that the function does two things — one for true and one for false. This is a direct violation of Single Responsibility. If a function takes a flag, split it into two well-named functions.

// BAD — what does false mean? caller must check implementation
render(true);
 
// GOOD — intent is unambiguous at the call site
renderForSuite();
renderForSingleTest();

F4 — Dead Function

Methods that are never called are dead code. They accumulate, clutter the namespace, and mislead developers into thinking they might be called somewhere. Source control remembers deleted code. Delete dead functions without hesitation.

# BAD — _compute_legacy_hash has not been called since 2019
class PaymentProcessor:
    def process(self, payment): ...
    def _compute_legacy_hash(self, data): ...  # never called
 
# GOOD
class PaymentProcessor:
    def process(self, payment): ...

---

Category G: General (36 smells)

G1 — Multiple Languages in One Source File

A Java source file containing inline XML strings, embedded SQL, or raw HTML fragments is mixing concerns and languages in a way that makes each harder to validate, test, and maintain. Separate concerns into separate files, templates, or layers. Let each tool do its job.

// BAD — HTML embedded in Java business logic
String html = "<table><tr><td>" + user.getName() + "</td></tr></table>";
 
// GOOD — HTML in a template file; Java passes the model
String html = templateEngine.render("user-table.html", user);

G2 — Obvious Behavior Is Unimplemented

The Principle of Least Surprise: a function or class should behave exactly as its name implies. If Day.fromString("Monday") throws an exception instead of returning Day.MONDAY, users will be surprised and the surprise will cost them time. Implement obvious behaviors fully.

// BAD — obvious behavior throws instead of working
Day day = Day.fromString("Monday");  // throws IllegalArgumentException
 
// GOOD
Day day = Day.fromString("Monday");  // returns Day.MONDAY, case-insensitive

G3 — Incorrect Behavior at the Boundaries

Developers often test the “happy path” and assume boundary conditions behave correctly. They almost never do. Every algorithm has edge cases: empty collections, zero, negative numbers, maximum values, null inputs. Test every boundary explicitly — never rely on intuition.

// BAD — untested boundary; fails when list is empty
public int first(List<Integer> list) {
    return list.get(0);  // IndexOutOfBoundsException if empty
}
 
// GOOD
public Optional<Integer> first(List<Integer> list) {
    return list.isEmpty() ? Optional.empty() : Optional.of(list.get(0));
}

G4 — Overridden Safeties

Overriding safety mechanisms — commenting out a failing test, disabling a compiler warning with @SuppressWarnings, setting failOnError=false in a build to make it pass — is trading short-term relief for long-term danger. Failing tests exist for a reason. Compiler warnings exist for a reason. Fix the underlying problem; never silence the signal.

// BAD — test disabled because it "takes too long" to fix
@Ignore("Flaky — fix later")
@Test
public void testConcurrentUpdates() { ... }
 
// GOOD — understand why it's flaky and fix the root cause
@Test
public void testConcurrentUpdates() { ... }

G5 — Duplication

DRY — Don’t Repeat Yourself. Every time you see duplicated code, you are looking at a missed abstraction. Duplication means that a future change must be made in N places, and the developer making the change will inevitably miss one. Some forms of duplication are subtle: duplicate switch/case statements testing the same set of types are a sign that polymorphism should replace the conditionals.

# BAD — same validation in two places; one will drift
def create_user(email, password):
    if not re.match(r"[^@]+@[^@]+\.[^@]+", email):
        raise ValueError("Invalid email")
    ...
 
def update_user(email, password):
    if not re.match(r"[^@]+@[^@]+\.[^@]+", email):
        raise ValueError("Invalid email")
    ...
 
# GOOD
def _validate_email(email):
    if not re.match(r"[^@]+@[^@]+\.[^@]+", email):
        raise ValueError("Invalid email")
 
def create_user(email, password):
    _validate_email(email); ...
 
def update_user(email, password):
    _validate_email(email); ...

G6 — Code at Wrong Level of Abstraction

High-level modules should contain high-level policy. Low-level modules contain low-level detail. When a high-level class reaches into implementation details — or when a low-level utility handles orchestration logic — the levels of abstraction are mixed and neither layer is easy to understand or test independently.

// BAD — Stack base class knows about "stack overflow" — an implementation detail
public abstract class Stack {
    public void push(Object o) {
        if (stackPointer >= MAX_STACK_SIZE)   // implementation detail in abstraction
            throw new StackOverflowError();
        ...
    }
}
 
// GOOD — keep implementation details in the concrete subclass
public abstract class Stack {
    public abstract void push(Object o);
}

G7 — Base Classes Depending on Their Derivatives

A base class should know nothing about its subclasses. If Animal imports or references Dog or Cat, the dependency relationship has inverted: the abstraction depends on the concretion. This makes it impossible to deploy the base class independently and couples the hierarchy in the wrong direction.

// BAD — base class checks subclass type
public abstract class Shape {
    public void render() {
        if (this instanceof Circle) { ... }  // base knows about Circle
        else if (this instanceof Square) { ... }
    }
}
 
// GOOD — polymorphism; base class knows nothing about subclasses
public abstract class Shape {
    public abstract void render();
}

G8 — Too Much Information

A well-designed module has a small, focused interface. Wide interfaces — classes with dozens of public methods, functions with long parameter lists, modules that expose their internals — are hard to understand, hard to mock, and hard to change. Limit what you expose. Hide data. Hide utility functions. Hide implementation details.

// BAD — exposes too much; callers can reach internal state
public class DataStore {
    public Connection getConnection() { ... }
    public Statement prepareStatement(String sql) { ... }
    public ResultSet executeQuery(Statement s) { ... }
    public void beginTransaction() { ... }
    // ... 20 more methods
}
 
// GOOD — exposes only the abstraction callers need
public class DataStore {
    public List<User> findUsersByEmail(String email) { ... }
    public void save(User user) { ... }
}

G9 — Dead Code

Code that is never executed — unreachable branches after a return, catch blocks for exceptions that can never be thrown, utility functions that no one calls — is dead code. It rots in place: APIs it depends on change, but nobody notices because nobody runs it. Delete dead code without hesitation; source control remembers it.

// BAD — the catch block can never execute; the try never throws this exception
try {
    int result = compute();
} catch (FileNotFoundException e) {  // impossible; no file I/O here
    logger.error("File not found", e);
}
 
// GOOD — remove the unreachable catch
int result = compute();

G10 — Vertical Separation

Variables and functions should be defined close to where they are used. Local variables should be declared just above their first use, not at the top of a 60-line function. Private helper functions should appear just below the function that calls them — not 200 lines away at the bottom of the file. Long vertical distances force the reader to scroll and hold context in working memory.

// BAD — variable declared 40 lines before first use
String errorMessage;
// ... 40 lines of unrelated code ...
if (failed) {
    errorMessage = "Something went wrong";
    log(errorMessage);
}
 
// GOOD — declared just before use
// ... 40 lines ...
if (failed) {
    String errorMessage = "Something went wrong";
    log(errorMessage);
}

G11 — Inconsistency

If you use a particular pattern, name, or convention in one place, use it consistently everywhere similar. If you name a variable controller in one class, don’t name the equivalent variable handler in another. Inconsistency forces readers to interpret meaning on every read. Consistency makes code predictable and navigation fast.

// BAD — same concept, three different names across the codebase
class OrderService    { private Controller controller; }
class PaymentService  { private Handler handler; }
class ShippingService { private Processor processor; }
 
// GOOD — one name, consistently applied
class OrderService    { private Controller controller; }
class PaymentService  { private Controller controller; }
class ShippingService { private Controller controller; }

G12 — Clutter

Code that serves no purpose is clutter: default constructors with no body, variables that are never assigned, getters/setters for fields that no caller uses, utility classes with one method that is only called in one place. Clutter accumulates incrementally. Clean it out with the same discipline you’d use in a physical workspace.

// BAD — default constructor added "just in case"; useless field; unused getter
public class Config {
    private int unusedField;
    public Config() {}  // compiler already generates this
    public int getUnusedField() { return unusedField; }
    public String getTimeout() { return timeout; }
}
 
// GOOD — only what is necessary
public class Config {
    public String getTimeout() { return timeout; }
}

G13 — Artificial Coupling

Things that don’t depend on each other should not be coupled together. The most common form: defining an enum or a constant inside a class that has nothing to do with it, because it was convenient at the time. Placing HttpStatus constants inside UserController forces every consumer of those constants to depend on UserController.

// BAD — HttpStatus enum defined inside UserController; all callers must import it
public class UserController {
    public enum HttpStatus { OK, NOT_FOUND, BAD_REQUEST }
    ...
}
 
// GOOD — HttpStatus lives in its own class; no artificial coupling
public enum HttpStatus { OK, NOT_FOUND, BAD_REQUEST }

G14 — Feature Envy

A method has feature envy when it uses the data or methods of another class more than its own. The method is envious — it wants to live in the other class. The fix is to move the method to the class whose data it manipulates.

// BAD — OrderPrinter knows more about Order's internals than Order does
public class OrderPrinter {
    public String format(Order order) {
        return order.getCustomerName() + ": " +
               order.getItems().size() + " items @ " +
               order.getTotalPrice();
    }
}
 
// GOOD — the method belongs on Order
public class Order {
    public String format() {
        return customerName + ": " + items.size() + " items @ " + totalPrice;
    }
}

G15 — Selector Arguments

A boolean or enum argument used to select behavior inside the function is a selector argument — it forces the reader of the call site to know what true or false means without looking at the implementation. Split into separate, clearly named functions.

// BAD — what does false mean at the call site?
paint(BLUE, false);
 
// GOOD — the intent is immediately clear
paintBackground(BLUE);
paintForeground(BLUE);

G16 — Obscured Intent

Code is written once and read many times. Obscured intent — abbreviated variable names, dense one-liners, bit-twiddling tricks, cryptic formulas — optimizes for the writer and penalizes every future reader. Name things for clarity. Write one operation per line. Save the tricks for proven hot paths.

// BAD — unreadable; intent is completely obscured
int q = (m_ldayOfYear - 1) / 30 + (m_ldayOfYear > 180 ? 1 : 0);
 
// GOOD — each step named and readable
int approximateMonth = (dayOfYear - 1) / 30;
bool pastMidYear = dayOfYear > 180;
int adjustedMonth = approximateMonth + (pastMidYear ? 1 : 0);

G17 — Misplaced Responsibility

Code should live where a reader would expect to find it. Math.PI belongs in Math because that is where someone would look. If it were in Circle, users of PI who are not writing circle code would not find it. When placing code, ask: “Where would a reader go to find this?” — and put it there.

// BAD — pi defined in Circle; not findable for non-circle uses
public class Circle {
    public static final double PI = 3.14159265358979;
}
 
// GOOD — universal constants belong in universal places
public class Math {
    public static final double PI = Math.PI;  // use java.lang.Math
}

G18 — Inappropriate Static

Static methods are fine for functions with no need of polymorphism (pure utility functions). But when a method could benefit from polymorphism — when future code might want to override it — making it static locks out that flexibility. Prefer instance methods. Make methods static only when you are certain no override could ever be useful.

// BAD — static; no way to swap in a test double or extend behavior
public class HourlyPayCalculator {
    public static double calculatePay(Employee e, double overtimeRate) { ... }
}
 
// GOOD — instance method; can be subclassed or mocked
public class HourlyPayCalculator {
    public double calculatePay(Employee e, double overtimeRate) { ... }
}

G19 — Use Explanatory Variables

Complex expressions — long boolean conditions, chained method calls, intricate arithmetic — should be broken into explanatory intermediate variables with good names. The variable does not add logic; it adds meaning. This is one of the fastest ways to improve readability with zero behavioral change.

# BAD — the condition is a puzzle
if re.match(r"[^@]+@[^@]+\.[^@]+", email) and len(password) >= 8 and not account.is_suspended:
    ...
 
# GOOD — each component named for what it represents
is_valid_email = bool(re.match(r"[^@]+@[^@]+\.[^@]+", email))
is_strong_password = len(password) >= 8
is_active_account = not account.is_suspended
 
if is_valid_email and is_strong_password and is_active_account:
    ...

G20 — Function Names Should Say What They Do

If you have to read the body of a function to understand what it does, the name is wrong. Names should be explicit enough that the call site reads like documentation. If you can’t find a name that captures the behavior, that’s a signal the function does too many things and should be split.

// BAD — "process" says nothing; must read the body
order.process();
 
// GOOD — unambiguous at the call site
order.calculateAndApplyDiscount();
// or split further:
order.calculateDiscount();
order.applyDiscount();

G21 — Understand the Algorithm

Hacking a function until its tests pass is not the same as understanding it. If you cannot explain to a colleague exactly what the algorithm does and why every line is necessary, you don’t understand it. Refactor until the code is clear enough to be self-explanatory — this process forces understanding and often reveals bugs.

// BAD — passes tests but developer cannot explain why this works
public boolean isLeapYear(int year) {
    return ((year % 4 == 0) && (year % 100 != 0)) || (year % 400 == 0);
}
// ^^ actually this one is correct and explainable — but typical "hack until green" code is not
 
// GOOD — make the logic structure mirror the explanation you would give out loud
public boolean isLeapYear(int year) {
    boolean divisibleBy4   = (year % 4   == 0);
    boolean divisibleBy100 = (year % 100 == 0);
    boolean divisibleBy400 = (year % 400 == 0);
    return divisibleBy400 || (divisibleBy4 && !divisibleBy100);
}

G22 — Make Logical Dependencies Physical

If one module depends on another — if ReportGenerator assumes that PageCounter has already been called — that dependency is logical but not physical. Logical dependencies are invisible to the compiler and to future maintainers. Make them physical: pass a PageCount object as a parameter, or have PageCounter return a result that ReportGenerator accepts as input.

// BAD — ReportGenerator silently depends on PageCounter having been called
public class ReportGenerator {
    public void generateReport() {
        int pages = PageCounter.getCount();  // assumes someone called calculate() first
        ...
    }
}
 
// GOOD — dependency made explicit via parameter
public void generateReport(int pageCount) {
    ...
}

G23 — Prefer Polymorphism to If/Else or Switch/Case

The ONE SWITCH rule: there should be at most one switch statement per type. Each switch is a potential duplication point — if you add a new type, you must hunt down every switch and update it. Use polymorphism instead: define the behavior in the class, and let dispatch happen automatically.

// BAD — adding a new EmployeeType requires updating this switch AND any others
double pay = switch (employee.getType()) {
    case HOURLY    -> employee.getHours() * employee.getRate();
    case SALARIED  -> employee.getMonthlySalary() / 160;
    case CONTRACTOR -> employee.getInvoiceAmount();
};
 
// GOOD — each subclass knows how to calculate its own pay
double pay = employee.calculatePay();

G24 — Follow Standard Conventions

Every team should follow a coding standard based on industry norms. This covers brace placement, indentation, variable naming, comment style — and the standard should be enforced by the IDE, not by code review comments. Nobody should have to discuss formatting in a PR. Automate it and move on.

// BAD — ad-hoc style; every developer has their own preferences
public class userAccount{
private String UserName;
public userAccount(String UserName){this.UserName=UserName;}
}
 
// GOOD — follows Java conventions; enforced by Checkstyle/Spotless
public class UserAccount {
    private String userName;
 
    public UserAccount(String userName) {
        this.userName = userName;
    }
}

G25 — Replace Magic Numbers with Named Constants

A bare number in source code — 86400, 7, 3.14159 — conveys nothing to the reader. Replace every magic number with a named constant whose name explains the number’s meaning. The one exception: 0 and 1 as loop indices or trivial checks are usually self-evident.

// BAD — what is 86400? what is 0.15?
double dailyRate = annualRate / 365;
double fee = amount * 0.15;
 
// GOOD — meaning is explicit
static final int DAYS_PER_YEAR = 365;
static final double TRANSACTION_FEE_RATE = 0.15;
 
double dailyRate = annualRate / DAYS_PER_YEAR;
double fee = amount * TRANSACTION_FEE_RATE;

G26 — Be Precise

Imprecision in code is a form of lying by omission. If a function can return null, say so — or throw. If a concurrency issue could arise, use a lock. If a floating-point comparison is approximate, document the epsilon. Vagueness at one level compounds into bugs at the next. Make every decision explicit and defensible.

// BAD — returns null on failure; callers often forget to check
public User findUser(String id) {
    return userMap.get(id);  // returns null if not found
}
 
// GOOD — contract is explicit; callers cannot ignore the absence case
public Optional<User> findUser(String id) {
    return Optional.ofNullable(userMap.get(id));
}

G27 — Structure over Convention

Conventions can be broken. Structure cannot (without a compiler error). When you have a choice between “we follow the convention to always implement this interface method” and “we use an abstract method that the compiler forces every subclass to implement” — choose the structure. It is automatically enforced.

// BAD — relies on convention; easy to forget to override
public class Report {
    public String getTitle() {
        return "Default Title";  // subclasses should override but won't be forced to
    }
}
 
// GOOD — abstract method; compiler forces implementation in every subclass
public abstract class Report {
    public abstract String getTitle();
}

G28 — Encapsulate Conditionals

Extracting a complex boolean expression into a well-named function makes the condition read like prose and hides the implementation details of the check. It also makes the condition reusable and independently testable.

// BAD — reader must parse the condition to understand intent
if (timer.hasExpired() && !timer.isRecurrent()) {
    deleteTimer(timer);
}
 
// GOOD — intent is immediate
if (shouldBeDeleted(timer)) {
    deleteTimer(timer);
}
 
private boolean shouldBeDeleted(Timer timer) {
    return timer.hasExpired() && !timer.isRecurrent();
}

G29 — Avoid Negative Conditionals

Negative conditions are harder to read than positive ones. if (!buffer.shouldNotCompact()) requires two mental negations. if (buffer.shouldCompact()) requires zero. Name things positively and check the positive form. Reserve negatives for cases where there is no natural positive framing.

// BAD — double negative; reader must think carefully
if (!buffer.shouldNotCompact()) { compact(buffer); }
 
// GOOD — positive form reads as plain English
if (buffer.shouldCompact()) { compact(buffer); }

G30 — Functions Should Do One Thing

A function that does several things at different levels of abstraction — validates input, persists to the database, and sends a notification — is a function that is hard to name, hard to test, and hard to change. Extract each distinct responsibility into its own function. If the function can be sectioned with comments, those sections should be separate functions.

// BAD — one function does three things
public void processUserRegistration(User user) {
    // Validate
    if (user.getEmail() == null) throw new IllegalArgumentException("...");
    // Persist
    userRepository.save(user);
    // Notify
    emailService.sendWelcomeEmail(user);
}
 
// GOOD — each step is its own focused function
public void processUserRegistration(User user) {
    validate(user);
    save(user);
    notifyUser(user);
}

G31 — Hidden Temporal Couplings

When functions must be called in a specific order, that order should be enforced by the code structure, not by convention or documentation. The classic technique: make each function return a result that is required as input by the next. This makes the coupling visible to the compiler.

// BAD — functions must be called in order, but nothing enforces it
void diveForMud();
void makeMudPies();
void cleanUp();
 
// GOOD — return values encode the required order
MudResult diveForMud();
MudPies makeMudPies(MudResult mud);
void cleanUp(MudPies pies);

G32 — Don’t Be Arbitrary

Structure in your code should exist for a reason. If a developer encounters an arbitrary-seeming structure — a class with no clear cohesion, variables ordered randomly, methods grouped with no logic — they will assume the structure does not matter and feel free to change it. Make every structural decision intentional and defensible.

// BAD — methods in random order; structure appears arbitrary
public class AccountService {
    public void sendStatement() { ... }
    public void openAccount() { ... }
    public void closeAccount() { ... }
    public void generateReport() { ... }
}
 
// GOOD — related operations grouped together; a reader can see the logic
public class AccountService {
    public void openAccount() { ... }
    public void closeAccount() { ... }
    public void sendStatement() { ... }
    public void generateReport() { ... }
}

G33 — Encapsulate Boundary Conditions

Boundary calculations like level + 1 or level - 1 appear repeatedly in code that traverses tree levels or array indices. Name these values. A single named variable (nextLevel = level + 1) documents the intent, centralizes the calculation, and prevents inconsistency if the boundary logic ever needs to change.

// BAD — level + 1 scattered throughout; one off-by-one in any instance causes bugs
if (node.children.size() > level + 1) { parse(node, level + 1); }
render(node, level + 1);
 
// GOOD — boundary named and calculated once
int nextLevel = level + 1;
if (node.children.size() > nextLevel) { parse(node, nextLevel); }
render(node, nextLevel);

G34 — Functions Should Descend Only One Level of Abstraction

All statements in a function should be at the same level of abstraction, and that level should be exactly one level below the function’s name. Mixing high-level orchestration with low-level implementation in the same function makes both harder to understand. Extract the low-level steps into helper functions.

// BAD — mixes high-level ("create the report") with low-level SQL
public String createReport() {
    StringBuilder sb = new StringBuilder();
    sb.append("<html>");
    ResultSet rs = conn.prepareStatement("SELECT...").executeQuery();
    while (rs.next()) { sb.append("<tr><td>" + rs.getString(1) + "</td></tr>"); }
    sb.append("</html>");
    return sb.toString();
}
 
// GOOD — each function is at one level of abstraction
public String createReport() {
    List<ReportRow> rows = fetchReportData();
    return renderAsHtml(rows);
}

G35 — Keep Configurable Data at High Levels

Constants and configuration values — timeouts, limits, default values, feature flags — should live at the highest level of the system, not buried deep in implementation code. High-level callers should pass configuration down to low-level implementations, not the other way around. This makes the system easy to reconfigure without changing internals.

// BAD — magic constant buried in a low-level utility
public class EmailSender {
    public void send(Email email) {
        int retryCount = 3;  // buried here; no easy way to change it per environment
        ...
    }
}
 
// GOOD — constant lives at the application configuration level
public class AppConfig {
    public static final int EMAIL_RETRY_COUNT = 3;
}
 
public class EmailSender {
    public void send(Email email, int retryCount) { ... }
}

G36 — Avoid Transitive Navigation

This is the Law of Demeter applied to module relationships. A module should only talk to its immediate collaborators — not to the collaborators’ collaborators. a.getB().getC().doSomething() is transitive navigation. It means the caller knows too much about the internal structure of A, B, and C. Any change to that chain requires updating the caller.

// BAD — caller navigates three levels deep; tightly coupled to the whole chain
user.getAddress().getCity().getName();
 
// GOOD — each object provides a direct accessor for what its callers need
user.getCityName();  // or: String cityName = address.getCityName();

---

Category J: Java (3 smells)

J1 — Avoid Long Import Lists by Using Wildcards

When you use many classes from a single package, import the package with a wildcard. A list of 10 individual imports from java.util is clutter; import java.util.* is concise. The counter-argument (wildcards can import ambiguous names) rarely applies in practice and can always be resolved with an explicit import for the ambiguous class alone.

// BAD
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
 
// GOOD
import java.util.*;

J2 — Don’t Inherit Constants

Implementing an interface just to get access to its constants is a misuse of inheritance. It pollutes the implementing class’s public interface and creates a spurious “is-a” relationship. Use static import instead.

// BAD — implements interface only to inherit constants
public interface HttpConstants {
    int STATUS_OK = 200;
    int STATUS_NOT_FOUND = 404;
}
 
public class ApiHandler implements HttpConstants {
    public void handle() {
        respond(STATUS_OK);  // STATUS_OK inherited from interface
    }
}
 
// GOOD — static import; no spurious inheritance
import static com.example.HttpConstants.STATUS_OK;
 
public class ApiHandler {
    public void handle() {
        respond(STATUS_OK);
    }
}

J3 — Constants versus Enums

Do not use public static final int as named constants when you could use an enum. Enums have type safety (the compiler rejects integers where an enum is expected), can carry methods and fields, support values() iteration, and work cleanly in switch statements. There is no advantage int constants have over enums in modern Java.

// BAD — int constants; type-unsafe; any int is accepted
public static final int DIRECTION_NORTH = 0;
public static final int DIRECTION_SOUTH = 1;
public void move(int direction) { ... }
move(42);  // compiles; obviously wrong
 
// GOOD — enum; type-safe; compiler rejects invalid values
public enum Direction { NORTH, SOUTH, EAST, WEST }
public void move(Direction direction) { ... }
move(Direction.NORTH);  // only valid Direction values accepted

---

Category N: Names (7 smells)

N1 — Choose Descriptive Names

Names clarify intent. A well-chosen name communicates what a variable holds, what a function does, or what a class represents — without requiring the reader to look further. Names should be chosen carefully, revised as understanding improves, and treated as a first-class part of the code.

# BAD — names communicate nothing
def d(a, b):
    return a * b / 100
 
# GOOD — intent is immediately clear
def calculate_discounted_price(original_price, discount_percent):
    return original_price * discount_percent / 100

N2 — Choose Names at the Appropriate Level of Abstraction

A name like dial(String phoneNumber) embeds an implementation detail (phone numbers, dialing) into a method that, at the abstraction level of Modem, should speak only about connectivity. Use connect(String connectionLocator) — the name works even if the modem later uses IP addresses instead of phone numbers.

// BAD — name leaks implementation detail
interface Modem {
    void dial(String phoneNumber);
}
 
// GOOD — name is at the right abstraction level
interface Modem {
    void connect(String connectionLocator);
}

N3 — Use Standard Nomenclature Where Possible

Industry patterns, idioms, and conventions carry meaning that is instantly recognized by experienced developers. UserFactory, OrderDecorator, PaymentAdapter — the Pattern name in the class name signals the structure and intent immediately. Use names that are already part of the shared vocabulary.

// BAD — custom naming for well-known patterns; colleagues must study the code to understand
public class UserMaker { ... }
public class OrderWrapper { ... }
 
// GOOD — standard pattern names; intent recognized immediately
public class UserFactory { ... }
public class OrderDecorator { ... }

N4 — Unambiguous Names

Names should be unambiguous — there should be only one reasonable interpretation. Avoid names that could mean several things (doRename, handleIt, process). Avoid noise words that add length without adding meaning (dataValue, theManager, infoObject).

// BAD — ambiguous; does "do" add any meaning?
public void doRename(String name) { ... }
public void handleIt() { ... }
 
// GOOD — unambiguous
public void rename(String name) { ... }
public void handlePaymentTimeout() { ... }

N5 — Use Long Names for Long Scopes

A loop variable i is perfectly fine for a 3-line loop. A variable i in a 60-line function with nested loops is a maintenance hazard. The longer a variable’s scope, the more descriptive its name must be. Short names are appropriate only where the scope is so short that context makes meaning obvious.

// BAD — short name in a long scope; meaning lost 50 lines away
int d;
// ... 50 lines ...
d = d + daysInCurrentMonth;
return d;
 
// GOOD — name is proportional to scope
int totalDaysElapsed = 0;
// ...
totalDaysElapsed += daysInCurrentMonth;
return totalDaysElapsed;

N6 — Avoid Encodings

Hungarian notation (m_, f_, strName, iCount) and other encoding schemes were invented to compensate for weak IDEs and compilers. Modern IDEs show type information on hover; type information is redundant in names. Encodings add noise, require memorization, and clutter refactoring.

// BAD — encoded prefixes; the IDE already knows the types
private String m_strCustomerName;
private int m_iOrderCount;
private List<Order> m_lstOrders;
 
// GOOD — clean names; no encoding
private String customerName;
private int orderCount;
private List<Order> orders;

N7 — Names Should Describe Side-Effects

If a function does more than its name implies — especially if it has side effects like creating state, writing to a file, or modifying global data — its name must reflect that. A get* method that creates an object if it doesn’t exist is not a getter; it is a factory. Name it accordingly.

// BAD — "get" implies a simple accessor; this method creates if absent
public ObjectOutputStream getOos() throws IOException {
    if (m_oos == null) {
        m_oos = new ObjectOutputStream(m_socket.getOutputStream());
    }
    return m_oos;
}
 
// GOOD — name declares the side-effect
public ObjectOutputStream createOrReturnOos() throws IOException {
    if (m_oos == null) {
        m_oos = new ObjectOutputStream(m_socket.getOutputStream());
    }
    return m_oos;
}

---

Category T: Tests (9 smells)

T1 — Insufficient Tests

A test suite with gaps is not a safety net — it is a safety net with holes. If something could break and there is no test for it, it will break eventually. Use coverage tools to find gaps. Test every condition, every branch, every edge case. The question to ask is not “have I tested enough?” but “is there anything that could break that has no test?”

// BAD — only happy path tested; no test for empty list, null, or max size
@Test
public void testAdd() {
    queue.add("item");
    assertEquals(1, queue.size());
}
 
// GOOD — all meaningful cases covered
@Test public void testAdd_normalCase() { ... }
@Test public void testAdd_whenFull_throwsException() { ... }
@Test public void testAdd_nullItem_throwsException() { ... }
@Test public void testAdd_emptyQueueAfterDrain() { ... }

T2 — Use a Coverage Tool

Coverage tools make invisible gaps visible. Without a coverage report, you cannot know which lines, branches, or paths have no tests. Aim for at minimum 80% line coverage, and treat any uncovered branch in critical business logic as a bug. Most IDEs integrate coverage reporting — there is no excuse not to use it.

# Run with coverage (Maven + JaCoCo)
mvn test jacoco:report
# Then open target/site/jacoco/index.html to see uncovered lines highlighted in red

T3 — Don’t Skip Trivial Tests

A test for a trivial case takes seconds to write and has nearly zero maintenance cost. But it has real value: it documents that the case was considered and verified, it prevents trivial regressions, and it serves as executable documentation. Skipping trivial tests because “it’s obviously right” is how obvious bugs escape.

// BAD — trivial case skipped; "obviously" correct
// (no test for the 0-item edge case in a sum function)
 
// GOOD — trivial case tested; documents intent and prevents regression
@Test
public void testSum_emptyList_returnsZero() {
    assertEquals(0, calculator.sum(Collections.emptyList()));
}

T4 — An Ignored Test Is a Question

@Ignore or @Disabled on a test means one of two things: the feature is not yet implemented, or there is ambiguity about what the correct behavior should be. In either case, the @Ignore is a question mark, not an answer. Resolve the ambiguity — implement the feature, clarify the requirement, or delete the test.

// BAD — test ignored indefinitely; ambiguity never resolved
@Ignore("Not sure what the correct behavior should be here")
@Test
public void testRefundForPartialShipment() { ... }
 
// GOOD — open a ticket, define the behavior, remove the ignore
@Test
public void testRefundForPartialShipment() {
    // Partial refunds are pro-rated by shipped quantity (JIRA-4421)
    ...
}

T5 — Test Boundary Conditions

The happy path is the easiest part of any code to get right. The edges are where bugs live. For every function, explicitly test: empty input, single element, maximum size, minimum value, null, zero, negative numbers, off-by-one conditions, and any values right at a conditional boundary.

// BAD — only tests typical case
@Test
public void testMonthConverter() {
    assertEquals(3, converter.toNumber("March"));
}
 
// GOOD — tests the full boundary spectrum
@Test public void testFirstMonth()  { assertEquals(1,  converter.toNumber("January")); }
@Test public void testLastMonth()   { assertEquals(12, converter.toNumber("December")); }
@Test public void testInvalidMonth(){ assertThrows(IllegalArgumentException.class,
                                         () -> converter.toNumber("Octember")); }
@Test public void testNullMonth()   { assertThrows(NullPointerException.class,
                                         () -> converter.toNumber(null)); }

T6 — Exhaustively Test Near Bugs

When you find a bug, do not just fix it and add one test. Bugs congregate. If there is one bug in a function, there are likely others nearby. Test the entire neighborhood — all edge cases, all paths, all boundary conditions in the affected area. The bug you found is a signal, not the only problem.

// BAD — fixed one bug; added exactly one test
@Test
public void testBugFix_4421() {
    // The specific case that was reported
    assertEquals(expected, compute(specificInput));
}
 
// GOOD — fixed the bug; tested the whole neighborhood
@Test public void testCompute_zero()     { ... }
@Test public void testCompute_negative() { ... }
@Test public void testCompute_maxInt()   { ... }
@Test public void testCompute_overflow() { ... }

T7 — Patterns of Failure Are Revealing

When multiple tests fail together, the pattern of failures tells you where the problem is. If all tests involving null inputs fail, the problem is null handling. If all tests for dates in February fail, the problem is February-specific logic. Learn to read failure patterns as a diagnostic tool before reaching for the debugger.

# Revealing failure pattern:
FAIL: testUserLogin_withUppercaseEmail
FAIL: testUserLookup_byEmailIgnoreCase
FAIL: testEmailDuplicate_caseInsensitiveCheck
PASS: testUserLogin_withLowercaseEmail

# Pattern reveals: case normalization is broken, not login itself

T8 — Test Coverage Patterns Can Be Revealing

After a test failure, look at the coverage report to see which lines the failing test did and did not execute. The line where execution diverges from the expected path is a strong candidate for the bug location. Coverage-as-diagnostic is a different use of the coverage tool from coverage-as-metric.

// If the failing test never executes line 42:
42:    if (discount.isExpired()) { return originalPrice; }
 
// That branch may be the bug — the discount is never being checked for expiry
// Add a test that specifically exercises line 42

T9 — Tests Should Be Fast

A test suite that takes minutes to run is a test suite that developers will run infrequently — only before commits, not during development. The feedback loop degrades. Tests must be fast enough to run on every save. If a test is slow, isolate why: replace real I/O with mocks, replace real databases with in-memory alternatives, and run integration tests separately.

// BAD — slow test due to real network/DB access; blocks TDD loop
@Test
public void testSaveUser() {
    // Calls real PostgreSQL; takes 800ms
    userRepository.save(new User("Alice"));
}
 
// GOOD — in-memory or mocked; runs in < 5ms
@Test
public void testSaveUser() {
    UserRepository repo = new InMemoryUserRepository();
    repo.save(new User("Alice"));
    assertEquals(1, repo.count());
}

---

Quick Reference Table

Code	Category	Smell Name	Severity
C1	Comments	Inappropriate Information	Low
C2	Comments	Obsolete Comment	Medium
C3	Comments	Redundant Comment	Low
C4	Comments	Poorly Written Comment	Low
C5	Comments	Commented-Out Code	High
E1	Environment	Build Requires More Than One Step	High
E2	Environment	Tests Require More Than One Step	High
E3	Environment	Build Is Fragile	High
E4	Environment	Dependency on External Resources	High
F1	Functions	Too Many Arguments	Medium
F2	Functions	Output Arguments	Medium
F3	Functions	Flag Arguments	Medium
F4	Functions	Dead Function	Medium
G1	General	Multiple Languages in One Source File	Medium
G2	General	Obvious Behavior Is Unimplemented	High
G3	General	Incorrect Behavior at the Boundaries	High
G4	General	Overridden Safeties	Critical
G5	General	Duplication	High
G6	General	Code at Wrong Level of Abstraction	Medium
G7	General	Base Classes Depending on Their Derivatives	High
G8	General	Too Much Information	Medium
G9	General	Dead Code	Medium
G10	General	Vertical Separation	Low
G11	General	Inconsistency	Medium
G12	General	Clutter	Low
G13	General	Artificial Coupling	Medium
G14	General	Feature Envy	Medium
G15	General	Selector Arguments	Medium
G16	General	Obscured Intent	Medium
G17	General	Misplaced Responsibility	Medium
G18	General	Inappropriate Static	Medium
G19	General	Use Explanatory Variables	Low
G20	General	Function Names Should Say What They Do	Medium
G21	General	Understand the Algorithm	High
G22	General	Make Logical Dependencies Physical	High
G23	General	Prefer Polymorphism to If/Else or Switch/Case	Medium
G24	General	Follow Standard Conventions	Low
G25	General	Replace Magic Numbers with Named Constants	Medium
G26	General	Be Precise	High
G27	General	Structure over Convention	Medium
G28	General	Encapsulate Conditionals	Low
G29	General	Avoid Negative Conditionals	Low
G30	General	Functions Should Do One Thing	High
G31	General	Hidden Temporal Couplings	High
G32	General	Don’t Be Arbitrary	Low
G33	General	Encapsulate Boundary Conditions	Low
G34	General	Functions Should Descend Only One Level of Abstraction	Medium
G35	General	Keep Configurable Data at High Levels	Medium
G36	General	Avoid Transitive Navigation	High
J1	Java	Avoid Long Import Lists by Using Wildcards	Low
J2	Java	Don’t Inherit Constants	Medium
J3	Java	Constants versus Enums	Medium
N1	Names	Choose Descriptive Names	High
N2	Names	Choose Names at Appropriate Level of Abstraction	Medium
N3	Names	Use Standard Nomenclature Where Possible	Low
N4	Names	Unambiguous Names	Medium
N5	Names	Use Long Names for Long Scopes	Medium
N6	Names	Avoid Encodings	Low
N7	Names	Names Should Describe Side-Effects	High
T1	Tests	Insufficient Tests	Critical
T2	Tests	Use a Coverage Tool	High
T3	Tests	Don’t Skip Trivial Tests	Low
T4	Tests	An Ignored Test Is a Question	Medium
T5	Tests	Test Boundary Conditions	High
T6	Tests	Exhaustively Test Near Bugs	High
T7	Tests	Patterns of Failure Are Revealing	Medium
T8	Tests	Test Coverage Patterns Can Be Revealing	Medium
T9	Tests	Tests Should Be Fast	High

---

Most Common Smells (Top 10)

These are the smells most frequently encountered in production codebases — the ones worth memorizing first.

1. G5 — Duplication: The single most prevalent smell. Every duplication is a future inconsistency. Copy-paste is the fastest way to accumulate technical debt.
2. G25 — Magic Numbers: Found in virtually every codebase. 3600, 0.15, 7 — none of these mean anything without context. One-minute fix: extract a named constant.
3. C5 — Commented-Out Code: Ubiquitous. Nobody deletes it because they are afraid. Delete it — source control is your backup.
4. G30 — Functions Do Too Many Things: Functions that span 50+ lines almost always violate SRP. The section comments inside them are the tell.
5. F1 — Too Many Arguments: Functions with 5+ parameters are common in older codebases. They are hard to call correctly and hard to test.
6. G28 — Unencapsulated Conditionals: Long boolean expressions in if statements — common, easy to fix, immediately improves readability.
7. N1 — Non-Descriptive Names: x, temp, data, process — vague names that force readers to read the entire function body to understand what a variable holds.
8. T1 — Insufficient Tests: Most non-TDD codebases have significant test gaps. Coverage tools make these gaps visible.
9. G36 — Transitive Navigation: a.getB().getC().doSomething() — common in codebases without clear layer boundaries; creates fragile call chains.
10. G4 — Overridden Safeties: @Ignore-ed tests, suppressed warnings, build.gradle with failOnError = false — each one is a time bomb.

---

Key Takeaways

1. The 61 smells are a shared vocabulary — they turn vague unease (“this feels wrong”) into named, actionable diagnoses that teams can communicate efficiently.
2. Smells are heuristics, not rules — a flag argument in a private two-line helper is probably fine; a flag argument on a public API is almost never fine. Judgment always applies.
3. G5 (Duplication) is the deepest problem — almost every other smell is either a symptom of duplication or enables future duplication. DRY is not just about copy-paste.
4. The most dangerous smells are the safety-overriding ones — G4 (overriding safeties), T1 (no tests), T4 (ignored tests). These create an invisible, growing gap between what you think the code does and what it actually does.
5. Test smells (T1–T9) compound everything — a codebase with poor tests makes every other smell harder to fix, because refactoring without tests is guesswork.
6. Environment smells (E1–E4) are force multipliers — a slow or multi-step test run means every other improvement is hindered; nobody refactors when running tests is painful.
7. Names are the cheapest and highest-leverage improvements — a good rename is a zero-risk, zero-behavior-change improvement that permanently reduces the cognitive load on every future reader.
8. Structure (G27) beats convention — abstract methods, type systems, and compiler checks are more reliable than team norms. When in doubt, make the wrong thing impossible to compile.
9. “Boy Scout” the smells — you don’t have to fix everything at once. Fix one smell per file visit, and the codebase continuously improves without requiring dedicated refactoring sprints.
10. Chapter 17 is a reference, not a narrative — bookmark it. Return to it during code review, refactoring, and retrospectives.

---

Related Resources

• ch01-clean-code — What clean code means and why it matters
• ch02-meaningful-names — Full treatment of N-smells
• ch03-functions — Full treatment of F-smells
• ch04-comments — Full treatment of C-smells
• ch09-unit-tests — Full treatment of T-smells and FIRST principles
• ch14-successive-refinement — Seeing G-smells identified during a real refactoring
• ch16-refactoring-serialdate — G-smells (G3, G5, G9, G25) identified and fixed in a real class
• Refactoring: Improving the Design of Existing Code (Fowler, 2018) — the definitive companion catalog
• Working Effectively with Legacy Code (Feathers, 2004) — applying these heuristics to existing systems without tests

---

Last Updated: 2026-04-14