What are Code Smells ?

What are Code Smells ?

The notion describing the “when” we should apply refactoring techniques in our code.
- Kent Beck

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

In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells are usually not bugs—they are not technically incorrect and don’t currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future.
- Wikipedia

Code smells are screams from our code trying to tell us that we should stop and think better in what we’re doing.
- Myself

Code smells can ruin our project

  • Code smells will slow down the team.
  • Code smells will increase the risk of bugs.
  • Code smells will make the code more and more complex.
  • Code smells will make harder to add new programmers in the project.
  • Code smells will make more difficulty to programmers to make changes and consequently generate value for our clients.
  • Code smells, sometimes, can make the project be cancelled.

How can I discover a code smell ?

Several programmers and writers have been talking about code smells since 90s when Kent Beck invented this concept. A lot of lists were created to catalog and be used as guidelines to fight against code smells in our projects. Below I made my list with the worst code smells in my opinion.

Duplicated Code

It’s probably the most seen code smell. If you see the same code block twice, it’s already a signal that you should stop and do a refactoring.

If the code is been repeated in the same class we should extract it in a method.
If the code is been repeated in different classes, maybe we are a missing a opportunity to create a super/base class. Or sometimes, if we analyze better ,the code belongs to only one class and the other should be calling a method in the former class.

Commented Code

Sounds funny but some programmers looks like they don’t trust their source code control system. I have seen a lot of comments like this:

// Some commented code
// This code was commented due a random reason - 05/10/2005 - Programmer 1

Delete this commented code, your source control won’t let you down!

Long Methods/Long classes

Sometimes we see code blocks known as Megazords or Gods, that are pieces of code with too much responsabilities. It’s a transgression of one of the SOLID principles – Single Responsability Principle.

Single responsibility principle
a code block should have only a single responsibility.

Maybe methods that are doing a lot of things eg. retrieving data from a source, processing and writing it in a console. It should be clearly splitted in 3 methods. Or maybe classes that are holding behaviors and characteristics that should be in another classes and so on. When writing a solution, a good signal is to look for comments, normally a code where you wrote a comment should be replaced by another function. When designing classes we should look for duplicated code, large methods and bad design.

Divergent Change / Shotgun Surgery

Software will change, we know it. So we have to design our code to make it easy to change. We want a single point of change and for a single reason.

Divergent Change is the code smell that happens when a piece of code is changed for various reasons. If you look for a piece of code and thinks, “I will change it if my database changes, if my business rules changes and if my view rules changes, your code has this bad smell! The solution here is to identify different responsabilities and extract them to different pieces of code.

If when we have a code changing for different reasons we have a Divergent Change, the opposite is also a smell. Shotgun Surgery is a code smell that happens when, for a single reason, you have to change several pieces of code. For example, when performing a database change, you shouldn’t change your business rules or your views rules. If when we have a Divergent Change we should split responsabilities, when we have a Shotgun Surgery we should join responsabilities, in this case, database code shouldn’t belongs to business or views classes.

Bad Names

Names in software are 90 percent of what make software readable. You need to take the time to choose them wisely and keep
them relevant. Names are too important to treat carelessly.
- Robert C. Martin aka Uncle Bob

Names plays a important role in software development. We spent more time reading code than writing code so we should care about our code readability, we should care about the names that we are giving to our variables, functions, classes and so on. Below are some name guidelines that we can use to achieve a better code.

Choose descriptive names. Look at this code below. What it does ? What is the software domain ? We know nothing about it. For me, it’s only a bunch of non-sense code.

public String getWinner(String p1, String p2) {
    if (p1 == 'r' && p2 == 's') {
        return 'r';
    }

    if (p1 == 's' && p2 == 'p') {
        return 's';
    }

    if (p1 == 'p' && p2 == 'r') {
        return 'p'
    }

    return 'd'
}

Now look at this code below. It’s exactly the same code but with better names. Now, we know what the code does and it’s easier to evolve and maintain.

/*
Changed the code to adopt best practices - 07/07/2013.
*/

public enum PlaysTypes
{
    ROCK,PAPER, SCISSOR;
}

public enum WinnersTypes
{
    ROCK,PAPER, SCISSOR, DRAW;   
}      

public WinnersTypes judgeWinner(PlaysTypes play1, PlaysTypes play2) {
    if (play1 == PlaysTypes.ROCK && play2 == PlaysTypes.SCISSOR) {
        return WinnersTypes.ROCK;
    }
    
    if (play1 == PlaysTypes.SCISSOR && play2 == PlaysTypes.PAPER) {
        return WinnersTypes.SCISSOR;
    }
    
    if (play1 == PlaysTypes.PAPER && play2 == PlaysTypes.ROCK) {
        return WinnersTypes.PAPER;
    }
            
    return WinnersTypes.DRAW;
}

Use standard names when possible. It’s easier to understand names that are based in conventions or standards. If you are using a MVC architecture, we should name our controllers in this way xxxController. If we are applying some design pattern, is good to give names like xxxxSingleton or xxxAdapter.
Your own project should have some standards. Let’s respect them.

Avoid encoding and Hungarian Notation. Once I was working in a project where we should use the following convention:

L for local variables
G for global variables
P for parameters
str for string
int for int
and so on.

We ended up with these terrible names.

Lstr_name
Gint_session_id
Pint_age

Today’s environments provide us all the info that we need. We can rely on them.

Names should describe everything that a code does. Don’t use simple names for functions that do more than one thing. Look at the code below.

public Connection getConnection() {
    if (connection == null) {
        connection = new Connection();
    }

    return connection;
}

This function does more than only get a connection. If connection is null, it also creates a connection. A better name could be createOrReturnConnection.

Code Smell’s Lists

Other people also shared their smells list. You can check out the lists below.

Jeff Atwood’s List
Cunningham’s List

Books that cover Code Smells and Code Refactoring

Refactoring: Improving the Design of Existing Code
Clean Code: A Handbook of Agile Software Craftsmanship

7 Comments

  1. Comment by Doh..:

    Speaking of code smell… how about using private static final String DRAW = “Draw”, etc. instead of enums? FFS, enums were introduced in 2004…

  2. Comment by madmaurice:

    You still have to change every use of ‘r’ ‘s’ ‘p’ and ‘d’ everywhere else in your code to ClassName.ROCK etc. Using enums wouldn’t be more effort and prevents more problems like your string comparisons with == . Example: getWinner(“Rock”,”Paper”) would return “Draw” as Java checks the addresses (String is a class so java uses pointers for that) with == and not the content. For content comparison use the equals method.

    Second: If you still insist on static variables then please use int. It is a base type (not a class and therefor no pointer) and its contents can be compared with ==.

    Third: Your judgeWinner-Method has a parameter with a type PlaysTypes. I don’t see a definition and i don’t think is can be usefully compared with String.

    Last but not least: i think getConnection is a fitting name for that method. what the method does is called lazy fetching. if your developer team has to change that to eager fetching for some kind of reason, you won’t have to rename that method everywhere it was used. Also getConnection is the typical getter naming pattern.

  3. Comment by paulo ortins:

    1, 2 and 3 – I updated the post to use enumerations.

    4. If my team has to change I can change the method name. The method name should describe what the method does. In this case createOrReturnConnection describe perfectly the method implementation.

  4. Comment by madmaurice:

    So you name your setters “putThatThingInAnAttribute”? getConnection already describes what it does: It “gets” you a connection. most of the time its irrelevant “how” it does that. getters aren’t limited to “just” return an attribute. often they calculate values using other attributes or just return a constant value.

    Consider the following you have a builder/factory in you program and want to limit the objects created cause most of them are equal and immutable, you write a decorator which uses an object pool to do that for you. suddenly the method doesn’t just “create” objects it also caches them. okay lets rename that to “createAndCache”. whoops that breaks the decorator pattern.

    you can still use javadoc or similar to describe how the method does something, but a method name should be what it does and not how it does it.

  5. Comment by Folgor:

    There is nothing wrong with smells. One of nicest objects in the universe is sometimes accused of smelling like dead fish. Those who appreciate the item understand that it is more like caviar.

Leave a Reply

Your email address will not be published. Required fields are marked *