Duplicated code

Description
Identical or very similar code exists in more than one location.

Example

public class SuperNumber {
  private int number;
  
  public void multiplyOne(SuperNumber multiplier) {
	number = number * multiplier.getNumber();
	recalculate();
	doSuperThing();
  }
  
  public void multiplyTwo(SuperNumber multiplier) {
	number = number * multiplier.getNumber()
	recalculate();
	doAnotherSuperThing();
  }
}

Duplicated code

What to do, if

• duplicated code is in two methods of same class?
  Extract Method.
• same expression is in sibling subclasses?
  Extract Method and Pull Up Field.
• code is similar but not same?
  Extract Method and use Form Template Method.
• duplicated code is two unrelated classes?
  Extract Class and reuse the extracted one.

Long method

Description
A method, function or procedure has grown too long.

Example

public class FooBar {
  
  public String reallyLongMethod(String value, String value2, String value3, int oopsAnInt, 
  			String value4, String value 5, String value6, int oopsAnotherInt, String value7,
			String value8, String nowItsEnough) {
	  String result = "Thats the beginning ...";
	  // now do something in hundreds of lines of code ...
	  return result;
  }
}

What to do?
In 99,9% of the time, just use Extract Method to shorten a method.

Large class

Description
A class that has grown to large. It's likely the class has become a “god object”, it knows too much or does to much (perhaps even both).

Example

public class NoExample {
  /* 
   * No example here, as the sheet isn't long enough, just think of one you have seen 
   * in the past. I'm quite sure, you have seen one.
   * If you can't remember one, just imagine this class here has 3000 more lines of code,
   * which aren't comments.
   *
   * A good example could be the class java.net.URI.
   */
}

Large class

What to do?

• Reduce redundancy, large classes tend to have duplicate code. How to eliminate this, have a look at the smell “Duplicated code” .
• If possible, use Extract Class.
• Often it's easier to use Extract Subclass.
• Perhaps it's even a good idea to add an interface while extracting classes or subclasses.

Long Parameter List

Description
A long list of parameters make a method or function error prone and hard to read.

Example

public class FooBar {
	
public String reallyManyParameters(String value, String value2, String value3, int oopsAnInt, 
            String value4, String value 5, String value6, int oopsAnotherInt, String value7,
            String value8, String value9, long heheYouDidntExpectThat, String value10,
            String nowItsEnough, String notYet, String oneMoreToCome) {
  String result = "Thats the beginning ...";
  // now do something ...
  return result;
  }
}

Long Parameter List

What to do?

• Sometimes it's also possible to get the needed values by calling a method. Use the Replace Parameter With Method pattern to accomplish this.
• If you get some parameters out of an object, provide the whole object, not just the values from it: Preserve Whole Object.
• If the parameters provided have no logical object, use Introduce Parameter Object.

Important exception: In rare cases you possibly don't want create a dependency from to the called to the larger object. In this cases it's reasonable to unpack the data and provide by parameters. But be aware of the pain you will suffer in the future when doing so.

Divergent Change

Description
A class is often changes in different ways for different reasons.

Example
In class CustomerData you have edit three methods every time you add a new role to your rights management. Every time you change your social media integration you have to alter four other methods in the very same class.

What to do?
This case is quite simple: use Extract Class, so every class is just responsible for one of the change causes.

Shotgun Surgery

Description
Shotgun surgery is similar to Divergent Change but the opposite. Every time you make a change, you have to make lots of little changes in serveral clases.

Example
You make a change to class Customer. As a result you have to adopt things in the classes SpecialCustomer, Reciept and Contact which forces changes in Address and SocialMedia.

What to do?
Use Move Method and Move Field to collect all changes in a single class. If you can't find any good candidate class create a new one. Often you can use a Inline Class to pull all the behaviour together.

Feature Envy

Description
A method is more interested in another class than the one it is in. It calls a load of getter methods of another class to calculate something.

Example

public class Invoice {
	
  public List<OrderItem> createOrderItemsList(Order order) {
	  List<OrderItem> orderItems = new LinkedList<OrderItem>();
	  
	  // ... some loop iterating over a list in oder
	    orderItem.setItemCallNumber(order.getCallNumber());
	    orderItem.setItemOrderDate(order.getOrderDate());
	    orderItem.setItemPrize(order.getItemByNumber(itemNumber).getPrize);
	    // and so on ...
	    orderItems.add(orderItem);
	  // ... end of the loop
	  return orderItems;
  }
}

Feature Envy

What to do?

• Use Move Method to put the suffering method to where it belongs to.
• Of course, a method normally doesn't only use data of one class. In this case it's a good heuristic to move the method to the class it uses most data from.
• Sometimes only parts of a method suffer from Feature Envy. Then use Extract Method and Move Method to get these part to it's right home.

Attention: Patterns like Strategy or Visitor (from the Gang of Four) break this smell. A fundamental rule of thumb is: Put together what changes together, this includes the behaviour that references the data. If behaviour and data would be separated, the behaviour will be moved to.

Data Clumps

Description
You see the same three or four data items together in lots of places: fields in a couple of classes, parameters in many method signatures.

What to do?

• Use Extract Class on the fields to turn the clumps into an object.
• Use Introduce Parameter Object or Preserve Whole Object to work on the method signatures.

Afterwards you can look for cases of Feature Envy which possibly results in some methods wandering into your new class.

Primitive Obsession

Description
No small classes for small entities exist. As a result the functionality was added to some other class.

Example

public class Customer {
  // some data ...	
  private int phoneNumberPrivate;
  private int cellPhoneNumberPrivate;
  private int phoneNumberWork;
  private int cellPhoneNumberWork;
  private int creditRating;
  
  public void setPhoneNumberPrivate(int phoneNumberPrivate) {
    this.phoneNumberPrivate = phoneNumberPrivate;
  }
  public int getPhoneNumberPrivate() {
    return this.phoneNumberPrivate;
  }
  // and so on ..
}

Primitive Obsession

What to do?

• Replace Data Value With Object to give the fields a better semantic.
• If the data value is a type code, use Replace Type Code With Class if the value does not affect behavior.
• If the type code has conditionals which depend on it, use Replace Type Code with Subclasses or Replace Type Code with State/Strategy.
• If you have a group of fields that should go together, use Extract Class.
• If you see these primitives in parameter lists, try a civilizing dose of Introduce Parameter Object.
• If you find yourself picking apart an array, use Replace Array with Object.

Switch Statements

Description
Switch Statements are the goto of object oriented languages. They are evil, you don't need them. Really.

Example

public class Customer {
  
  public List<PaymentOption> decidePaymentOptions() {
    switch (creditRating) {
	  case 1:
	    // do something
	    break;
	  case 2:
	    // do something
	  default:
	    // do something
	    break;
	}
	return paymentOptions;
  }	  
}

Switch Statements

What to do?

• Most times you find switch statements you should think about polymorphism. But spend a minute on where the polymorphism should occour. When the switch statement switches on a type code, you want it in the class or method that hosts the type code. Do the following:
  • Use Extract Method to extract the switch statement.
  • Then Move Method to get the new method to class is belongs to.
  • Decide whether to Replace Type Code with Subclasses or Replace Type Code with State/Strategy.
  • And at last you can use Replace Conditional With Polymorphism.
• If there are just a few cases that affect only a single class polymorphism is overkill. Then Replace Parameter with Explicit Methods is a good joice.
• If one of your conditional cases is a null, try
  Introduce Null Object.

Parallel Inheritance Hierarchies

Description
Every time you make a subclass of one class, you have to make a subclass of another one.

Example
You want to extend WVideoImpl. To get it working you also have to extend VideoImpl.

What to do?
Make sure that instances of one hierarchy refer to instances of the other. If you use Move Method and Move Field, the hierarchy on the referring class disappears.

Lazy Class

Description
Classes that don't do enough to pay for themself should be eliminated. The cost of maintaining them is higher than there benefit.

Example

public class StreetName {
  private String streetName;
  
  public void setStreetName(String streetName) {
    this.streetName = streetName;
  }
  
  public String getStreetName() {
    return this.streetName;
  }
}

Lazy Class

What to do?

• If you have subclasses that aren’t doing enough, try to use Collapse Hierarchy.
• Nearly useless components should be subjected to Inline Class.

Speculative Generality

Description
Speculative generality appears when some functionality was built in because some time in the future someone might need it. If only a test class uses a method or class (and it is no helper to test some legitimate functionality) you can be rather sure you smell speculative generality.

What to do?

• If you have abstract classes that aren’t doing much, use Collapse Hierarchy.
• Unnecessary delegation can be removed with Inline Class.
• Methods with unused parameters should be fixed with Remove Parameter.
• Methods named with odd abstract names should be treated with Rename Method.

Temporary Field

Description
Instance variables are only set under certain circumstances, e.g. during th execution of a complex algorithm in one method.

What to do?

• Use Extract Class to create a home for the poor orphan variables. Put all the code that concerns the variables into the new class.
• You may also be able to eliminate conditional code by using Introduce Null Object to create an alternative component for when the variables aren't valid.
• If the variables result from some algorithm, extract the method(s) and variable(s) with Extract Class. The new object is a method object.

Message Chains

Description
Imagine a client asks a object for another object. The last one is asked by the client for another one, wich again is asked for a other object. When stumbling over this you surely smell a message chain. It can manifest as a series of getThis() methods or a sequence of temporary fields.

Example

public class InvoicePrinter {
  public List<InvoiceLine> getInvoiceLines(Order order) {
	// some code  ...
	String productName = order.getCustomerOrder().getOrderList().get(i).getCustomerOrderItem()
	    .getProduct().getProductName();
	// some more code ...
    CustomerOrder customerOrder = order.getCustomerOrder();
	CustomerOrderItem customerOrderItem = customerOrder.getOrderList().get(i);
	Product product = customerOrderItem.getProduct();
	String productNumber = product.getOrderNumber();
	// more code ...
  }
}

Message Chains

What to do?

• Use Hide Delegate but be carefull not to turn the intermediate object into a middle man.
• Often it is better to have a look what the resulting object is used for. Then check if Extract Method is possible and you can, using Move Method, push the new method down the chain.
• Don't be to agressive in eliminating Message Chains.

Middle Man

Description
Delegation is a nice concept. But you can take to far. If you look at an class interface and half of all methods delegates the work to one other class you have a Middle Man problem. Why not directly talking to the other class?

What to do?

• Use Remove Middle Man to talk directly to the class which knows what's really going on.
• If only a few methods aren’t doing much, use Inline Method to inline them into the caller.
• If there is additional behavior, you can use Replace Delegation with Inheritance to turn the middle man into a subclass of the real object.

Inappropriate Intimacy

Description
Classes become far too intimate and spend too much time delving in each others’private parts.

What to do?

• Use Move Method and Move Field to separate the pieces to reduce the intimacy.
• See whether you can arrange a Change Bidirectional Association to Unidirectional.
• If the classes do have common interests, use Extract Class to put the commonality in a safe place.
• Or use Hide Delegate to let another class act as go-between.
• Subclasses are always going to know more about their parents than their parents would like them to know - apply
  Replace Delegation with Inheritance.

Alternative Classes with Different Interfaces

Description
Any methods that do the same thing should have the same signatures for what they do.

Example

public class Order {
  public Order getOrderById(int orderId) {
    // do something
    return order;
  }
}
public class Invoice {
  public Invoice getInvByIdentifactionNumber(int invoiceIdentification, boolean switch) {
	  // do something
	  return invoice;
  }
}

Alternative Classes with Different Interfaces

What to do?

• Use Rename Method so all methods doing the same, have the same signature.
• Often you recognize here classes don't do enough. Keep using Move Method to move behavior to the classes until the protocols are the same.
• If you have to redundantly move code to accomplish this, you may be able to use Extract Superclass.

Incomplete Library Class

Description
Working without libraries is nearly impossible nowadays. But the builders of libraries are not omniscient. Sometimes we miss a function we would like to be in the used library. Modifying a library is a no-go (if it is not your own).

What to do?

• If you just need some methods in the library class use Introduce Foreign Method.
• If there is a whole load of extra behavior, you need Introduce Local Extension.

Data Class

Description
Data classes are classes that just have field and setters for this fields, nothing else. They are dumb data holders and are almost certainly being manipulated in far too much detail by other classes.

Example

public class Beer {
  public String name;
  public int sizeInLiters;
  public int prize;
  
  public void setName(String name) {
    this.name = name;
  }
  public void setSizeInLiters(int sizeInLiters) {
    this.sizeInLiters = sizeInLiters;
  }
  // and so on ...
}

Data Class

What to do?

• In early stages these classes may have public fields. If so, you should immediately apply Encapsulate Field before anyone notices.
• If there are collections, check if they are properly encapsulated. If not apply Encapsulate Collection.
• On any field that should not be changed use Remove Setting Method.
• Check where the setters and getters are used. Often you can move behaviour into the data classes using Move Method. If it is just a part of the method you could move, apply Extract Method before.
• After a while you can start using Hide Method on the getter and setter methods.

Refused Bequest

Description
Subclasses get to inherit the methods and data of their parents. But what if they don’t want or need what they are given? They are given all these great gifts and pick just a few to play with.

What to do?

• Mostly the hierachy is wrong. Create a new sibling class and use Push Down Method and Push Down Field to push all the unused methods to the sibling. The parent only holds what is common. But pay attention: in 90% of the time the smell is too faint to clean it up.
• Whereas if the subclass is reusing behavior but does't want to support the interface of the superclass the smell is much stronger. In this leave the hierachy as it is, apply Replace Inheritance With Delegation.

Comments

Description
Comments itself aren't bad, they are not really a smell. But they are often used as a deodorant to cover smells (known as bad code). There is often some correlation between amount of comments and code quality: the more comments you see, the worse the code.

What to do

• If a comment explains what a block of code does, try Extract Method.
• If the method is already extracted but you need a comment to describe what the method does use Rename Method.
• If you need to state some rules about the state of the system use Introduce Assertion.
• Use comments to say why you did something not how.

Comments

References

• Fowler, Martin; Beck, Kent; Brant, John; Opdyke, William (1999): Refactoring. Improving the Design of Existing Code. Addison-Wesley. ISBN 0-201-48567-2
• http://sourcemaking.com/refactoring/bad-smells-in-code
• http://www.soberit.hut.fi/mmantyla/badcodesmellstaxonomy.htm
• http://de.wikipedia.org/wiki/Smell_(Programmierung)
• http://en.wikipedia.org/wiki/Code_smell