Code Smells

naturally, the credit for the contents here go to refactoring.guru

overview#

code smells are surface indicators that usually correspond to deeper problems in the system. they are not bugs – the code still functions – but they signal weaknesses in design that may slow down development or increase the risk of future bugs.

bloaters vs dispensables

bloaters still encode functionality – you cannot simply delete them. dispensables can be straight-up removed.

couplers vs change preventers

couplers make change difficult by binding classes tightly. change preventers make change widespread: a single logical change forces modifications in many places.

bloaters#

these smells make the code hard to work with. they do not usually appear immediately, but are a consequence of program evolution and accumulation over time.

long method#

when the method in a class is too long. you can split a method up into many smaller methods.

signs#

• method exceeds 10-15 lines
• you need comments to explain sections of the method
• deeply nested conditionals or loops

refactoring techniques#

• extract method: pull out cohesive chunks into named helper methods
• replace temp with query: if a variable is computed once and used later, make it a method call
• introduce parameter object: if multiple params travel together, bundle them

large class#

a consequence of the fact that it is easier to add functionality to an existing class (as a programmer), than to create a new class.

signs#

• class has many fields or methods
• class has multiple unrelated responsibilities
• you find yourself only using a subset of the class for any given task

refactoring techniques#

• extract class: split into multiple classes, each with a single responsibility
• extract subclass: if some features are only used in certain cases
• extract interface: if clients only need a subset of behaviour

primitive obsession#

these get spread wide and far. it emerges from a degree of laziness. passing around data as:

String email;
int age;
double latitude;
double longitude;

whilst it should really look like:

EmailAddress email;
Age age;
GeoPoint location;

we distinguish between creating a meaningful class and a “data class” (which is another smell);

data class

contains only fields (and maybe getters / setters), but no meaningful behaviour

value object

contains invariants, validation, domain behaviour, meaningful operations, encapsulation.

in summary, the value object knows it exists and exists for a reason – to model a concept. unlike the data class, which just moves the smell and exists to “carry data”

refactoring techniques#

• replace data value with object: wrap the primitive in a small class
• introduce parameter object: group related primitives
• replace type code with class/subclass/strategy: if a primitive controls behaviour

data clumps#

a sibling smell to primitive obsession, but with the added concern that the relationship between values is not explicit.

in the case of primitive obsession, a single concept was being represented using raw primitives as opposed to a domain type.

here, we have a set of variables repeatedly appearing together that should be treated as one concept:

double x, y;
double latitude, longitude;
int day, month, year;

but often times, the data clumps are indeed made up of primitives, so the smells reinforce each other.

refactoring techniques#

• extract class: create a new object with the relevant attributes
• ensure the new class has meaningful behaviour beyond just getters/setters (validation, formatting, calculations)

example#

instead of passing startDate, endDate, timezone separately everywhere, create a DateRange class with methods like contains(date), overlaps(other), duration().

long parameter list#

when the parameters are all related:

void createOrder(
    String customerId,
    String email,
    String street,
    String city,
    String postcode,
    String country,
    BigDecimal amount,
    Currency currency
)

we can create a value object as with the primitive obsession fix, to call the method as such:

void createOrder(CustomerId customerId, Address address, Money total)

refactoring techniques#

• introduce parameter object: bundle related parameters
• preserve whole object: pass the object instead of extracting its parts
• replace parameter with method call: if a param can be obtained from another object the method already has access to

object-orientation abusers#

these smells indicate incomplete or incorrect application of object-oriented principles.

alternative classes with different interfaces#

when two classes perform identical functions but have different method names.

happens when the programmer doesn’t know about existing functionality.

signs#

• two classes that do the same thing but have different method signatures
• you find yourself writing adapter code frequently

refactoring techniques#

• rename method: make signatures consistent
• extract superclass: pull common behaviour into a shared base
• extract interface: define a common contract

refused bequest#

when subclasses inherit functionality that they do not need.

a subclass inherits fields/methods it doesn’t want or can’t sensibly support (often overrides to “do nothing”, throw, or leaves inherited behaviour unused) – inheritance is the wrong relationship.

example#

class Square extends Rectangle

if Rectangle has setWidth(w) and setHeight(h), a Square can’t accept them independently:

• square.setWidth(5) must also change height, breaking expectations of Rectangle users, or
• it throws/ignores setHeight​/​setWidth (refusing the bequest)

this is a sign to use composition or redesign the hierarchy.

refactoring techniques#

• replace inheritance with delegation: use composition instead
• extract superclass: create a more abstract parent that only contains truly shared behaviour

switch statements#

multiple switch statements in multiple places are the smell.

usually one switch is okay, i.e. to create objects in the factory method or otherwise.

signs#

• the same switch/case appears in multiple places
• adding a new type requires changes across multiple files
• switch on type codes instead of using polymorphism

refactoring techniques#

• replace conditional with polymorphism: use subclasses or strategy objects
• replace type code with subclasses: let the type system handle dispatch
• introduce null object: eliminate null checks if they’re part of the switching

temporary field#

temporary fields get their value (and thus are needed by objects) only under certain circumstances.

outside of these circumstances, they’re empty.

signs#

• fields that are only set in some methods
• null checks scattered throughout the class
• fields used to pass data between methods instead of using parameters

refactoring techniques#

• extract class: move the field and related behaviour into a new class
• introduce null object: replace conditional null checks
• replace method with method object: turn the method into its own class if it needs temporary state

change preventers#

these smells make any change to the codebase significantly more difficult than it should be.

divergent change#

when the same class is changed for many reasons.

violation of the single responsibility principle

signs#

• one class keeps getting modified for unrelated features
• you have to touch the same file for database changes, UI changes, and business logic changes

refactoring techniques#

• extract class: split the class so each part has a single reason to change
• extract module: group related functionality into separate modules

parallel inheritance hierarchies#

whenever you create a subclass for a class, you find yourself needing to create a subclass for another class.

signs#

• class hierarchies that mirror each other
• prefixes of class names match across hierarchies (e.g., InvoiceProcessor and InvoiceRenderer)

refactoring techniques#

• move method/field: make one hierarchy refer to instances of the other
• collapse hierarchy: eliminate one of the parallel trees if possible

shotgun surgery#

when a change to one class necessitates many small changes to other classes.

one logical change forces edits in many classes/files.

signs#

• a single feature change touches 10+ files
• related logic is scattered across the codebase
• high risk of missing a change location

refactoring techniques#

• move method/field: consolidate related behaviour into one class
• inline class: if a class is doing too little, merge it back

dispensables#

code that is pointless and unnecessary. its absence would make the code cleaner, more efficient, and easier to understand.

comments#

usually an indication of poorly named variables, bad logic or otherwise a band-aid for bad code.

comments that explain “what” the code does (rather than “why”) suggest the code itself is unclear.

when comments are okay#

• explaining why a non-obvious decision was made
• documenting public APIs
• warnings about consequences
• TODOs (temporarily)

refactoring techniques#

• extract method: give a chunk of code a descriptive name
• rename method/variable: let the code explain itself
• introduce assertion: make assumptions explicit in code rather than comments

duplicate code#

when two code fragments look almost identical.

signs#

• copy-paste code that has drifted slightly apart
• fixing a bug requires changes in multiple places
• the same algorithm appears with minor variations

refactoring techniques#

• extract method: pull common code into a shared method
• extract class: if duplication spans classes
• pull up method: if duplication is in sibling subclasses
• form template method: if algorithms are similar but differ in steps

data class#

classes that do not provide any additional functionality. they only contain fields with at most methods for getting and setting these variables.

additionally, these classes cannot independently operate on the data that they own.

a class that mostly holds fields + getters/setters, with little behaviour.

signs#

• class is all fields and accessors
• behaviour that operates on the data lives in other classes
• other classes reach in to manipulate the data

refactoring techniques#

• move method: move behaviour that operates on the data into the class
• encapsulate field: hide fields behind meaningful methods
• encapsulate collection: don’t expose internal collections directly

dead code#

when a variable, parameter, field, method or class is no longer used – usually because it is obsolete.

signs#

• unreachable code paths
• unused parameters or variables
• commented-out code
• methods that are never called

refactoring techniques#

• delete it: version control keeps the history
• use IDE tools to find unused code

lazy class#

understanding and maintaining classes always costs time and money. so if a class doesn’t do enough to earn your attention, it should be deleted.

a class doesn’t do enough to justify its existence (too little behaviour/responsibility).

signs#

• class has only one or two trivial methods
• class exists only because “we might need it later”
• class that just delegates everything to another class

refactoring techniques#

• inline class: merge it into the class that uses it
• collapse hierarchy: if it’s a subclass that adds nothing

speculative generality#

when we write code for “future use”, but that code is speculative and not connected to the “in use” code in any way.

extra abstraction/hooks “for future use” that aren’t needed now (unused params, abstract classes, extension points).

signs#

• abstract classes with only one concrete implementation
• parameters or methods that are never used
• overly generic names like doProcess or handleData

refactoring techniques#

• collapse hierarchy: remove unnecessary abstract classes
• inline class/method: bring code back to where it’s used
• remove parameter: delete unused parameters
• embrace YAGNI (You Aren’t Gonna Need It)

couplers#

these smells result in excessive coupling between classes, or show what happens when coupling is replaced by excessive delegation.

feature envy#

when a method accesses the data of another object more than its own data.

a method uses another class’s data/behaviour more than its own; it “wants to live” in the other class.

signs#

• method makes many calls to another object’s getters
• method knows too much about another class’s structure
• extracting the method makes you want to move it elsewhere

refactoring techniques#

• move method: put the method where the data lives
• extract method: if only part of the method envies another class

incomplete library class#

eventually a library will stop meeting our needs and we’ve now got heaps of work-arounds and wrappers.

this causes our code to become less maintainable and increases technical debt.

as such, this library is now insufficient for our particular requirements.

refactoring techniques#

• introduce foreign method: add a utility method in your own class
• introduce local extension: create a wrapper or subclass of the library class
• consider replacing the library altogether if the mismatch is severe

middle man#

if a class performs only one action; delegating work to another class - why keep it at all?

a class mostly delegates calls to another class and adds little/no value.

signs#

• most methods just call methods on another object
• the class adds no behaviour or state of its own
• clients could easily use the delegate directly

refactoring techniques#

• remove middle man: let clients talk directly to the delegate
• inline method: if only a few methods delegate

inappropriate intimacy#

one class uses the internal fields and methods of another class.

two classes are too tightly coupled (reach into each other’s internals / lots of private-field access).

signs#

• classes access each other’s private members (via reflection or friend access)
• circular dependencies between classes
• changes to one class frequently break the other

refactoring techniques#

• move method/field: put behaviour where it belongs
• extract class: create an intermediary to mediate between them
• hide delegate: reduce direct access
• change bidirectional association to unidirectional

message chains#

when you see a series of calls resembling a->b()->c()->d().

long call chains like a.getB().getC().doThing(); lots of knowledge of object structure.

violation of the law of demeter

signs#

• chains of getX().getY().getZ() calls
• client code knows the internal structure of many objects
• changes to intermediate objects break distant code

refactoring techniques#

• hide delegate: make intermediate objects provide what’s needed
• extract method: encapsulate the chain in a method on the first object
• move method: put the behaviour where it can access the data directly