Code Smells & Refactoring Moves

A code smell is a surface indication of a deeper structural problem. Smells don't necessarily mean the code is wrong - they signal that refactoring might improve the design. Always use judgment: not every smell needs fixing.

Function-level smells

Long Function

• Symptom: Function exceeds 20 lines or requires comments to separate sections
• Refactoring: Extract Method - pull each section into a named function
• Signal phrases: "this function does X, then Y, then Z"

Too Many Arguments

• Symptom: Function takes 3+ arguments
• Refactoring:
  • 0 args (niladic) - ideal
  • 1 arg (monadic) - good
  • 2 args (dyadic) - acceptable
  • 3 args (triadic) - needs justification
  • 4+ args - introduce a parameter object or builder

# Bad: 5 arguments
def create_user(name, email, age, role, department): ...

# Good: parameter object
def create_user(user_data: CreateUserRequest): ...

Flag Arguments

• Symptom: Boolean parameter that makes the function do two different things
• Refactoring: Split into two functions with clear names

// Bad
render(data, true)   // what does true mean?

// Good
renderForPrint(data)
renderForScreen(data)

Side Effects

• Symptom: Function name suggests it does X but it also secretly does Y
• Refactoring: Either rename to reflect all effects or extract the side effect

Dead Code

• Symptom: Unreachable code, unused variables, commented-out blocks
• Refactoring: Delete it. Version control has the history if you need it back.

Class-level smells

Large Class (God Object)

• Symptom: Class has too many instance variables, too many methods, or does too many unrelated things
• Refactoring: Extract Class - group related fields and methods into new classes
• Test: Can you describe the class's purpose in one sentence without "and"?

Feature Envy

• Symptom: A method uses more data from another class than from its own
• Refactoring: Move Method - put the method in the class whose data it uses most

# Bad: OrderPrinter is envious of Order's data
class OrderPrinter:
    def format(self, order):
        return f"{order.customer.name}: {order.total()} ({len(order.items)} items)"

# Better: Move the formatting to Order
class Order:
    def summary(self):
        return f"{self.customer.name}: {self.total()} ({len(self.items)} items)"

Data Clumps

• Symptom: The same group of variables appears together in multiple places
• Refactoring: Extract a class or named tuple for the group

// Bad: start_x, start_y, end_x, end_y appear together everywhere
function drawLine(startX: number, startY: number, endX: number, endY: number) {}

// Good: introduce Point
function drawLine(start: Point, end: Point) {}

Primitive Obsession

• Symptom: Using primitive types (string, int) for domain concepts
• Refactoring: Replace with a value object

// Bad
String phoneNumber = "555-1234";
String zipCode = "90210";

// Good
PhoneNumber phone = new PhoneNumber("555-1234");  // validates format
ZipCode zip = new ZipCode("90210");               // validates format

Refused Bequest

• Symptom: Subclass inherits methods/properties it doesn't want or use
• Refactoring: Replace inheritance with composition, or push unused methods down to siblings that actually need them

Structural smells

Duplicated Code

• Symptom: Same or similar logic in two or more places
• Refactoring:
  • Same class: Extract Method
  • Sibling classes: Extract to parent or shared utility
  • Unrelated classes: Extract to a standalone function
• Warning: Don't extract prematurely. Two occurrences might be coincidental similarity. Three is a pattern.

Divergent Change

• Symptom: One class is modified for many different reasons
• Refactoring: Split the class by responsibility (apply SRP)

Shotgun Surgery

• Symptom: One change requires small edits across many classes
• Refactoring: Consolidate the scattered logic into one class

Speculative Generality

• Symptom: Abstract classes, interfaces, parameters, or hooks that exist "just in case" but have only one implementation
• Refactoring: Remove it. You Aren't Gonna Need It (YAGNI). Add abstraction when the second use case actually appears.

Inappropriate Intimacy

• Symptom: Two classes access each other's private details excessively
• Refactoring: Move methods and fields to reduce coupling, or introduce a mediator between them

Comment smells

Redundant Comment

// Bad: adds nothing the code doesn't already say
i++; // increment i

Mandated Comment

// Bad: forced Javadoc that just restates the signature
/** Sets the name. @param name the name */
public void setName(String name) { this.name = name; }

Commented-Out Code

# Bad: dead code hiding in comments
# total = calculate_legacy_total(order)
total = calculate_total(order)

Delete it. Git has history.

Journal Comments

// Bad: changelog in the file
// 2024-01-15 - Added validation
// 2024-02-01 - Fixed null check

That's what git log is for.

Refactoring decision guide

1. Is there a test for this code? If no, write one first. Never refactor without tests.
2. What smell do I see? Name it specifically.
3. What's the minimal refactoring move? Don't restructure the world.
4. Does the refactoring make the code more readable? If not, revert it.
5. Do the tests still pass? If not, the refactoring introduced a bug.

Naming Guide

Names are the most powerful tool for making code readable. A good name eliminates the need for comments, makes intent obvious, and prevents misuse. Invest time in names - it pays dividends every time someone reads the code.

Universal rules

Use intention-revealing names

The name should answer: why does this exist, what does it do, how is it used?

# Bad: requires a comment to understand
d = 86400  # elapsed time in seconds

# Good: name reveals intent
SECONDS_PER_DAY = 86400

Use pronounceable names

If you can't say it out loud, it's hard to discuss in code reviews and meetings.

// Bad
Date genymdhms;    // generation year-month-day-hour-minute-second

// Good
Date generationTimestamp;

Use searchable names

Single-letter names and numeric constants are hard to grep. The length of a name should be proportional to the size of its scope.

// Bad: searching for "5" finds thousands of results
if (items.length > 5) { ... }

// Good: searchable and self-documenting
const MAX_ITEMS_PER_PAGE = 5;
if (items.length > MAX_ITEMS_PER_PAGE) { ... }

Avoid encodings and prefixes

Don't embed type information, scope, or notation in names.

// Bad: Hungarian notation, member prefixes
let strName: string;
let m_count: number;
let IShapeFactory: interface;

// Good: let the type system handle types
let name: string;
let count: number;
interface ShapeFactory { ... }

Avoid mental mapping

Readers shouldn't have to mentally translate a name into a concept they already know.

# Bad: reader must remember that 'r' means 'urlRewritten'
r = rewrite_url(original)

# Good: no mapping needed
rewritten_url = rewrite_url(original)

Exception: Single-letter variables are fine in small scopes where the convention is universal: i, j for loop indices; x, y for coordinates; e for exception in a catch block.

Rules by entity type

Variables

Booleans

Booleans should read as true/false questions using prefixes like is, has, can, should, was:

Avoid negated boolean names - they cause double negatives:

# Bad: double negative is confusing
if not is_not_found: ...

# Good: positive name
if is_found: ...

Functions and methods

Functions should be verbs or verb phrases. The name should describe the action.

Accessors, mutators, and predicates follow conventions:

getName()       // accessor (get + noun)
setName(val)    // mutator (set + noun)
isActive()      // predicate (is + adjective)
hasPermission() // predicate (has + noun)

Classes

Classes should be nouns or noun phrases. Never verbs.

Avoid meaningless suffixes that add no information:

Exception: Suffixes that communicate a design pattern are valuable: EmailSender, OrderRepository, PaymentStrategy, UserFactory.

Constants

Use SCREAMING_SNAKE_CASE. The name should explain the meaning, not just the value.

# Bad: the value is obvious, the meaning is not
FIVE = 5
SIXTY = 60

# Good: explains meaning
MAX_RETRY_ATTEMPTS = 5
SESSION_TIMEOUT_SECONDS = 60

Enums

Enum type should be singular; members should be SCREAMING_SNAKE_CASE:

enum OrderStatus {
  PENDING,
  PROCESSING,
  SHIPPED,
  DELIVERED,
  CANCELLED,
}

Scope-length rule

The length of a name should be proportional to the scope in which it's used:

Naming consistency

Pick one word per concept

Choose one synonym and use it everywhere. Don't mix:

• fetch / retrieve / get / load - pick one
• create / make / build / generate - pick one
• controller / manager / handler - pick one

Use domain language

Use the terms your business domain uses (Domain-Driven Design's "ubiquitous language"). If the business says "policy," don't call it "rule" in code.

# Bad: invented terminology
class DiscountRule:
    def applies_to(self, purchase): ...

# Good: matches business language
class PricingPolicy:
    def applies_to(self, order): ...

Renaming checklist

When improving names in existing code:

1. Identify the name's scope and all usages
2. Choose a name that passes the "read aloud" test
3. Use your IDE's rename refactoring (not find-and-replace)
4. Verify tests still pass
5. Check that no external API contracts are broken

SOLID Principles

The five SOLID principles guide class and module design toward code that is easy to change, test, and extend. Apply them when you feel the pain of rigidity - not preemptively everywhere.

S - Single Responsibility Principle (SRP)

A class should have only one reason to change.

This means one actor (stakeholder or group) owns each class. When two different business concerns live in the same class, a change for one concern risks breaking the other.

Before (violates SRP)

class Employee:
    def calculate_pay(self):        # owned by accounting
        ...
    def generate_report(self):      # owned by reporting
        ...
    def save_to_database(self):     # owned by DBA/infra
        ...

After (SRP applied)

class PayCalculator:
    def calculate_pay(self, employee): ...

class EmployeeReporter:
    def generate_report(self, employee): ...

class EmployeeRepository:
    def save(self, employee): ...

When NOT to apply

• Tiny scripts or one-off utilities - splitting a 30-line script into 3 classes adds friction without value.
• Data classes / DTOs - a class that just holds data fields doesn't need splitting even if multiple consumers read from it.

O - Open/Closed Principle (OCP)

Software entities should be open for extension but closed for modification.

You should be able to add new behavior without changing existing, tested code. Achieve this through polymorphism, strategy pattern, or plugin architectures.

Before (violates OCP)

function calculateShipping(order: Order): number {
  if (order.type === 'standard') return 5.99;
  if (order.type === 'express') return 12.99;
  if (order.type === 'overnight') return 24.99;
  // Every new shipping type requires modifying this function
}

After (OCP applied)

interface ShippingStrategy {
  calculate(order: Order): number;
}

class StandardShipping implements ShippingStrategy {
  calculate(order: Order) { return 5.99; }
}

class ExpressShipping implements ShippingStrategy {
  calculate(order: Order) { return 12.99; }
}

// New types are added by creating new classes, not modifying existing code
function calculateShipping(order: Order, strategy: ShippingStrategy): number {
  return strategy.calculate(order);
}

When NOT to apply

• When you have 2-3 cases that rarely change - a simple if/switch is clearer than a strategy pattern with one implementation per case.
• Internal utilities where you control all callers - just change the code directly.

L - Liskov Substitution Principle (LSP)

Subtypes must be substitutable for their base types without altering correctness.

If code works with a base class, it must work identically with any subclass. Violations show up as instanceof checks or broken assumptions in calling code.

Classic violation: Square/Rectangle

class Rectangle {
    void setWidth(int w) { this.width = w; }
    void setHeight(int h) { this.height = h; }
    int area() { return width * height; }
}

class Square extends Rectangle {
    void setWidth(int w) { this.width = w; this.height = w; }   // breaks LSP
    void setHeight(int h) { this.width = h; this.height = h; }  // breaks LSP
}

// Calling code assumes width and height are independent - Square breaks that
Rectangle r = new Square();
r.setWidth(5);
r.setHeight(10);
assert r.area() == 50; // FAILS: area is 100

Fix

Don't make Square extend Rectangle. Use a Shape interface with an area() method that each implements independently.

When NOT to apply

• LSP is rarely "over-applied" - it's more of a diagnostic. If your subtypes work correctly everywhere the parent is used, you're fine. Don't create elaborate type hierarchies just to prove LSP compliance.

I - Interface Segregation Principle (ISP)

Clients should not be forced to depend on methods they do not use.

Fat interfaces that serve multiple clients force unnecessary coupling. Split them into focused, role-specific interfaces.

Before (violates ISP)

interface Worker {
  work(): void;
  eat(): void;
  sleep(): void;
  attendMeeting(): void;
}

// A Robot worker is forced to implement eat() and sleep()
class Robot implements Worker {
  work() { /* ... */ }
  eat() { throw new Error('Robots do not eat'); }  // forced stub
  sleep() { throw new Error('Robots do not sleep'); }
  attendMeeting() { /* ... */ }
}

After (ISP applied)

interface Workable { work(): void; }
interface Feedable { eat(): void; }
interface Restable { sleep(): void; }
interface Meetable { attendMeeting(): void; }

class Robot implements Workable, Meetable {
  work() { /* ... */ }
  attendMeeting() { /* ... */ }
}

When NOT to apply

• Don't split an interface that has a single consumer into micro-interfaces of one method each. ISP solves the problem of multiple consumers with different needs.
• Standard library or framework interfaces (e.g. Iterable) shouldn't be split even if some methods go unused.

D - Dependency Inversion Principle (DIP)

High-level modules should not depend on low-level modules. Both should depend on abstractions.

The business logic (policy) should define the interface it needs. Infrastructure (database, HTTP, filesystem) implements that interface and is injected.

Before (violates DIP)

class OrderService:
    def __init__(self):
        self.db = MySQLDatabase()         # hard-coded dependency
        self.mailer = SmtpEmailSender()   # hard-coded dependency

    def place_order(self, order):
        self.db.save(order)
        self.mailer.send(order.customer.email, "Order placed")

After (DIP applied)

class OrderService:
    def __init__(self, repository: OrderRepository, notifier: Notifier):
        self.repository = repository
        self.notifier = notifier

    def place_order(self, order):
        self.repository.save(order)
        self.notifier.notify(order.customer, "Order placed")

Now OrderService depends on abstractions (OrderRepository, Notifier) that it defines. Tests can inject fakes; production wires in real implementations.

When NOT to apply

• Simple scripts or small applications where you control the full stack and dependency injection adds ceremony without testability benefit.
• When there will only ever be one implementation (e.g. wrapping a single third-party SDK) - inject the concrete class directly; add the interface when a second implementation actually appears.

Applying SOLID pragmatically

Use this decision heuristic:

1. Feel the pain first - Don't apply SOLID preemptively. Wait until a change is hard, a test is awkward, or a class is confusing.
2. Identify which principle is violated - The symptom tells you the principle:
   • Hard to change without breaking other things -> SRP or OCP
   • Subclass doesn't work where parent is expected -> LSP
   • Forced to implement empty methods -> ISP
   • Can't test without real database/network -> DIP
3. Apply the minimum fix - Don't restructure the whole codebase. Fix the one class or interface causing the problem.
4. Verify with tests - The refactoring should make tests easier to write, not harder. If you need more mocks after applying SOLID, you may have over-applied.

Test-Driven Development (TDD)

TDD is a discipline where tests drive the design of the code. You write the test first, watch it fail, make it pass with minimal code, then refactor. This cycle produces code that is only as complex as needed and always has test coverage.

The Three Laws of TDD

1. You may not write production code until you have written a failing test.
2. You may not write more of a test than is sufficient to fail (compilation failures count as failures).
3. You may not write more production code than is sufficient to pass the currently failing test.

These laws create a tight feedback loop: red-green-refactor cycles of 1-5 minutes.

Red-Green-Refactor

Red

Write a test that describes the behavior you want. Run it. It must fail. If it passes, either the behavior already exists or the test is wrong.

def test_new_account_has_zero_balance():
    account = Account()
    assert account.balance == 0

Green

Write the simplest, most naive code that makes the test pass. Don't optimize. Don't handle edge cases you haven't tested yet. Just make it green.

class Account:
    def __init__(self):
        self.balance = 0

Refactor

With the test green, clean up both production and test code. Remove duplication, improve names, extract methods. The tests protect you - if you break something, you'll know immediately.

What makes a clean test

FIRST Principles

Arrange-Act-Assert (AAA)

Every test should have exactly three sections:

def test_deposit_increases_balance():
    # Arrange
    account = Account()

    # Act
    account.deposit(100)

    # Assert
    assert account.balance == 100

Keep each section small. If Arrange is long, extract a factory or builder. If Act is more than one line, the API might be too complex.

One assert per concept

A test should verify one behavioral concept. Multiple asserts are fine if they all verify the same concept from different angles:

# Good: one concept (user creation) verified from multiple angles
def test_create_user():
    user = create_user("Alice", "alice@test.com")
    assert user.name == "Alice"
    assert user.email == "alice@test.com"
    assert user.is_active is True

# Bad: two unrelated concepts in one test
def test_user_operations():
    user = create_user("Alice", "alice@test.com")
    assert user.name == "Alice"       # concept 1: creation
    user.deactivate()
    assert user.is_active is False    # concept 2: deactivation

Test naming

Tests should read as behavior specifications. Use this pattern:

test_<action>_<scenario>_<expected_result>

Examples:

• test_withdraw_with_sufficient_funds_reduces_balance
• test_withdraw_exceeding_balance_raises_insufficient_funds
• test_new_account_has_zero_balance

Avoid generic names like test_withdraw_1, test_edge_case, or test_bug_fix.

What to test

Test behavior, not implementation

# Bad: tests implementation detail (internal list structure)
def test_add_item():
    cart = Cart()
    cart.add(item)
    assert cart._items[0] == item     # reaching into private state

# Good: tests observable behavior
def test_add_item():
    cart = Cart()
    cart.add(item)
    assert cart.item_count() == 1
    assert cart.contains(item)

Test boundaries, not internals

Focus tests on:

• Public API - the contract other code depends on
• Edge cases - empty inputs, zero, negative, maximum values
• Error paths - what happens when things go wrong
• Business rules - the logic that encodes domain requirements

Don't test:

• Private methods (test them through the public API)
• Framework behavior (trust that your ORM saves correctly)
• Trivial getters/setters (unless they have logic)
• Implementation details that could change during refactoring

Test doubles

Use the right kind of test double for the situation:

Prefer stubs over mocks. Mocks create tight coupling to implementation. If you refactor how a method achieves its result, mock-heavy tests break even though behavior is unchanged.

Prefer fakes for infrastructure. An in-memory repository is better than mocking every database call.

Common TDD mistakes