Your code stinks! 13 signs that your code needs refactoring

Code smells are signs that there may be deeper problems with your code; here are a number of common ones to keep your nose peeled for

13 Common code smells
Credit: REUTERS/Health Ministry/Handout

An important part of a software developer’s job is to find and fix problems within code. Everybody knows about bugs that prevent software from functioning properly. Sometimes, though, there are more subtle problems that don’t prevent it from doing what it's supposed to do, but which can make it harder to maintain or more susceptible to bugs in the future. Code smells are indicators of such problems which, when present, can mean that your code should be refactored. There are many types of code smells, some of which are language-specific. However, there are a number of commonly accepted code smells. Here are a baker’s dozen of signs that your code could use some Febreze.

Long Methods
Long Methods

Length is a good quality to have if you’re talking about the wingspan of birds, stretch limos or if you jump into sandpits for a living. But it you’re a programmer, overly long methods, functions or procedures can be harder to maintain and debug. If you find your fingers getting tired from scrolling through a particular method it probably means it’s time to consider breaking it up into smaller functional units of code. When it comes to the length of a method, less is definitely more.

Shotgun Surgery
Shotgun Surgery

Surgery, not matter how minor, is always a danger, so unnecessary surgery should be avoided at all costs. That also goes for operating on your code. If achieving one change in the functionality of your application requires changes to lots of different parts of your code base (such as a number of different classes) then maintenance becomes more risky, complex and time consuming. If you’re faced with this problem, it’s time to clean things up so the affected code is together, for example, in one class, rather than spread across several (or many). The best part: no need to deal with insurance companies.

Inappropriate Intimacy
Inappropriate Intimacy

Two people sharing their innermost thoughts and secrets can be a good thing - unless those people are in relationships with other people, in which case it could be inappropriate. The same can apply to your code; if a method in one class knows too much about the internals of another class, that can mean your code suffers from inappropriate intimacy. Maybe the method in the first class should really be in the second class. Or perhaps the internals of the second class should be public. Or maybe there are two methods doing the same thing. In any case, the inappropriate intimacy shouldn’t be ignored and will need to be resolved with refactoring, not counseling.

Feature Envy
Feature Envy

Envy is one of the Seven Deadly Sins and, potentially, a sign that something is amiss in your code. If you have a method in one class that seems to be spending way too much time calling methods or referencing objects from another class, then it suffers from feature envy. It’s a sign that, perhaps, the method belongs in that other class. Things can get trickier if the method is envious of several other classes, in which case you’ll need to choose the best class for it, or consider breaking it up and moving the pieces to the appropriate classes - and save your code’s eternal soul.

Duplicate Code
Credit: REUTERS/Stringer
Duplicate Code

Sometimes, duplication means doubling your pleasure. But, in the world of programming, duplicating code can mean all sorts of trouble, mainly in terms of maintenance. Think you found the problem causing that bug? Well, you may not have if the bad code is replicated elsewhere. This is often the result of copying, pasting and modifying an existing piece of code. Try to avoid maintenance headaches by not repeating yourself when representing a feature in the code. For similar reasons, that same principle should also be applied to things like database schemas and documentation, not to mention casual conversation.


Divergent Change
Divergent Change

Being able to handle multiple responsibilities can sometimes be a good thing, like for a one-man band, but the same doesn’t always apply to code. If a single class is responsible for too many things, you can find yourself making lots of unrelated changes in the same part of your code, meaning it suffers from  divergent change. This can happen over time as functionality is added and new responsibilities are given to existing classes when, in fact, a new class should have been created. Unlike a one-man band, classes that do too many different things shouldn't be rewarded with spare change or applause. Instead they should be broken up by extracting a new class(es).

Lazy Classes
Lazy Classes

Just as code classes that do too much can be a code smell, so too can lazy classes, that is, classes that aren’t doing enough to justify their existence. Each new class makes your codebase a little more complex and harder to maintain, so it should it at least be doing enough to justify the overhead. If you’ve added a class in anticipation of future changes which never happened, or refactored a significant portion of a class’s workload into other classes, it may be time to ask whether the class is pulling its own weight. Refactor the class’s remaining methods and responsibilities into other classes and you won’t need to pick up after it anymore.


Solution Sprawl
Solution Sprawl

Suburbanites generally know that sprawl can be a bad thing, leading to you spend too much time driving all over the place to get anything done. In programming, spreading a piece of functionality over a number of different classes can mean your application suffers from solution sprawl. Solution sprawl is a smell that’s really the other side of the shotgun surgery coin. If you notice either or both smells, the solution is to consider refactoring the code so the functionality is encapsulated in fewer classes, which is (hopefully) a lot easier than reconfiguring a suburban population.

Refused Bequest
Refused Bequest

Sometimes, children will fight over an inheritance from their parents. In code, though, it can be the case that a subclass may actively refuse some or most of the functionality inherited from a parent class, which can mean your code needs reworking. Admittedly, this is potentially a minor issue that might not even be worth addressing. However, if, for example, a subclass is refusing its inheritance by overriding inherited methods with empty bodies (“no-ops”) this can lead to confusion and means the problem should be addressed. If the smell is this strong, consider moving the refused superclass methods to a sibling class of the ungrateful child. Thankfully, this shouldn’t involve probate.

Primitive obsession
Primitive obsession

If you find your code too often using primitive data types to represent things that really should be represented by a class, then your code smells from primitive obsession. Take the time upfront to write some extra code and create a small class to represent that idea, instead of simply using a string or hash. This will help to prevent headaches later, such as those caused by duplicate code, which can often be the result. Before you know it, you’ll also likely find yourself coming up with new methods to add to the class. Break that obsession now and save your family from having to hold a primitive data type intervention for you later.

Data Clumps
Data Clumps

Like some pasta, data items can sometimes clump together. That is, you will sometimes see the same couple of data items following each other around, as attributes or input parameters. If one data item is hardly ever seen without another (and vice versa), these items are trying to tell you that they should be put into their own class. Doing so will help extinguish several smells with one code snuffer by eliminating not only the data clumps smell, but also potentially other smells like long parameter lists. Once you’ve refactored, celebrate the end of data clumps with some non-clumpy pasta and a nice Chianti.


Middle Man
Middle Man

Ever see the old sitcom bit where one person gets mad at another and they refuse to speak directly to each, so they only communicate through another person, who’s usually sitting on the couch between them? A middle man can make for good comedy, but not for good code. If one of your classes is doing little more than serving as a wrapper for calls to another class, your code has the classic middle man code smell. The added complexity and maintenance overhead that such a class causes just isn’t worth it. Consider removing the useless middle man class and moving whatever real functionality it has into an existing class and leave middle men to the sitcom writers.

Comments
Credit: flickr/THOR
Comments

Whether or not comments are really a code smell is a hotly debated topic. Some advocate no comments in code, arguing that class, method, input parameter and identifier names should be constructed in such a way as to make clear what the code is and does. The presence of any comments, they say, indicates that your code needs to be refactored. However, many others argue that some comments are fine and can be useful, in particular when they tell you the why of the code. Most agree, though, that comments which only tell you what something does indicate that some cleanup is called for. When it comes to comments, “Just the facts, ma’am,” isn’t the way to go.