Click here to monitor SSC
  • Av rating:
  • Total votes: 35
  • Total comments: 8
Nick Harrison

Exploring Smelly Code

27 April 2009

Bad Code Smells are similar in concept to Development-level Antipatterns. They don't describe bad programming aesthetics and you can't sniff them out precisely with code metrics. They describe code in need of refactoring in rich language such as 'Speculative Generality', 'Inappropriate Intimacy' or 'shotgun surgery'. They're useful because they give us words to describe antipatterns that we all come across in code. Nick Harrison explains...

Exploring Smelly Code

What is Smelly Code?

“Smelly Code” is code in need of refactoring.  Kent Beck quotes his grandmother “If it smells bad, change it.”  There are many ways that code can smell bad.  Most of these have been categorized as code smells with associated refactors that can resolve the problems with the code that gave the foul smell.

This provides another way to identify problems in code and associate specific problems with common solutions.

A Brief Survey of Smells

Some smells are easy to recognize and relatively easy to resolve.  Other smells are much more subtle and can be difficult to resolve.  Some smells are even controversial and not always accepted as even causing a code problem.

It is rather easy to identify code that smells of “Long Method”.  In fact, we have seen how metrics can be applied to automate pinpointing code with this smell.  On the controversial side, can code smell of comments?  Does having too many comments imply bad code?  Sometimes yes!

With meaningful variable names and meaningful method names, there should be no need to have comments explain what your code is doing.  Comments should strictly help explain why your code is doing what it is doing.  As you look through your code, note the sections that are heavily commented.  Chances are, you can eliminate some of these comments by renaming methods or extracting code to create new methods.  The original comment may even provide guidance for the new method’s name.

In this article, we will review three of the most common smells found in application code.  We will review what the smell looks like, why we care about the smell, and what we can do about it.

Shotgun Surgery

Shotgun Surgery pops up when you have to make changes throughout the code base to implement a single requirement.  This may often be caused by “copy and paste” programming.  This may also be caused by not properly leveraging inheritance or not recognizing when related classes could have a common base class.

Whatever the cause, there is one common solution.  Identify the code that has to be changed.  Move this code to a common location, and change every reference to call the new centralized implementation.

What Does It Look Like

A common example might be the steps needed for object initialization.  Let’s consider the not so humble connection object.  Typically, there are several steps needed to initialize the connection object.  These steps are also subject to periodic changes but will most likely be the same throughout the application.  Unfortunately, this commonality is not often properly leveraged.

A typical data access implementation may often include code similar to this:

public SqlDataReader GetData()

{

    SqlConnection con = new SqlConnection(ConnectionString);

    SqlCommand cmd = con.CreateCommand();

    con.Open();

    cmd.CommandType = CommandType.StoredProcedure;

    cmd.CommandText = "spSampleStoredProc";

    return cmd.ExecuteReader(CommandBehavior.CloseConnection);

}

 

The first four lines of code will be the same if every “GetData” type method.  Shotgun surgery comes into play if we want to change this logic.  Suppose you get a new requirement to explicitly set the ConnectionTimeout or the CommandTimeOut or there is a new best practice for initializing these objects.  To implement such a new requirement, every “GetData” type method will have to be modified.

This may seem like an obvious mistake that no one would ever make, but it does happen.  Most of the time, this problem is much more subtle. 

Consider a logging strategy implemented in every class performing logging operations, or any object with multiple steps to initialize.  We will also see shotgun surgery rear its head in the examples we review a little later.

Why Do We Care?

If you have ever had to update such code you know how time consuming it can be to make such changes.  If you have never had to make such changes, consider yourself lucky. 

If takes time to locate every place that needs to be changed. It takes extra testing time to ensure that every places was properly updated, and following such a pattern makes it that much more difficult to write the original code.

What Do We Do About It?

A much better solution is to move the redundant code to a common function and that replace the multiple instances with references to the new function.  This is the basis for the Factory pattern.

The example shown earlier could be rewritten as:

private SqlCommand PrepareCommand(string storedProcedureName)

{

    SqlConnection con = new SqlConnection(ConnectionString);

    SqlCommand cmd = con.CreateCommand();

    con.Open();

    cmd.CommandType = CommandType.StoredProcedure;

    cmd.CommandText = storedProcedureName;

    return cmd;

}

 

public SqlDataReader GetData()

{

    SqlCommand cmd = PrepareCommand("spSampleStoredProc");

    return cmd.ExecuteReader(CommandBehavior.CloseConnection);

}

 

The common approach is to use the ExtractMethod refactor to create a new method with the shotgun code and then update every method that duplicated the shotgun code to call the new method

Feature Envy

Feature envy starts smelling when methods in an object use the methods or properties of another object more than its own methods and properties.   We say that this method is envious of the features in the other object.  This is not always a bad thing.  Sometimes, the object is really acting as an adapter to a legacy object, or perhaps the object is implementing a decorator pattern. 

These implementations do not imply the feature envy smell.  Adapter and decorator are legitimate patterns to solve underlying design problems.  Feature Envy is a smell that points to a new design flaw in its own right.

What Does It Look Like?

There are several examples of Feature Envy that are worth exploring.  Consider a “Customer” class that exposes a mailing Address property.  If the details for the address are stored in an Address object, then the implementation for the mailing address might be similar to this:

private Address currentAddress = null;

public string MailingAddress()

{

    StringBuilder sb = new StringBuilder ();

    sb.Append(currentAddress.AddressLine1);

    sb.Append("\n");

    sb.Append(currentAddress.City + ", " + currentAddress.State);

    sb.Append("\n");

    sb.Append(currentAddress.PostalCode);

    return sb.ToString();

}

 

Formatting the mailing address could and should be in the address class.  Having this logic in the Customer class makes the main logic in the Customer class harder to follow.

Other examples may include a ShoppingCart object that calculates the price for each Item in the cart instead of delegating that to the Item iteself. 

Even more dangerous, instead of a well defined class like Customer being envious of the properties in Address, imagine multiple web pages being envious of the Address details.

A simplistic way to identify potentially envious code is to track any methods in an object that make references to another object above a specified threshold.  For example, if a single method in the Logger class accesses more than five properties in the Borrower object, then there is probably a method embedded there that is just waiting to be moved to the Borrower object.

Why Do We Care?

You may look at this code above and not see a problem.  You may even be able to write such code without any problems.  However, eventually, such coding practices will create a maintenance nightmare.  There are two main problems.

Polluting an unsuspecting object with logic that better belongs in another object hides the true purpose of our unsuspecting object.  In our earlier examples, the ShoppingCart’s main task is not to calculate the price for every item in the cart.  This logic only detracts from the main purpose and violates the cohesion principle.  Because our unsuspecting object includes functionality that would be better housed somewhere else, it is not very cohesive.  When you are trying to maintain the code, you may have difficulty finding the logic since it is not where you would expect to find it.

There is also the potential problem with duplicated code.  If a particular piece of logic is not where it is supposed to be, there is a good chance that it might be repeated in several wrong places.  If you ever need to change this functionality, not only will you have to search to find the code, you also have to ensure that you have found everywhere that needs to be updated.   Sound familiar?  This is one way that Shotgun Surgery starts stinking up our code.

What Do We Do About It?

We rely on the MoveMethod refactor and potential ExtractMethod as well to resolve these types of issues. 

The address example above could be rewritten as:

    public  class Address

    {

        public override string ToString()

        {

            StringBuilder sb = new StringBuilder();

            sb.Append(AddressLine1);

            sb.Append("\n");

            sb.Append(City + ", " + State);

            sb.Append("\n");

            sb.Append(PostalCode);

            return sb.ToString();

 

        }

     . . .

    }

And in the Customer class, the MailingAddress property can be reduced to:

       private Address currentAddress = null;

        public string MailingAddress()

        {

            return currentAddress.ToString() ;

        }

Not only does this make the Customer class more highly cohesive and add more responsibility to the Address class, we are also shielded from the details of how the address is stored and how to properly format it.  The Address object can now handle the details of formatting the address properly for international addresses or any other future requirements without requiring Customer and any other objects that need to render an address to get bogged down in these details.

Smelly Switches

Smelly switches refer to code using a switch statement to decide which of a series of potential actions to take.  If the same switch statement is repeated in multiple places, at the very least we have duplicated code.  At the very worse, we have a maintenance nightmare.  Not all switch statements are problems, but they are all worth looking into to make sure that a better design is not waiting to come out.

What Does It Look Like?

When you look over your code base and find the same switch statement repeated, you may have code that smells of switch statements on your hands. 

Code snippets like this hint that you have a switch smell in your code:

         switch (state)

        {

            case "SC":

                {

                    // Process Broker

                    break;

                }

            case "AL":

                {

                    // Process Retailer

                    break;

                }

            case "GA":

                {

                    // Process Supplier

                    break;

                }

            case "KY":

                {

                    // Process Transport

                    break;

                }

        }

 Now if you have such a switch statement only once, then it is probably not an issue, but if you have this same, switch statement in multiple places, then you might have more of a problem that needs to be addressed.  If the individual case statements are likely to change, refactoring to eliminate the switch statement will definitely pay off in the long run.

Why Do We Care

You may be asking yourself, “What is wrong with this”?  Or you may even be thinking “I do this all the time!  I have never had any problems with it!”   “A switch statement is so much simpler than a collection of if statements!”  All of this may be true.

 The problem is not comparing if statements with switch statements.  The problem lies in the amount of work needed to unwind the business logic embedded in the complex conditionals. 

Object oriented purists will point out all that has to change if you add a new state.  Consider the problems that can arise with the state specific business logic spread throughout the system.  When it is time to add a new state, you will have to search through the code base to find every switch statement, and update them individually.  This sounds a little like shotgun surgery again only on a larger scale.

If the switch statement has only a handful of case statements and the case statements are not likely to change, the complexity of the method will be manageable.  But as the number of cases statements grow, the complexity can quickly get out of hand and the overhead of making the updates system wide can be not only a maintenance nightmare but also a deployment nightmare.

What Do We Do About It

We can often eliminate these switch statements with an object hierarchy.

For our state specific business logic example, we can build an object hierarchy implementing a strategy pattern.  Every method needing state specific logic would have a corresponding method in the object hierarchy.  Instead of duplicating the switch statement everywhere, we use a factory to return the appropriate object and then simply call the corresponding method from the returned object.

Our object hierarchy might look similar to this:

The key thing is for all of our objects to have a common ancestor.  The inheritance depth can be to whatever level makes sense for your application.  If two states have a common implementation, let them share a common ancestor and put that common implementation in the ancestor class, but there is no need to create a more complicated structure than is necessary. 

A method such as this:

    public double CalculateSalesTax  (string state, double price)

    {

        switch (state)

        {

            case "SC":

                {

                    return price * SCTAX ;

                }

            case "AL":

                {

                    return price * ALTAX ;

                }

            case "GA":

                {

                    return price * GATAX ;

                }

            case "KY":

                {

                    return price * KYTAX;

                }

        }

    }

 

Can be reduced to:

    public double CalculateSalesTax(string state, double price)

    {

        StateBase activeState = Factory.CreateState(state);

        return activeState.CalculateSalesTax(price);

    } 

 

This simple method can handle invoking the state specific logic regardless of how many states are supported.  Each state object is easy to implement because it has to worry about how to take care of that one state.

The individual state objects can also benefit from inheritance making their implementation even easier.  As new states are added, we don’t have to search through the codebase for what needs to be changed.  We need to simply implement the new state object.   Depending on how the factory is implemented; we may not have to change any existing code, simply deploy a new assembly with the new state object.

Conclusion

Sometimes bad things happen to good code.  Even the best code can start to smell if not carefully monitored.  Fortunately, we can still intervene and save our code even after it starts to smell.

You can get more information on the basic Refactors here: http://www.refactoring.com/catalog/index.html

A good catalog for the code smells can be found here:  http://www.soberit.hut.fi/mmantyla/BadCodeSmellsTaxonomy.htm

Nick Harrison

Author profile:

Nick Harrison is a Software Architect and .NET advocate in Columbia, SC. Nick has over 14 years experience in software developing, starting with Unix system programming and then progressing to the DotNet platform. You can read his blog as www.geekswithblogs.net/nharrison

Search for other articles by Nick Harrison

Rate this article:   Avg rating: from a total of 35 votes.


Poor

OK

Good

Great

Must read
Have Your Say
Do you have an opinion on this article? Then add your comment below:
You must be logged in to post to this forum

Click here to log in.


Subject: Great Article
Posted by: Brent (view profile)
Posted on: Wednesday, April 29, 2009 at 11:00 AM
Message: Really great article Nick. I especially liked the last example but i'm going to suggest a way to improve it would would save a lot of work in the future. It appears your your coding directly off the state object which i would not do. I believe you should be using an interface. For example create a IStateCalcTax interface. That will help eliminate making breaking changes in the future otherwise very good article.

Subject: Great Article
Posted by: Brent (view profile)
Posted on: Wednesday, April 29, 2009 at 12:47 PM
Message: Really great article Nick. I especially liked the last example but i'm going to suggest a way to improve it would would save a lot of work in the future. It appears your your coding directly off the state object which i would not do. I believe you should be using an interface. For example create a IStateCalcTax interface. That will help eliminate making breaking changes in the future otherwise very good article.

Subject: Sometimes the cure is worse than the disease
Posted by: BuggyFunBunny (view profile)
Posted on: Thursday, April 30, 2009 at 3:20 PM
Message: Long Method Smell.
Too often, naive OO coders adopt some arbitrary metric (variable count, line count, etc.) and reduce all methods to that count. BAD. I had a professor who, when asked how long a paper should be (we all wanted to know how much typing we needed to do, of course), replied: "organic length". Same answer to how long should a method be. What I see happen far too often is a class being split into 10's of methods, where the methods are called by only one other method. In order to code with this class, the coder needs to keep both the method list and their sequence diagram IN HIS HEAD while working with the code. Only if the functionality of a method is used by multiple other methods (either internal or external) should it be given its own named method; and not one second sooner. This principle is almost always ignored.

Feature Envy Smell.
I'm not sure from reading whether "Tell Don't Ask" is part of the semantics being promoted. Allen Holub (C, C++, and java guru), Dave Thomas (the ruby one), and others make the case that objects (and a database table is an object) are implementations of capabilities (Holub's term), not a shotgun marriage of data and function. With this semantic of capabilities, objects don't pass data, systems don't have a mix of data objects and action objects, don't rely on get/set syntax. They tell the objects they make or acquire what to do, not retrieve data from said objects upon which they then perform operations.

Subject: Smelly code
Posted by: Anonymous (not signed in)
Posted on: Thursday, May 07, 2009 at 9:12 AM
Message: I really love this article. It has set me wondering if the same principals can be made to apply to TSQL code. Maybe we need our own special terms. Hmm..

Subject: Nothing new
Posted by: Anonymous (not signed in)
Posted on: Tuesday, May 26, 2009 at 7:13 AM
Message: Nothing new, just a rehash of anything you can read in a decent book on refactoring (which is probably what you copied it from?)

Subject: Smelly Switches
Posted by: Anonymous (not signed in)
Posted on: Tuesday, May 26, 2009 at 7:31 AM
Message: We got so many of smelly switches in our current code base, It is really hard to get approval for fixing them with the timelines

Subject: Re: Nothing new.
Posted by: Phil Factor (view profile)
Posted on: Tuesday, May 26, 2009 at 12:02 PM
Message: Perhaps the anonymous commenter who talked about a rehash is slightly missing the point. A lot of people want a short article like this, which introduces a complex subject in an engaging way so as to persuade us to go and read the books. I would never have read up this subject had it not been for this article. Not all writings are intended to break new ground, but are required to reflect what has already been written on the subject.


Subject: Re: Nothing new
Posted by: Caledonian Chris (view profile)
Posted on: Tuesday, May 26, 2009 at 4:40 PM
Message: I agree with you Phil. The anonymous commentator is just taking pot shots!

 

Top Rated

Acceptance Testing with FitNesse: Multiplicities and Comparisons
 FitNesse is one of the most popular tools for unit testing since it is designed with a Wiki-style... Read more...

Acceptance Testing with FitNesse: Symbols, Variables and Code-behind Styles
 Although FitNesse can be used as a generic automated testing tool for both applications and databases,... Read more...

Acceptance Testing with FitNesse: Documentation and Infrastructure
 FitNesse is a popular general-purpose wiki-based framework for writing acceptance tests for software... Read more...

TortoiseSVN and Subversion Cookbook Part 11: Subversion and Oracle
 It is only recently that the tools have existed to make source-control easy for database developers.... Read more...

TortoiseSVN and Subversion Cookbook Part 10: Extending the reach of Subversion
 Subversion provides a good way of source-controlling a database, but many operations are best done from... Read more...

Most Viewed

A Complete URL Rewriting Solution for ASP.NET 2.0
 Ever wondered whether it's possible to create neater URLS, free of bulky Query String parameters?... Read more...

Visual Studio Setup - projects and custom actions
 This article describes the kinds of custom actions that can be used in your Visual Studio setup project. Read more...

.NET Application Architecture: the Data Access Layer
 Find out how to design a robust data access layer for your .NET applications. Read more...

Calling Cross Domain Web Services in AJAX
 The latest craze for mashups involves making cross-domain calls to Web Services from APIs made publicly... Read more...

Web Parts in ASP.NET 2.0
 Most Web Parts implementations allow users to create a single portal page where they can personalize... Read more...

Why Join

Over 400,000 Microsoft professionals subscribe to the Simple-Talk technical journal. Join today, it's fast, simple, free and secure.