Good Design, Part II: Code Smells

Last month we talked about good software design and introduced the notion of code smells. Code smells are names given to those instinctual thoughts you have whenever you look at a chunk of less-than-elegant code. Some are subtle, and some really stink.

Stinky Code

Each code smell represents something that would make understanding, extending, or maintaining the code just a bit more challenging than it needs to be.

How to use them on your team? The good news is that this is easy. You and your teammates know what you don’t like to see. You’ll be looking at a blob of code because you need to either understand or alter it and upon first glance, the worst of those code smells will hit you. You refactor away those, then look again. You refactor until you understand the code sufficiently, and adding some bit of new functionality is made easier. Refactoring is an “Incremental Design” practice that results in readable, sufficiently flexible “Emergent” designs.

This makes refactoring the indispensable engineering practice for successful incremental software development.

But I’m getting ahead of myself. Let’s explore some common code smells. The following is Java or C# code (with Java naming idioms). The Connection object is this team’s home-brewed database interface. Don’t get your axle wrapped up in those details. Also, we’re not looking for defects. Code smells are definitely not the same thing as functional defects because function (preferably tested) and design are orthogonal aspects of software development.

Just…follow your nose, and trust your instincts and experience. Perhaps write down what you dislike about this code.

public static ProductBean installProduct(String p) {

ProductBean pi = new ProductBean(p);

Connection c = new Connection(“$Updates”);

for (Record x : c.getAllForKey(“PLUG_INS”, “P_NAME”, p)) {

// validate serial number

if (“SNUM”.equals(x.getString(“RECORD_TYPE”))) { SecurityHelper.validateProduct(

x.getString(“SERIAL_NUMBER”),

getEncryptionCertificate()); pi.addSerialNumber(x.getString(“SERIAL_NUMBER”));

}

// install license

if (“LIC”.equals(x.getString(“RECORD_TYPE”))) { SecurityHelper.validateLicense(

x.getString(“USER_LICENSE”),

getEncryptionCertificate());

SecurityHelper.registerLicense(

x.getString(“USER_LICENSE”));

pi.addLicense(x.getString(“USER_LICENSE”));

}

// install driver

if (“PDRV”.equals(x.getString(“RECORD_TYPE”))) { Drivers.hotInstall(x.getBlob(“BIN_DRIVER”));

}

}

return pi;

}

What’s wrong with this code? Nothing? Looks fine?

Or everything?

The detection of code smells is sensitive to team context and the context of the product’s existing functionality. Who is on the team? What experience have they had with procedural programming (it’s a static method, which is Java’s way of providing procedural programming structure in an object-oriented language)? Object-oriented programming? Patterns? Are we planning to fix a defect? Do we need to add a new record type? Is there similar code elsewhere that works with a different type of data-source? How old is this code?

Without knowing these things, you and I will not be able to guess what the “best” design would be. Once your team is familiar with many of the most common smells, and the refactorings needed to clean them up, you’ll see just how bad this code is because you’ll have seen much better code.

Code smells have names for the same reason design patterns and refactorings have names: So that we can quickly communicate complex ideas to others on our team.

Here are some code smells I notice, above, and refactorings that could help.

Long Method
The first thing that strikes me is the length of the method. Right away I know it’s doing a lot because I have to take time to study it. A well-factored method is quickly read, digested, and understood (“grokked”).

Long Method is resolved mostly using the Extract Method refactoring. We take an identifiable piece of what it’s doing and move that into its own well-named method.

Comments
Fowler described Comments as a code smell, but as a pleasant smell we often use to cover up other stinky code.

Again, Extract Method is appropriate in our case, and the comment becomes a template for the intention-revealing name of the method called. We’ll want to choose a method name that makes sense in the calling code; more so than in the definition of the method.

Whitespace
My own flavor of the Comments code smell. Again, it’s not a bad thing, per se, but whitespace between paragraphs of code tells me that the method I’m looking at is doing more than one thing, which means it’s doing too much. (I.e., It lacks cohesiveness.)

Yep, Extract Method again.

Duplicated Code
This is the most common, and most odiferous of code smells. There are the obvious, character-for-character forms (e.g., .equals(x.getString(“RECORD_TYPE”) occurs in three different places). There are also more subtle forms. For example, notice how each if statement follows the same format: If the record-type is a certain value, get some other data from the record and do something with it.

There are numerous refactorings that help with the various forms of duplication.

In our case, you could imagine refactoring the if() statements into a switch() statement, assuming that the programming language can currently support switching on string values (was not always so!).

Switches can also be a code smell, though, particularly if there are two switches on the same value. We don’t see that here, but I’ve seen it used a lot.

Two switch() statements switching on the same value is called “Poor Man’s Polymorphism.” The solution is barely hiding in that smell name: Refactor to a simple, lightweight object hierarchy using Extract Class, Extract Superclass, and perhaps refactoring to a Strategy or a Template Method pattern.

If that seems like overkill, you may want to review Kent Beck’s Four Rules of Simple Design: Passing tests (and testability) are of prime importance, and the objects that make up traditional Design Patterns are amazingly testable. Yes, you’ll feel like your barely testing anything on tiny objects. I’ll reassure you, that’s the right feeling. We call them unit tests, or micro tests, after all. Powerful, fluid, composed behaviors are built by keeping discrete behaviors separated until runtime.

With this particular sample of stinky code, you could also use lambda functions (closures, anonymous functions…), one per record-type. I would choose named methods or classes, or anonymous lambda methods, depending on whether naming the behaviors would be beneficial elsewhere, or would simply aid in comprehension. Another important consideration is what functionality I’m planning to add via TDD or BDD, and what behavioral variation is implied by that addition. For example, will I need to add processing for other record types? How often, and how is that configured? Per product or per platform? The variations are endless and unpredictable, so I encourage teams to defer these design decisions until the PBI (or user story) arrives in their sprint backlog (or queue).

Magic String
A cousin of Magic Numbers. Magic Numbers are hard-coded constants that aren’t declared as constants. They just appear in their raw, naked form in the code. For example, (x < 1000). Why 1000? What is that? What if I find a different 1000 elsewhere in the system? Are they related??? We have to read through much more of the code in order to determine why there is a 1000 in a particular place.

Extract Constant is a good way to deal with Magic Numbers, but you have to be careful not to let your powerful IDE replace all 1000s unless they all really represent the same thing.

Similarly, the string “RECORD_TYPE”. Here, the dangers are a bit different. After all, this string is fairly self-explanatory. What I’ve seen is when someone decides that this string needs to change, but doesn’t change them all. Or, somehow the cursor falls into the string and someone adds a typo. Extract Constant thus reduces duplication and reduces the opportunity to break it.

Abbreviations, or Bad Names
pi?! x?! Need I say more? With today’s sophisticated IDEs, there’s never any reason to type a longer name more than once. So you’re free to name identifiers clearly. Avoid jargon (unless it’s part of your whole team’s and customers’ ubiquitous language), and avoid acronyms and abbreviations. If your classes contain the word Abstract or end in Impl, you’re breaking the readability rule by adding noise to the code. Yes, this also applies to Hungarian Notation, particularly in languages with strong typing.

Rename Method, Rename Class, and Rename Variable, all allow us to improve our code and match our emerging ubiquitous language.

Refactoring
Follow your nose. We don’t have to refactor all our code all at once. Something (a new user story, for example) has us examining or altering a particular subset of classes.

We typically start with the really stinky smells first, then clean up the remaining smells that are preventing us from skillfully accomplishing our task. We commit frequently, so we can recover whenever a test fails. A failing test means the step we just took wasn’t a complete refactoring. We’ve taken a misstep. Frequent commits also allow us to rewind a set of refactorings in a controlled way, in case the design goes someplace that isn’t working out.

Keep in mind, we’re talking about minutes, or perhaps an hour in the worst case; not hours or days. Run the tests whenever they should pass, and commit locally whenever they do pass.

Next Up

During your many hours of holiday boredom, feel free to try out some refactorings of the code above. But if you don’t get to it, no worries…

After the holidays, we’ll continue with Part III of these Developer Essentials “Good Design” miniseries. We’ll tune in to our favorite developers, Alice and Bob, as they refactor the code above. Then, in Part IV (the finale?), we’ll look more deeply at the role tests (or “specs”) play in good software design.