Code Smells

The concept of a code smell is, perhaps, one of the most evocative in our profession.  The name itself has a levity factor to it, conjuring a mental image of one’s coworkers writing code so bad that it actually emits a foul odor.  But the metaphor has a certain utility as well in the “where there’s smoke, there may be fire” sense.

In case you’re not familiar, a code smell is an observable feature of the code (the smoke) that often belies a deeper existing problem (the fire).  When you say that a code smell exists, what you’re communicating is “you may be justified here, but I’m skeptical – in my experience this is probably a design flaw.”

Of course, accusing code of having a smell is only slightly less incendiary to the author than accusing code of being flat out bad.  Them’s fightin’ words, as they say.  But, for all the arguments and all of the righteous indignation that code smell accusations have generated over the years, their usefulness is undeniable.

No doubt you’ve heard of some of the most common and easiest to visualize code smells.  The God Class, Primitive Obsession, and Inappropriate Intimacy all come to mind.  These indicate, respectively a class in your code base doing way too much, a tendency to use primitive types when you should take advantage of classes, and a module or class that breaks encapsulation by knowing too many details about another.  The combination of their visual memorability and their wisdom has prodded us over the years to break things down, to create cohesive objects, and to preserve encapsulation.

I would argue, however, that there are many more code smells out there than the big, iconic ones that get a lot of attention.  I’d like today to discuss a few that I don’t think are as commonly known.  I’ll make the case for why, once you’ve mastered avoiding the well-known ones, you should watch for these as well.

Bloated Constructor

One smell that I see rear its head a lot, but that might not get as much play as others is the bloated constructor.  The constructor is where you should put the logic necessary to satisfy your object’s preconditions.  And, that’s it.  Nothing else belongs in there.

For one thing, bloated constructors make your classes difficult to test because a lot of code has to be executed just to get the things instantiated.  But, beyond that, it makes your application inflexible as well, and it’s often indicative of a wholesale, procedural approach to code.  If you see bloated constructors, you can usually bet that objects are not really organized by behaviors.

Excessive Using

I wanted to give this a memorable name, and “excessive using” is where I landed.  What I’m referring to here is having a class that uses a lot of types (which is often quickly visible in C# with a lot of using statements at the top of the file).  This is a smell indicating that your type is probably doing too much.

Many years ago, I encountered a class where the author had to use the full type qualifier for a class in the Excel library called something like “DataSet.”  The reason?  It was because there was a name collision with something in the SQL driver that was also called dataset.  The deeper, underlying problem?  This one class was dealing with spreadsheets and databases.  If there’s a more obvious prima facie violation of theSingle Responsibility Principle, I can’t recall it.

If you have a class using a large number of other types, whether in your codebase or in a third party or framework libraries, there’s a good chance it’s doing too much.  And a class doing too much is a maintenance headache.

Lazy Loading

This one might seem a bit controversial, but I would argue that the presence of lazy loading in your application is a smell.   Surely I must be crazy, right?   I mean, lazy loading is an elegant solution  to two simultaneous problems: how else do you avoid speculative performance hits while presenting a clean façade to consumers of your API?  Lazy loading ensures you’ll incur the performance hit at the last responsible moment and then not again.

All of that is true, and all of that is the reason that “code smell” is not simply called “code problem.”  The trouble with lazy loading comes in a more insidious form, however.  The trouble is that in many codebases it winds up being overused and broadly shared.  The data access object manager’s lazy loading needs to log, so it triggers the logging lazy loader, which, naturally, triggers the configuration management lazy loader, which itself triggers… well, you get the idea.

Lazy loading is powerful and, when used tactically, it retains that power while helping you.  But the trouble is that lazy-loading constructs have two distinct operating modes, and those modes are opaque to their consumers.  In this fashion, a powerful construct can wind up turning your code into a confusing quagmire that’s nearly impossible to reason about at compile time.

Ask and Tell

The last lesser known smell that I’ll mention I call “Ask and Tell.”  With this, I’m referring to methods that return values and have side effects at the same time.  For instance, consider a method that takes a string, writes that string to a file, and then returns a Boolean indicating whether or not the operation succeeded.  This is “ask and tell,” because invoking the method involves telling the API to do something (write to the file) and asking it a question (“what happened?”).

There is a less well-known principle in programming known as “command-query separation,” which advises that any method should either be a side-effect free query about the state of something or a void operation that changes the state of something.  It should not be both.

The attraction of this principle comes in how elegantly it allows you to reason about APIs, provided they comply.  If anything returns a value you to you, you can count on it not having any side effects in computing that value.  Thus if you have an Invoice object and you ask it what the total is, you can count on it not doing something weird like charging a credit card or dumping information to a database.  (As an aside, the aforementioned lazy loading is an ipso facto violation of this principle).  If you find yourself looking at a void method, on the other hand, you know it will have an effect.

Like lazy loading, ask and tell methods are not something to crusade to eradicate from your codebase.  But if you find yourself using them frequently, you should probably rethink your APIs and class designs.

Code Smells: Not Hard to Detect

All four of these code smells are relatively straightforward to detect.  If you’re an NDepend user, the first two would be simple tweaks to out of the box metrics/stats.  The latter two would be fairly straightforward to take a crack at with CQLinq.

But whether you use an automated tool to detect them or simply through code review/inspection, they’re worth looking out for.  You could easily tie yourself in knots trying to conform to all principles and avoid all code smells, but being aware of them and keeping your eyes open can only make you a better developer and your code less likely to draw accusations of odor.