Code Smells

Heuristics for Detecting Bad Code

University of Nantes - LS2N, France

https://sunye-g.univ-nantes.io

Gerson Sunyé gerson.sunye@univ-nantes.fr

Plan

Introduction

Goals

sniffer

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

Definition

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

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!

List of Code Smells

  1. Comprehensive: when smells affect the whole code

  2. Overweighted: when things get too big

  3. Lack of abstraction: when some more design is needed

  4. OO Gluttons: when too much OO is used

  5. OO Timidness: when the code is not OO enough

  6. Naming: when the problem is the identification

Comprehensive

When distinct scents make the whole code smell

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

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

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

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

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

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

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;
}

Dead Code — Problems

Empirical

  • Reduces understandability: lost of time reading dead code

  • Gives the impression of poor testing (bad code coverage)

Overweighed

When more is not enough

Overweighted Code Smells

garfield

  • Large Classes

  • God Classes

  • Long Methods

Large classes

Symptoms

  • Too many lines of code

  • How many?

    • No metric fits all cases

Example:
large class

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

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

  1. The class uses directly several attributes of other classes

  2. Functional complexity is very high

  3. Class cohesion is low

god programming

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

god class

Long method

Symptoms

  • Too many lines of code

  • How many?

    • Again, no metric will always be correct

Example:

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 [1]:

    • It is hard to override complex behaviors

  • Statements within a method should be at the same level of abstraction

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

Lack of Abstraction

Should I create a new class only for that?

Lack of Abstraction Code Smells

  • Primitive Obsession

  • Long Parameter List

  • Data clumps

  • Shotgun Surgery

dt120107
Dilbert by Scott Adams

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;
}

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

Long Parameter List

Symptoms

  • Methods with more that 3 or 4 parameters

  • Methods that only manipulates data from the parameters

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);
}

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)

Data Clumps

Symptoms

  • Two or more variables or parameters that are always found together

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);

Data Clumps — Problems

Empirical

  • Error prone (possible argument inversion)

Conceptual

  • They hide a lack of abstraction

    • Examples: Point, DateTime, etc.

  • Promote code duplication

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.

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");
        }
    }
}

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

language learning curves

Object-Oriented Gluttons

I see objects everywhere

Object-Oriented Gluttons Code Smells

  • Too many private methods

  • Parallel Inheritance Hierarchies

  • Message Chains

  • Middle Man

  • Speculative Generality

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

Diagram

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

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

Diagram
Diagram

Parallel Inheritance Hierarchies — Problems

Conceptual

  • Misunderstanding of the single responsibility principle

Empirical

  • Code maintenance and extension becomes harder and harder

Message Chains

Symptoms

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

Message chain examples
customer.getAddress().getState();
window.getBoundingbox().getOrigin().getX();

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

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

Diagram

Speculative Generality — Problems

Conceptual

  • Against the YAGNI [1] principle

  • Wrong identification of the variability points

Empirical

  • Code becomes hard to understand and maintain

1. You Ain’t Gonna Need It

Middle Man

Causes

  1. Objects encapsulates (hides) details

  2. Encapsulation leads to delegation

  3. Sometimes, it goes to far!

Symptom

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

Diagram

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

Object-Orientation Timidness

I’m not taking too much OO, I’m on a diet!

Object-Orientation Timidness List

  • Data Classes

  • Feature Envy

  • Deeply Nested Code

  • Temporary Attributes

  • Refused Bequest

  • Alternative Classes with Different Interfaces

  • Utility Methods

Data Classes

Causes

  • Procedural programming influence, with procedures and records

Symptoms

  • Classes with attributes, getters and setters and nothing else

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;
    }
}

Data Classes — Problems

Conceptual

  • Breaks encapsulation

  • Increases coupling

  • Reduces cohesion

Empirical

  • The code is hard to understand and maintain

Diagram

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;
    }
}

Feature Envy — Problems

Conceptual

  • Increases coupling

Empirical

  • The code is hard to understand and maintain

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;
 }

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

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

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

Refused Bequest

Symptom

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

Diagram

Refused Bequest — Problems

Conceptual

  • Wrong inheritance hierarchy

  • Violates the Liskov substitution principle

Empirical

  • The code is hard to test

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

Diagram

Alternative Classes with Different Interfaces — Problems

Empirical

  • Duplicate code makes the code hard to maintain and understand

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);
  }
}

Utility Methods — Problems

Conceptual

  • Violates the single responsibility principle

  • Decreases cohesion

Empirical

  • Decreases testability

Naming

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

Type Embedded in Name

Symptoms

  • Methods that have the parameter’s type in their name

Examples
int priceInt = 5;

public void addCourse(Course c) {}
Problem

  • Affects maintainability:

    • the identifier (method, parameter) must be renamed if the type changes

Uncommunicative Name

Symptom

  • Uncommunicative identifier names

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

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

Conclusion

Summary

  • Code Smells are heuristics to detect signs of bad design

  • Smells are not errors and are not necessarily bad

More readings