Code Smells

1. Introduction

1.1. Goals

Himanshu Khagta/Getty Images

Building good quality software design is hard and requires experience
We will discuss good design principles and design improvement later
For now, let us learn how to flair bad design

1.2. Definition

A code smell is a hint that something has gone wrong somewhere in your code.

— Kent Beck

1.3. The Good, the Bad, and the Ugly

A code smell is:

A sign that something in the code is not good
- not necessarily a certainty
- not necessarily bad
- certainly ugly

In other words: use your flair to find bad code!

There’s good code and bad code, right? Well, there is, but there’s also a lot of code in the middle. Code that’s pretty good, but could be better. What’s good about it and what’s bad? We want to save the part’s that are good, and fix the parts that are less good. That’s what refactoring is all about.

To refactor the code, however, we have to develop a sense of what’s bad. There are design principles that make a pretty clear distinction and there are heuristics that, with thought, can generally indicate the difference. Sometimes something is a little bad, but it’s the best we can do right now for reasons out of our control. Perhaps the alternatives are worse, so we live with it. But we need to develop a nose for code that "smells bad" so that we recognize it quickly. Then we can fix it, or even prevent it from going into the system in the first place.

1.4. List of Code Smells

Comprehensive: when smells affect the whole code
Overweighted: when things get too big
Lack of abstraction: when some more design is needed
OO Gluttons: when too much OO is used
OO Timidness: when the code is not OO enough
Naming: when the problem is the identification

2. Comprehensive

2.1. Comprehensive Code Smells

object DRY {
  def main(args: Array[String]) = {
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
    println("I will not repeat myself")
  }
}

Code Smells that affect the whole code

Duplicated code
Comments
Dead code

Comprehensive code smells are related to the whole code

2.2. Duplicated Code

Symptom

Two or more code snippets that have a similar behavior

Causes

Often a result of «Copy and Paste Programming»
May occur when different developers independently write similar code

extern int array_a[];
extern int array_b[];

int sum_a = 0;
for (int i = 0; i < 4; i++)
   sum_a += array_a[i];

int average_a = sum_a / 4;

int sum_b = 0;
for (int i = 0; i < 4; i++)
   sum_b += array_b[i];

int average_b = sum_b / 4;

Source: Wikipedia

2.3. Duplicated code — Problems

Contrary to the principle «Once and Only Once»^[1]
Duplicate code makes the system hard to understand and thus hard to maintain:
- Any change must be duplicated
- The maintainer must be aware of the duplications

2.4. Comments

Code never lies, comments sometimes do.

— Ron Jeffries

Symptoms

Lots of useless comments
Comments that disguise bad naming choices
Code snippets that are unintelligible without comments
Comments that do not correspond to the code

2.5. Comments — Problems

A comment should describe an intention and not explain an action
Too many unnecessary comments overloads the code and make it unreadable
Comments may hide deeper problems
Comments must be maintained along with the code

// convert to meters
a = x * 1000

// average meters driver
avg = a / n

2.6. Dead Code

Symptoms

Code snippets, variables, parameters, methods, or classes that are never executed

Causes

Maintenance after requirement changes or error correction
Code used only for testing

public calculatePrice(Product p) {
  double priceAfterTaxes = p.getPrice() * qty * tax;

  return p.getPrice() * qty;
}

2.7. Dead Code — Problems

Empirical

Reduces understandability: lost of time reading dead code
Gives the impression of poor testing (bad code coverage)

3. Overweighed

When more is not enough

3.1. Overweighted Code Smells

Large Classes
God Classes
Long Methods

Bloaters are code, methods and classes that have increased to such gargantuan proportions that they are hard to work with. Usually these smells do not crop up right away, rather they accumulate over time as the program evolves (and especially when nobody makes an effort to eradicate them).

3.2. Large classes

Symptoms

Too many lines of code
How many?
- No metric fits all cases

Example:

class org.eclipse.emf.common.util.URI
6,193 lines of code
Non-commenting source statements: 2,034
Methods: 115
Inner classes: 5

A class that is trying to do too much can usually be identified by looking at how many instance variables it has.
When a class has too many instance variables, duplicated code cannot be far behind.

3.3. Large Classes — Problems

Empirical

Too many lines of code reduces the Readability/Comprehensibility and thus, the Maintainability and the Debuggability
Often, an excessive number of methods and/or attributes hides a duplication of code.

Conceptual

The class probably hides more than one concept, reducing the Testability and the Reusability
Look for disparate sets of methods and instance variables

Violates the Single Responsibility Principle

Readability/Comprehensibility: too many lines of code is hard to master
Maintainability: how to isolate a bug?
Testability
Reusability (the class hides more than one concept): hard to reuse both

3.4. God Classes

Characteristics

A class that controls several other classes and has grown beyond all logic to become the class that does everything
- The other classes are often «Data Classes»
Often, «God classes» are «Large Classes» (and inversely)

Symptoms

The class uses directly several attributes of other classes
Functional complexity is very high
Class cohesion is low

3.5. God Classes — Problems

Empirical

Hard to test and reuse

Conceptual

Contrary to the «Divide and Conquer» strategy
A class that does everything is not very different from a procedural program

3.6. Long method

Symptoms

Too many lines of code
How many?
- Again, no metric will always be correct

Example:

Method parseIntoURI(String uri): about 300 lines.

3.7. Long Methods — Problems

Empirical

The longer a method is, the more difficult it is to understand how it works
The more execution paths a method has, the less it is testable (more test data is needed)

Conceptual

The method is the smallest unit of overriding ^[2]:
- It is hard to override complex behaviors
Statements within a method should be at the same level of abstraction

Polymorphisme d’inclusion = redéfinition/spécialisation de méthodes durant l’héritage = overriding Polymorphisme ad hoc = surcharge de méthodes = overloading Polymorphisme paramétrique = méthodes génériques = templates/generics

4. Lack of Abstraction

Should I create a new class only for that?

4.1. Lack of Abstraction Code Smells

Primitive Obsession
Long Parameter List
Data clumps
Shotgun Surgery

Dilbert by Scott Adams

4.2. Primitive Obsession

Symptoms

Use primitive types to represent simple domain data: amounts of money, telephone number, social security number, etc.

public class Account {
   private String 	name;
   private int		accountNumber;
   private String 	email;
   private String 	address;
   private int		socialSecurityNumber;
   private float	weight;
   private double	balance;
}

4.3. Primitive Obsession — Problems

Empirical

The lack of unities (g, m, sec, etc.) stimulates type mismatch errors

Conceptual

Introduces duplicate code:
- For instance, if the socialSecurityNumber is used elsewhere in the code, the verification code will de duplicated

4.4. Long Parameter List

Symptoms

Methods with more that 3 or 4 parameters
Methods that only manipulates data from the parameters

Listing 1. Example

public static URI createHierarchicalURI(String scheme, String authority,
                                        String device, String[] segments,
                                        String query, String fragment) {
  if (device != null) {
    if (isArchiveScheme(scheme)) {
      throw new IllegalArgumentException("archive URI with device");
    }
    if (SCHEME_PLATFORM.equals(scheme)) {
      throw new IllegalArgumentException("platform URI with device");
    }
  }
  return POOL.intern(false, URIPool.URIComponentsAccessUnit.VALIDATE_ALL, true, scheme, authority, device, true, segments, query).appendFragment(fragment);
}

4.5. Long Parameter List — Problems

A long parameter list often hides a missing abstraction
A method with too many parameters is seldom reusable
Error prone (argument permutation)

Don’t pass in everything the method needs; pass in enough so that the method can get to everything it needs.
Replace Parameter with Method
Preserve Whole Object Introduce Parameter Object

4.6. Data Clumps

Symptoms

Two or more variables or parameters that are always found together

Listing 2. Examples

public double distance(double x1, double y1, double x2, double y2);
public double move(double x1, double y1, double x2, double y2);
public void saveThisMoment(int year, int month, int day, int hour, int minutes, int seconds);
public void setBirth(int year, int month, int day, int hour, int minutes, int seconds);

Clumps of data items that are always found together.
Turn the clumps into an object with Extract Class Then continue the refactoring with Introduce Parameter Object or Preserve Whole Object

4.7. Data Clumps — Problems

Empirical

Error prone (possible argument inversion)

Conceptual

They hide a lack of abstraction
- Examples: Point, DateTime, etc.
Promote code duplication

4.8. Shotgun Surgery

Causes

A responsibility/concern that was split up among several methods
Speculative over layering
Copy-paste coding

Symptom

Changing a simple feature/property results in several changes in other classes.

Listing 3. Example

public class Account {

    public void debit(double debit) throws Exception {
        if (balance <= 100) {
            throw new Exception("Mininum balance is 100");
        }
        balance = balance - debit;
    }

    public void transfer(Account from, Account to, double transferAmount) throws Exception {
        if (from.balance <= 100) {
            throw new Exception("Mininum balance is 100");
        }
        to.balance = balance + transferAmount;
    }

    public void sendWarningMessage() {
        if (balance <= 100) {
            System.out.println("Balance should be over 100");
        }
    }
}

Speculative Over Layering: Another common example arises from speculative over-architecting. Have you ever seen a codebase to handle a simple CRUD app, but that defined multiple layers, complete with data transfer objects, data access objects, domain objects, and so on? And so every time you want to add a table to the database, you now have to add scaffolding across all four layers in addition to the various property bag objects within those layers? This is another shotgun surgery situation.
Copy-Paste Coding: And then, there is the most common and straightforward example: changing a codebase with copy-paste code everywhere. This means that changes to some of the copy-paste code require you to make those same changes to each additional incarnation.

4.9. Shotgun Surgery — Problems

Empirical

Time consuming:
- modifications in the specific behavior imply several small modifications
Duplicated code:
- merge conflicts become more likely
- leads to bug introduction (partial changes)
The development of small features takes more time
Eases error introduction

Conceptual

Poor separation of concerns.
A sign that the developer failed to identify single responsibilities
Seep learning curves for newcomers

5. Object-Orientation Gluttons

5.1. Object-Oriented Gluttons Code Smells

Too many private methods
Parallel Inheritance Hierarchies
Message Chains
Middle Man
Speculative Generality

5.2. Too Many Private Methods

Symptoms

Too many private methods
- As for large classes, no metric fits all cases
Code that cannot be tested, because it is private

5.3. Too many private methods — Problems

Conceptual

Methods should be public, unless they violate a class invariant.
Indication that a class is doing too many things

Empirical

Private methods cannot be tested.
They cannot be reused as well

5.4. Parallel Inheritance Hierarchies

Causes

Overenthusiasm to break each functionality as a separate interface
- worked as long as the hierarchy stayed small

Symptoms

Every time you make a subclass of one class, you also have to make a subclass of another
Often, classes from both hierarchies share a same prefix and/or suffix

5.5. Parallel Inheritance Hierarchies — Problems

Conceptual

Misunderstanding of the single responsibility principle

Empirical

Code maintenance and extension becomes harder and harder

5.6. Message Chains

Symptoms

Code snippets resembling o.a().b().c().d()

Listing 4. Message chain examples

customer.getAddress().getState();
window.getBoundingbox().getOrigin().getX();

5.7. Message Chains — Problems

Empirical

There is an implicit dependency between the caller and the implementor through a chain of objects
- Any change in the chain will impact the caller
The system becomes harder to test

Conceptual

Breaks the Law of Demeter

5.8. Speculative Generality

Symptoms

Over-generalized code in an attempt to predict future needs.
Unused classes, methods, attributes, or parameters.

Causes

«What if..» school of design

5.9. Speculative Generality — Problems

Conceptual

Against the YAGNI ^[3] principle
Wrong identification of the variability points

Empirical

Code becomes hard to understand and maintain

5.10. Middle Man

Causes

Objects encapsulates (hides) details
Encapsulation leads to delegation
Sometimes, it goes to far!

Symptom

A class that is doing too much simple delegation instead of really implementing a behavior

5.11. Middle Man — Problems

Conceptual

If a class performs only delegates work to other classes, why does it exist at all?

Exceptions

Some Design Patterns are Middle Man: Mediator and Facade

6. Object-Orientation Timidness

6.1. Object-Orientation Timidness List

Data Classes
Feature Envy
Deeply Nested Code
Temporary Attributes
Refused Bequest
Alternative Classes with Different Interfaces
Utility Methods

6.2. Data Classes

Causes

Procedural programming influence, with procedures and records

Symptoms

Classes with attributes, getters and setters and nothing else

Listing 5. Data Class Example

package fr.unantes.test.badcode;

public class Company extends Person {
    private String companyName;
    private String phone;

    public String getCompanyName() {
        return this.companyName;
    }

    public void setCompanyName(String companyName) {
        this.companyName = companyName;
    }

    public String getPhone() {
        return this.phone;
    }

    public void setPhone(String phone) {
        this.phone = phone;
    }
}

6.3. Data Classes — Problems

Conceptual

Breaks encapsulation
Increases coupling
Reduces cohesion

Empirical

The code is hard to understand and maintain

6.4. Feature Envy

Causes

Procedural programming influence

Symptoms

A method that uses more features of another class than of its own.
- Sometimes just a portion of a method

public class EnterpriseGroup extends Group {
    public String toString() {
        String display;
        display = "Group: " + this.nom + "\n\n";
        for (int i = 0; i < this.persons.size(); i++) {
            display += ((Enterprise) this.persons.get(i)).getCompanyName() + "\n";
        }
        return display;
    }
}

6.5. Feature Envy — Problems

Conceptual

Increases coupling

Empirical

The code is hard to understand and maintain

6.6. Deeply Nested Code

Symptom

Deeply nested code, usually loops and/or conditionals

 public MappedField getMappedField(final String storedName) {
     for (final MappedField mf : persistenceFields) {
         for (final String n : mf.getLoadNames()) {
             if (storedName.equals(n)) {
                 return mf;
             }
         }
     }
     return null;
 }

6.7. Deeply Nested Code — Problems

Conceptual

Symptom of methods in the wrong place

Empirical

The code is hard to understand
They tend to grow more and more become complicated over time
- developers keep adding conditions and more levels of nesting

6.8. Temporary Attributes

Symptoms

Attributes that are only used by certain methods or under certain circumstances, but remain unused the rest of the time

Causes

A developer that didn’t to know where else to put a variable
An algorithm that requires a large number of input variables

6.9. Temporary Attributes -– Problems

Empirical

The code is hard to understand, maintain, and debug
- Why is this attribute null here?
- Is it really needed?

Conceptual

Breaks the single responsibility principle:
- a single class hiding two conceptual classes

6.10. Refused Bequest

Symptom

A subclass that only uses some of the features from its parents

Bequest = Don

6.11. Refused Bequest — Problems

Conceptual

Wrong inheritance hierarchy
Violates the Liskov substitution principle

Empirical

The code is hard to test

Polymorphism broken: an instance of Company cannot replace an instance of Government

6.12. Alternative Classes with Different Interfaces

Symptom

Two classes with similar behavior, but with different method signatures

Cause

The developer of one class wasn’t aware of the existence of the other

6.13. Alternative Classes with Different Interfaces — Problems

Empirical

Duplicate code makes the code hard to maintain and understand

6.14. Utility Methods

Symptom

A method with no reference (explicit nor implicit) to self or this

Cause

The developer doesn’t know where to put the method
The class the methods should belong doesn’t exist

class Date {
  private int day, month, year;

  public static boolean isLeapYear(int year) {
    return ((year % 4 == 0)
      && (year % 100 != 0))
      || (year % 400 == 0);
  }
}

6.15. Utility Methods — Problems

Conceptual

Violates the single responsibility principle
Decreases cohesion

Empirical

Decreases testability

7. Naming

7.1. Naming Code Smells

Type Embedded in Name
Uncommunicative Name
Inconsistent Names

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

— Leon Bambrick

erreur de décalage unitaire

7.2. Type Embedded in Name

Symptoms

Methods that have the parameter’s type in their name

Listing 6. Examples

int priceInt = 5;

public void addCourse(Course c) {}

Problem

Affects maintainability:
- the identifier (method, parameter) must be renamed if the type changes

7.3. Uncommunicative Name

Symptom

Uncommunicative identifier names

Listing 7. Examples

public void process(String data, String data2, String data3);

/** Modify the value viewed through the lens, returning a `C` on the side. */
def modp[C](f: B1 => (B2, C), a: A1): (A2, C) = {
  val (b, c) = f(get(a))
  (set(a, b), c)
}

Lens class from the Scalaz project

Problem

Affects readability

Choose names that communicate intent (pick the best name for the time, change it later if necessary).

7.4. Inconsistent Names

Symptom

Classes playing a same role, using different suffixes
Project with no terminology

Examples

Managers: ClientService, DocumentProcessor, ProductManager
Factories: ClientFactory, CustomerProvider, ProductCreator, ConnectionBuilder

Problem

Project with no style nor coherent terminology is harder to understand

There is no best choice, as long as the terminology is consistent

8. Conclusion

8.1. Summary

Code Smells are heuristics to detect signs of bad design
Smells are not errors and are not necessarily bad

8.2. More readings

1. An objective of OOAD, hard to reach without collateral effects, such as unnecessary coupling.

2. In French: Polymorphisme d’inclusion: redéfinition/spécialisation de méthodes durant l’héritage

3. You Ain’t Gonna Need It