A quick intro to code smells

Code Smell: An indicator of a design principle violation

What are code smells?
The term was popularized by Kent Beck in the late 90s and its usage increased after appearing in the well known Martin Fowler’s book Refactoring. Code smells are indicators that something may be wrong in a piece of code. They are not a problem just because they are a smell, a deeper analysis is needed to determine if there is a problem or not.

In order to clarify, let’s see what code smells are not?

• They are not bugs
• They are not technically incorrect
• They do not block a working program

Okay, this isn’t scary, but then why should I be worried at all? Well, let’s see what code smell actually are:

• Indicators of weaknesses in design
• Indicators of slow development
• Indicators of high risk of introducing new bugs and errors

Memorizing the list of code smells and understanding the complex idea behind each one of them, helps to reduce the conflicts within the development team regarding code discussions. There is no place anymore for arguments like: “I don’t like your code, but I can’t tell you why...”. They don’t miscommunicate or talk using ambiguous terms. When a person mentions the name of a smell, all the rest know exactly what it means and understand immediately the point of the discussion.

Let’s move to what we all came for, here is the list of code smells. They appear divided in 5 categories:

Bloaters: Things that grew too big

• Long Method: Too many lines of code in just one method.
• Large Class: Always putting in but never taking things out.
• Primitive Obsession: The objects passing around are too dumb. As everybody understands primitives, everyone can do its own interpretation.
• Long Parameter List: Use several parameters instead of an object with all the required data.
• Data Clumps: Pieces of data that always appear together and probably should become an object.

Abusers: Tools from OO that were misused

• Alternative Classes with Different Interfaces: Two classes that look different but perform the exact same operations.
• Refused Bequest: A class and a super class share a tiny part of logic, but the inheritance as a whole does not make sense at all.
• Switch Statements: Switch statements (or sequenced if statements) are a signal that polymorphism could be used instead.
• Temporary Field: An attribute that is only used as a part of a process or algorithm and never again.

Change Preventers: Things that makes changes hard and expensive

• Divergent Change: When the logic in a single place is so twisted that you have to change unrelated things to perform your changes. Opposite to shotgun surgery.
• Shotgun Surgery: When the logic is wide distributed and a modification requires small changes in many different places.
• Parallel Inheritance Hierarchies: Two mirrored hierarchies that need to change always at the same time.

Dispensables: Elements that could been avoided

• Comments: If comments are needed, something is not good. A good naming for the method and its parameters should be enough.
• Duplicate Code: Two fragments of code that are identical.
• Data Class: A class that is only a container of data with getters and setters and all the logic is placed in other classes.
• Dead Code: Anything that is no longer used in the code.
• Lazy Class: A class that does not get enough attention in a while, maybe has lost their right of being a class on its own.
• Speculative Generality: Anything that is built “for the future” or “just in case”, in very rare cases we will be right doing predictions of the future.

Couplers: Stuff that binds the objects together, therefore they cannot be used in any other context.

• Feature Envy: A method of a class that accesses the data of another object more than their own data.
• Inappropriate Intimacy: A class that uses private fields or methods of another class.
• Incomplete Library Class: When a library does not meet all the requirements anymore. Then the use of that library becomes painful. Fix it or throw it.
• Message Chains: A client requests something to an object, and this requests something to another object, and so on. If the type changes, everybody needs to adapt.
• Middle Man: When the only job of a class is delegate work or forward messages to other classes. Couldn’t it be avoided?

If you want to dig deeper in code smells, I leave here a link with much better explanations and code examples.

A code with smells is only a problem when you have to interact with it, but until that very moment, it is working software and it is not costing any money as long as it does not change. Think twice before refactoring something, most probably there is no need to clean all the smells of your code base, and certainly not all at once. Legacy code is perfectly valid and working software, beware if you decide to go into the mud.