3 Code Smells

"A code smell

is a surface indication that usually corresponds to a deeper problem in the system." -- Martin Fowler

Providing names to different types of ugly code enables more effective discussions on design improvement.

Duplicate Behavior ("Duplicate Code")

Then you need to make even more changes... and your code base grows. Oh, and you did copy-paste for a hundred other routines.

• You find yourself in a never-ending cycle of updating little spots of code all over the codebase.

• The routines get complex and difficult to understand.

• You can't be reassigned to a new project since you're the only one who understands the code.

Don’t Repeat Yourself (DRY)

• Composition or Inheritance

• parameters or polymorphism

Solution to Duplicate Code:

Write code in a method just once, then call it where you need it.

• Use — to reduce duplication

• Use — to handle variations.

When you need to update your logic, you only need to update and test one spot.

Code is easier to read since it is broken up into descriptive methods.

Long Methods & Large Classes

Class has a lot of fields.

Difficult to understand, maintain, reuse.

OOP is about small, specialized components.

multiple smaller methods.

Long Methods & Large Classes

Each method should only do one, simple thing.

• No side-effects.

• Method name should be descriptive of that one thing.

• If a method is hard to name, it's probably doing to much.

A class should be a specialized, cohesive unit.

• Just a few fields.

• If the class is difficult to name, or has a generic name, it might be doing too much (or not enough).

Solutions:

Break up a long method into —

Break up a large class into two or more classes.

Primitive Obsession:

Sticking with built-in types of the language instead of creating one's own classes.

Primitive Obsession: Sticking with built-in types of the language instead of creating one's own classes.

   int customerId1;

   String customerFirstName1;

   String customerLastName1;

   int[] orderIdsOfCustomer1;

   int customerId2;

   String customerFirstName2;

   String customerLastName2;

   int[] orderIdsOfCustomer2;

Data Clump:

Data that's often found together.

   int customerId;

   String customerFirstName;

   String customerLastName;

   String customerEmailAddress;

   int[] orderIdsOfCustomer;

   Date[] orderDates;

Long Parameter Lists

void method(int customerId, int customerId, String customerFirstName,

           String customerLastName, String customerEmailAddress,

           int[] orderIdsOfCustomer, Date[] orderDates) {

• form fields, etc.

What happens as the data clump changes?

• Additional and removal of —

Maintenance nightmare!

Cohesion

Solution to Primitive Obsession, Data Clumps & Long Parameter Lists

Encapsulate a data clump into a meaningful class.

class Customer {

   private final int id;

   private String firstName;

   private String lastName;

   private String emailAddress;

   private Set<Order> orders;

   ...

}

No more data clumps.

Customer customer1;

Customer customer2;

Customer customer3;

No more long parameter lists.

void method(Customer customer) {

Long parameter lists

can also be a sign that the method is in the wrong class.

• It always requires data from somewhere else.

OOP is about combining data structures with the operations that act on them.

Solution: Move the method to the class where most or all of the data it uses is found.

Divergent Change

principle of Separation of Concerns.

The same class is changed for different reasons.

"Well, I will have to change these three methods every time I get a new database and I have to change these four methods every time there is a new financial instrument..

This class is handling both business and persistence concerns.

A class is suffering from it if it breaks the —

one concern.

Solution to Divergent Change

Similar to large classes, you should break up the class so that it only does —

• Examples of concerns: business logic, persistence, presentation, transactions, security...

Shotgun Surgery

When multiple classes need to be modified to do one change.

• Opposite of Divergent Change.

• Caused by a lack of Cohesiveness - the many separate bits should be in one class.

Cohesion

Solution to Shotgun Surgery:

— Encapsulate the things that change together into one class.

"Things that change together should be together."

Also applies to packages - Classes that change together should be in the same package.

Feature Envy

Move the method

A method keeps accessing data from another class.

Solution: — to the class where the data it uses resides.

• Again - OOP is combining data with the methods that act on that data

Conditional Complexity

Large and growing If-Else and Switch logic become difficult to maintain.

polymorphism

Solution to Conditional Complexity

Use —.

• Separate the common logic from the special case logic.

• Create a supertype (abstract class or interface) as placeholder for special case logic.

• Create subtypes to hold logic for each special case.

Parallel Inheritance Hierarchy

Every time you add a class to one package, you have to add a class to another package

• Customer → CustomerService

• Order → OrderService

• Product → ProductService
  

Solution: Move code from one hierarchy to the other, and then collapse what's left into fewer classes.

Lazy Class

Middle Man

Dead Code:

— Doesn't do much to justify its existence.
— Delegates to another class but doesn't add its own value.

— Code no longer used.

Inline

Combine

Delete

Any code that exists takes maintenance cost. Eliminate code that doesn't justify its existence.

Lazy Class: — its behavior into other classes.

Middle Man: — the Middle Man with the class to which it delegates.

Dead Code: — it.

Alternative Classes with Different Interfaces

Classes that are similar but are not substitutable, since they do not share the same supertype.

Choices have to be hard-coded, or messy conditionals need to be used.

What if we have to support other forms of data access (web services?) later on?

Polymorphism

Solution to Alternative Classes with Different Interfaces

Solution: —- Have the classes share a common supertype.

Data Class or Anemic Domain Model

Class that only contains data and not behavior.

public class BankAccount {

   private int acctNo;

   private BigDecimal balance;

   public void setAcctNo(int acctNo) {

       this.acctNo = acctNo;

   }

   public int getAcctNo() {

       return acctNo;

   }

   public void setBalance(BigDecimal balance) {

       this.balance = balance;

   }

   public BigDecimal getBalance() {

       return balance;

outside the object

invalid state

Data Class or Anemic Domain Model

• Balance should not be negative.

• ATM withdrawals should not exceed 20,000.

• All transactions must be logged with a transaction ID.
  

In an Anemic Domain Model, all this behavior needs to be done — Error-prone.

Object may be in an — (ex. negative balance). Constant validation must be done.

Fields can be changed by any code. Encapsulation is broken.

Rich Domain Model and Information Expert.

Solution to Data Class or Anemic Domain Model

Solution: Implement — Place methods in the class with the data they operate on.

• Remove unneeded getters and setters, to preserve encapsulation.

public class BankAccount {

   private final int acctNo;

   private BigDecimal balance;

   public BankAccount(int acctNo) {...}

   public Transaction deposit(BigDecimal amt) {

       if (amt.signum() < 0) {

           throw new IllegalArgumentException("cannot be negative");

       }

   ...

   }

...

Refused Bequest

Replace inheritance with composition.

When a class inherits members that it doesn't need.

• Ok if the unneeded members are encapsulated.

• Dangerous if the unneeded members are part of the interface.

Solution: —

Speculative Generality

When code is written to be more flexible than the present requirements need it to be.

Usually in anticipation of future requirements.

• any data type

• Overloaded

Speculative Generality

• Methods that try to accommodate —

• ex. String or int instead of enum, LocalDate, BigDecimal or custom class

• — methods/constructors that are not or rarely used.

• Methods that don't do much, and don't add to clarity.

• Parameters that aren't used.

• Supertypes that only have one subtype.

• Middle Man, Lazy Class, Dead Code

Code is hard to understand and maintain.

YAGNI - You Ain't Gonna Need It

DTSTTCPW - Do the Simplest Thing That Could Possibly Work

Solution to Speculative Generality

Code just enough to satisfy the requirements of the current Sprint.

—

Requirements change. Even if the written requirements state that something is required, that might change later, and any effort you do to fulfilling those requirements will be wasted.

Focus on the more certain requirements - those in the current Sprint.

• Code is simpler, more readable, less likely to be buggy, gets done on-time.

Collapse unneeded hierarchies, inline methods and classes, remove unused parameters, etc.

God Class / God Method

Much of the code is concentrated in one class or method, or many other classes or methods depend on that one class or method.

• Single Responsibility Principle

• Information Expert

• Law of Demeter

Solution to God Class / God Method: (3)

Temporary Field

Class or instance fields that are temporarily set for method operations. They don't describe the state of the object.

private int x;

void method() {

   x = 0;

... // do stuff with x

}

Dangerous in multithreaded environment! Multiple threads may be modifying the field in the middle of method executions.

Confusing for other teammates (or your future self) since it's hard to figure out what the field is for.

• local variable/s.

Solution to Temporary Field

• Make the field/s —

void method() {

   int x = 0;

... // do stuff with x

}

• If there are several of them and they end up getting passed around together as parameters, encapsulate them in a new class.

Message Chains or Train Wrecks

Reaching into an object's attributes to call the attributes' methods.

currSection.getSchedule().equals(newSection.getSchedule())

Causes tight coupling. The calling class becomes tightly coupled to the internal structure of the object. The object cannot change without breaking the caller.

Message Chains or Train Wrecks

Not to mention being difficult to read.

urrSection.getSchedule().getDays().equals(

newSection.getSchedule().getDays()) &&

(currSection.getSchedule().getStartTime().compareTo(

newSection.getSchedule().getEndTime()) >= 0 ||

currSection.getSchedule().getEndTime().compareTo(

newSection.getSchedule().getStartTime()) <=0)

Prone to error

Difficult to maintain.

Information Expert / Law of Demeter

Solution to Message Chains or Train Wrecks

Apply —

• Don't get the fields. Create a method that does the work and just get the results.

currSection.hasConflict(newSection)

No tight coupling. Internal structure of Section can change without breaking callers.

Easier to read & maintain.

different objects.

Do not confuse Message Chain with Method Chain / Fluent Interface

Method Chains / Fluent Interfaces return the same instance, except possibly for the last call. Message Chains return —

"How" Comments

If you need a comment to understand how some code works, then the code is not readable:

// start of fund transfer process

// end of fund transfer process

// check if section schedule has conflict with existing section

"Why" comments

readable

— are good:

// This method uses encryption library Xxx to comply with US regulations on...

// This field is immutable in order to make it threadsafe.

// This private no-arg constructor is to allow JPA to set the final fields.

Javadoc comments that state the contract that overridden methods need to keep are also good.

See equals method Javadoc.

"How" Comments

Solution: Make the code itself —.

Flag Arguments

Having a method do different operations based on a Boolean parameter.

public void login (

   Credentials creds, bool isAdmin

) {

   if (isAdmin) {...}

   else {...}

}

Doing this makes programmers try to remember what the Boolean parameters was for.

Boolean more obvious.

Solution to Flag Arguments

Solution: Make the meaning of the —