Sometimes our code has some characteristics that indicate it might have problems underneath. For example, excessive or unnecessary comments, ultra long method bodies or classes with no functionality (only data). This indications / symptoms are called Code Smells (they stink :)). There are lots of "catalogued" Code Smells around. The term was created by Kent Beck (the TDD guy) in the late 90's and got momentum after the publishing of a Martin Fowler's book called "Refactoring: Improving the Design of Existing Code" [Fowler, 1999].

Code Smells are not bugs, and don't point to bugs. In general they point design issues which lead to hard times on development, maintenance, etc. The advantage of cataloguing and studying Code Smells is that they can help in refactoring processes and in general learning of designing software. According to Martin Fowler, "One of the nice things about smells is that it's easy for inexperienced people to spot them, even if they don't know enough to evaluate if there's a real problem or to correct them" [BlogCodeSmells, Fowler]. This can be an incentive to increase interaction between experienced and beginners on a development team. Also, of course, cataloguing this symptoms creates a language to handle with the process of refactoring easier. A team can have a code revision that specifies explicitly which Code Smells should necessarily be searched in code. Also, tickets opened for the development team can just point the possible design flaw with the name of the Code Smell, without the need to explain what are the characteristics or the possible underlying problems, etc.

Next we'll see some code smells which I think are common to spot, or were a surprise to me when I first read about them.

Duplicated Code

Generally, it's pretty easy to spot duplicated code. And almost always it's not a good design decision. One can avoid code duplication by creating private methods that provide the duplicated functionality, or by using composition of an object that provides this functionality (maybe using the Strategy Pattern), or by using inheritance, etc. There are several ways to avoid duplicated code. And if you don't, expect to have lots of problems once you start changing or expading your software. Even more if you don't use unit tests. Remember the DRY (Don't Repeat Yourself) principle.

Dead Code

That's easy. Who never saw commented code in a software source code? Totally useless code, which should not be there. As there is no good reason not to use Version Control tools nowadays, all dead code should be removed from the source code. If it was an old version of something, it belongs to history, and if needed can be obtained through the Version Control tool.

Too Many Parameters

In good object oriented design, every method should be small. If a method receives several parameters, in general, it has a big body. Switch cases, lots of if-elses also indicates the same thing. When using OO languagens, you should also prefer inheritance and polimorphism to take place, and create a structure of classes that through virtual methods you execute exactly what you need. This is one of the options (using objects to separate designed functionality per group of parameters). The Parameter Object and Query Object patterns, for example, can be used for that. The other option is to revise the responsibilities of the class and method and refactor them.

Inappropriate Intimacy

Classes should depend the minimum from each other (single responsability principle, low coupling principle, etc). When a class depends a lot on the implementation of another, we have a "inappropriate intymacy". The most common case of this is when we see a method which should not be public being used by an object to use some internal object that composes the first. For example:

[code language="java"]
public Integer numberOfBooksInInventory(Inventory inventory) {
return inventory.getBooks().count();
}
[/code]

In the code above, our class which contains this method is acessing an internal collection of books of the Inventory class, and using the method count() of it. This creates a dependency of Inventory and Book with our class. If the class Inventory changes internally, it will crash our class code. So we're not using encapsulation correctly, and our class has "inappropriate intimacy" with Inventory.

Consider this worse cenario:

[code language="java"]
public Set getBooksWithLowStock(Inventory inventory) {
Set booksLowInStock = new HashSet();
for (Book book : inventory.getBooks()) {
if (book.getInStock() <= LOW_STOCK_LIMIT) {
booksLowInStock.add(book);
}
}
return booksLowInStock ;
}
[/code]

In this case, it is yet more clear that our class is "invading" the Inventory class, and the coupling between them is high. To fix these problems (in both examples), the Inventory class must absorb both functionalities (providing number of books in the inventory and the books with low stock), and provide public methods, hiding access to it's internal Book collection from external objects.

Other Code Smells

There are several other Code Smells around. A famous book that contains a catalog of them is [Refactoring, 1999], which is the book that made the term become famous. Another book that was recommended to me is "Refactoring to Patterns" [Kerievsky, 2004]. A good online list can be found in [SiteCodeHorror].

----

[Fowler, 1999] Fowler, Martin (1999). Refactoring. Improving the Design of Existing Code. Addison-Wesley. ISBN 0-201-48567-2.
[BlogCodeSmells, Fowler] CodeSmells, http://martinfowler.com/bliki/CodeSmell.html.
[SiteCodeHorror] Code Smells, http://blog.codinghorror.com/code-smells.