27 April 2009

Exploring Smelly Code

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:

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:

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:

 

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:

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

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:

 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:

702-image002.jpg

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:

Can be reduced to:

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

Keep up to date with Simple-Talk

For more articles like this delivered fortnightly, sign up to the Simple-Talk newsletter

This post has been viewed 20170 times – thanks for reading.

  • Rate
    [Total: 35    Average: 4/5]
  • Share

Nick Harrison

View all articles by Nick Harrison

Related articles

Also in .NET

Posting Form Content via JavaScript

Web-based applications run smoother if instead of using the traditional form method, they use JavaScript to post data to the server and to update the user interface after posting data: It also makes it easier to keep POST and GET actions separated. SignalR makes it even slicker; it can even update multiple pages at the same time. Is it time to use JavaScript to post data rather than posting via the browser the traditional way?… Read more

Also in .NET Framework

What's New in C# 6

The C# language itself has changed little in version 6, the main importance of the release being the introduction of the Roslyn .NET Compiler Platform. However the New features and improvements that have been made to C# are welcome because they are aimed at aiding productivity. Paulo Morgado explains what they are, and how to use them.… Read more

Also in refactoring

SQL Server System Views: The Basics

When maintaining or refactoring an unfamiliar database, you'll need a fast way to uncover all sorts of facts about the database, its tables, columns keys and indexes. SQL Server's plethora of system catalog views, INFORMATION_SCHEMA views, and dynamic management views contain all the metadata you need, but it isn't always obvious which views are best to use for which sort of information. Many of us could do with a simple explanation, and who better to provide one than Rob Sheldon?… Read more

Also in .NET Framework

The Zen of Code Reviews: Review As If You Own the Code

A code review is a serious business; an essential part of development. Whoever signs off on a code review agrees, essentially, that they would be able to support it in the future, should the original author of the code be unavailable to do it. Review code with the energy you'd use if you owned the code. Michael Sorens runs through the principles of reviewing C# code.… Read more
  • Brent

    Great Article
    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.

  • Brent

    Great Article
    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.

  • BuggyFunBunny

    Sometimes the cure is worse than the disease
    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.

  • Anonymous

    Smelly code
    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..

  • Anonymous

    Nothing new
    Nothing new, just a rehash of anything you can read in a decent book on refactoring (which is probably what you copied it from?)

  • Anonymous

    Smelly Switches
    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

  • Phil Factor

    Re: Nothing new.
    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.

  • Caledonian Chris

    Re: Nothing new
    I agree with you Phil. The anonymous commentator is just taking pot shots!

Join Simple Talk

Join over 200,000 Microsoft professionals, and get full, free access to technical articles, our twice-monthly Simple Talk newsletter, and free SQL tools.

Sign up