Code Smell

A code smell is a hint that something has gone wrong somewhere in your code. Use the smell to track down the problem. Kent Beck (with inspiration from the nose of Massimo Arnoldi) seems to have coined the phrase in the "Once And Only Once" page, where he also said that code "wants to be simple". Bad Smells in Code was an essay by Kent Beck and Martin Fowler, published as Chapter 3 of Refactoring Improving The Design Of Existing Code.

Highly experienced and knowledgeable developers have a "feel" for good design. Having reached a state of "Unconscious Competence," where they routinely practice good design without thinking about it too much, they find that they can look at a design or the code and immediately get a "feel" for its quality, without getting bogged down in extensive "logically detailed arguments".

Note that a Code Smell is a hint that something might be wrong, not a certainty. A perfectly good idiom may be considered a Code Smell because it's often misused, or because there's a simpler alternative that works in most cases. Calling something a Code Smell is not an attack; it's simply a sign that a closer look is warranted.

Theyre Just Rules or as Douglas Bader put it : "Rules are for the guidance of the wise and the obedience of fools."

There are two major approaches to programming: [is there a page that covers this? there should be]

Pragmatic:
Code Smells should be considered on a case by case basis

Purist:
all Code Smells should be avoided, no exceptions

...And therefore a Code Smell is a hint of possible bad practice to a pragmatist, but a sure sign of bad practice to a purist.

If I recall correctly, this is part of the reason why (was it Kent Beck or Martin Fowler) chose this name for the term. They wanted to emphasize the pragmatic view of Code Smells by the connotation of the word smell. If something smells, it definitely needs to be checked out, but it may not actually need fixing or might have to just be tolerated.

See also

Code Stench, but note that all Code Smells are Code Stenches to purists!


Here are some Code Smells that tell you something might be wrong.

Too much code, time to take something off the stove before it boils over:

Classes with too many instance variables - See:
Extract Component, Value Object, and Whole Value

Classes with too much code - See:
One Responsibility Rule, God Class

Strikingly similar subclasses - See:
Behavior To State

An instance variable that is only set in some circumstances

Comparing variables to null - See:
Null Object, Null Considered Harmful

Too many private (or protected) methods - Methods Should Be Public.

Feature Envy Smell, many messages to the same object from the same method - See:
Move Method.

Verb Subject construction - See:
Move Method

Excessive Overloading - this probably like Verb Subject construction - See:
Move Method

Expensive Set Up Smell - As in the "setUp()" of Java Unit.

With Blocks (With Block Code Smell) - Extract Method to the object the code is acting on. (Or encapsulate the data or 3rd party object in an object you control and can add methods to.)

Code not actually ever used - see:
You Dont Need It Anymore

Trivial modules or layers - they only call the next layer down

Similar looking code sections that vary only a few percent - Code Generation Isa Design Smell unless it's Active Code Generation, in which case it's a valid workaround for a Language Smell.

Not enough code, better put the half-baked code back in the oven a while:

Classes with too few instance variables

Classes with too little code - See:
One Responsibility Rule

Methods with no messages to self - See:
Utility Method

Explicitly setting variables to null. Can indicate that either

there are references to things that this code has no business referencing, or

the structure is so complex that the programmer doesn't really understand it and feels the need to do this to be safe.

Is this too language specific? For example in Perl and other agile/scripting languages setting a variable to null can be the equivalent of destroying an object. -- Mark Aufflick

Not actually the code:

Excessive Logging - Lots of logs are needed to figure out what the heck the code is doing!

Boredom Isa Smell - If you're bored, you might be doing something wrong.

Problems with the way the code is changing:

Sporadic Change Velocity - Different rates of change in the same object.

Shotgun Surgery - The same rate of change in different, disconnected objects.

Other code problems:

Arrow Anti Pattern (nested if statements), Complex Conditionals

Law Of Demeter Violations - Messages to the results of messages.

Dependency cycles among packages or classes within a package.

Concrete classes that depend on other concrete classes

Methods requiring Special Formatting to be readable

Back Pedalling (loss of context)

Long method names. Seriously:
If you follow good naming standards, long method names are often an indication that the method is in the wrong class. For example, createWhateverFromWhateverOtherClass(Other Class creator) vs creator.createWhatever(). See Using Good Naming To Detect Bad Code.

Procedural code masquerading as objects. See Dont Name Classes Object Manager Handler Or Data

Embedded code strings. Large chunks of SQL, HTML or XML (for example) are not best read, edited or tested in the code. They start there because its simpler, but end up making both languages unreadable as they get more complex, and require developers to know both languages.

False unification of procedures. A procedure, function, or method has a boolean that provides a variation on its behavior, but the two variations in fact have completely different semantics. It would be better to refactor the common code into another method and split the remainder into two separate methods, removing the boolean.

False unification of interfaces. An interface has two implementors. One implementor implements half of the interface and throws Unsupported Operation Exception for the other half. The second implementor implements the other half and throws Unsupported Operation Exception for the first half. It would be better to split the interface into two. Similar to Refused Bequest.

Hidden coupling. Code depends on completely non-obvious characteristics, such as relying on reference equality instead of value equality.

Hardwired policy. Instead of policy being wrapped around a mechanism, the policy is wired directly into the mechanism. Often, implementation of the policy requires additional information not available, so it has to be fetched from outside, creating an implicit external dependency. Instead of a simple, self-contained mechanism, you now have a fragile, context-sensitive mechanism.

Alternative Classes with Different Interfaces

The list of canonical smells found in Refactoring Improving The Design Of Existing Code can be found at: jexp.de (we're slowly making sure they're all here).

-- updated broken link, Michael Hunger

Some Principles for judging whether code smells bad or good are defined in Principles Of Object Oriented Design.

See Also:

Listen To The Code - For another sensory metaphor.

Coding Cost Model - Asking for an objective metric for "better code" (i.e.:
"not smells")

Small Lint - a Smalltalk Language style checker, with some rules for "good" and "bad" constructs.

Bigger Refactoring Thoughts - Code Smells prompt Refactor Mercilessly, which often ends up rediscovering Design Patterns.

The Other Smellable Things:
Model Smells, Text Smells, Language Smells


[This list needs refactoring to above:] Code Smells referenced in:

Refactoring Improving The Design Of Existing Code - a book including Kent Beck's comments on Code Smells and refactoring.

Beauty Aint My Business No Sir - Kent Beck's comment on parallel inheritance hierarchies, [a minor example of code that smells].

Extreme Normal Form Defined - another metric to drive refactoring?

Extreme Reuse - reuse is good. But you may have to ignore Code Smells in 3rd party library, unless you want maintenance and upgrade problems.

Xp Productivity Measurement Problem - and does it, and refactoring, violate You Arent Gonna Need It?

My Java Students - (...just a reading recommendation.)

Value Objects Should Be Passed By Value - ''a vague feeling of "wrongness."

Code Smell Metrics - Objective signals that your code might benefit from some refactoring

Test Resistance - If writing Unit Tests for your code is difficult, your code is probably not well-factored. Pay attention when you become reluctant to write necessary tests.

Abstraction Distraction - Use of different abstraction levels in the same code.


Discussion:

What ever happened to "Listen to what Smalltalk is telling you?"

Same thing, isn't it? Just a different analogy: perhaps some of us smell out code problems; some see them; and some Listen To The Code.

. . . . . .

I have recently completed a Sensory Orientation Survey of 11,352 randomly-selected software developers, determining their primary sensory mode. 10,901 were visually-oriented, 430 hearing-oriented, 20 were touch-oriented, and one was smell-oriented. Some other interesting statistics were also gleaned, but I'm not sure how they are related. There were 386 visually-impaired developers, 19 vision-and-hearing-impaired developers, and one developer named Spot.

I think I interviewed Spot today.

I think my major problem with the terminology is that it complicates critiques. It means one thing for one developer to say to another, "your code doesn't sound right," or, "your code sounds off-key," and quite a different thing to say, "your code smells." I wish the earlier auditory metaphor had stuck... -- Russell Gold

Most people are visual ("I see that"). Fewer are auditory ("I hear you"). Fewer still are tactile ("I feel that"). So few use smell as their primary sense that speaking of smells seems a bit alien to almost everyone. Perhaps that makes it a good choice.

You see this, you hear that. But you go "what is that smell ?"

It must be Coffee Which brings up the point that all smells are not Bad Things. Smells tend to permeate the air and are not necessarily easy to track down. (Except in the kitchen). It seems that smell has been used in a negative sense. Could this term be used to describe a Good Thing? Can someone create code that smells good?

Smell is such a primitive (neurologically speaking) sense that people often respond to smells without being consciously aware of them, with their hindbrain acting on it before the cerebrum learns about it (if it ever does). This goes for smells good and bad. Not to say the same thing doesn't happen with other senses, but that "something's going on here" impression that comes with a consciously unperceived whiff of something may possibly be not too dissimilar to the same "something's going on here" impression that comes with "smelly code".

I think that some of this misses the point. Smell is an indirect sense. It has in humans a very limited direction finding capablity. It may indicate an issue or it may not. The smell that you think is due to X may be due to Y. But most important to my mind it is a primative tool of hunting. We are hunting issues in code. Seeing is precise, directional and of limited range. Hearing is less percise, may or may not be directional ( echos frequency issues etc) and intermediate range. Smell is not precise, has limited direction, but may cary for miles on the wind. The Fire you smell to the west may in fact be on your east, if the wind has carryied it around. There is a fire somewhere, it is big, it is a problem-- so start hunting for it. .. If you think that 'nondirectional longrange indeterminate trouble detection criterion' is a better term to use-- have at it.


In English, "it smells" ~= "it smells bad".


Code is too abstract to map well to visual metaphor, and not enough people have the ear for auditory metaphor, but [almost] everyone has a nose and can smell garbage or roses. -- Pete Hardie

Also, smells cause a greater visceral reaction, reaching deeper into the grey matter than sight or sound. Both sight and sound are so overloaded that we have evolved the ability to tune out a lot of noise and clutter. Its easier to provoke a response with a smell, assuming the scent is communicated. -- Eric Scheid


Ah, but when you smell a problem, you only know that you have to fix something. You still have to look to find exactly what needs fixing.

The smell is only a symptom.


When I eat a bowl of plain rice while smelling my neighbor's broiled duck, I eat a feast fit for a king. -- Chinese saying (paraphrased)


Code Smell can creep up on you. Look at Empirical Pattern Discovery as a development tool to guide you in the process of Slow Refactoring as complement to doing what we call Refactor Mercilessly. -- Niclas Olofsson


While chatting with Charles Medcoff over drinks, we came up with the idea of Process Smell's. He gave the example of a company that has more QA staff than developers. Maybe this should be called Massive Quality Assurance Required, or maybe something shorter. Hmmm...



In English, "it smells" ~= "it smells bad".

Yes, but 'smell' is not neutral enough for my liking. I suppose Code Smell is more neutral than Code Stench or Code Perfume.

Codour? (or Codor, for those that think "US English" is a language) ;o) -- Nick Grimshaw


A bit of empirical evidence for the code smell concept - see "Using Redundancies to Find Error" at www.stanford.edu .

In it, the authors use code scanners to look for redundant operations (assignment to self, duplicated conditions statements). They find that not only are redundant operations sometimes "hard errors" themselves, but also that the presence of redundant operations in a source file is a good predictor of other hard errors in the same file.


code smells in C, C++ (Edit Hint: do we need a separate page for this?)

Tail Call Optimization has some example code. One function has a ("automatic") local variable, and it passes that variable to another function. Then it mentions

"On the other hand, bar() may do something like tuck the pointer away for later use, so using an automatic variable this way may not be safe anyhow."

I agree - this may not be safe; it's a code smell. I can't decide if it's the

"passing a pointer to a local variable" part that smells bad, or the

"tuck the pointer away for later use" part that smells bad.

The problem: After that function tucks the pointer away for later use and returns, after the calling function returns, then the (stack) memory that pointer points to is no longer valid. It probably gets overwritten with other values. Then if that pointer ever gets used, it is pointing to irrelevant invalid data.

Java goes to great lengths to make sure this problem never happens, by making it impossible to pass a pointer to a local (stack) variable - the only pointers that can be passed are "references" to objects created on the heap.

Please look at Java Servlets and the re-entrancy problems due to avoidance of stack based storage. I don't want to start yet another language battle, I just want to highlight that memory management issues are a complex problem that rears its head in different ways in different languages.

I guess it depends on the documentation for the function. If a function 'tucks away' a pointer, then I would expect it to have a name like 'register_foo' or 'insert_foo' or at least document the fact that it requires a long-lived heap pointer. In C++ of course, you would use shared_ptr and avoid any problems with stack pointers becoming invalid. Mike Weller.


'code fragrance' - when a piece of coding is particularly elegant in design and efficiency. (code smells good)

Who said, "when I find myself congratulating myself on how very clever a line or word is, I take it out"?


Another advantage of the "Smell" metaphor is that scents are often very noticeable initially, but after a while you become accustomed to them and don't notice them any more, which I figure is also the case for Code Smells, if you're not careful. (And even more so for User Interface Smells, I suppose.) -- Scott Mc Murray


Category comment

I de-duplicated the Development Anti Patterns area in the Anti Pattern Catalog. There is some overlaps between Development Anti Patterns found in the Anti Pattern Catalog and the pages attached to the Category Code Smell which often uses the same template as Anti Pattern.

In order to get the maximum of pages auto-indexed in categories, I propose:

Then in the Development Anti Pattern Road Map, links will be put to Category Code Smell.

All not categorized Code Smells will be attached to both categories Category Code Smell (few pages not in there) and Category Development Anti Pattern.

Any comments?


Boilerplate. The existence of boilerplate code is either a smell coming from your code (if it's reasonably possible to eliminate it) or from your language / framework (if it isn't). Some languages (e.g. Lisp Macro) make boilerplate elimination easy, others (e.g. Csharp Language) positively require boilerplate under certain circumstances.


I propose that Hiding Type With String should be considered a Code Smell. -- Martin Spamer

Some examples.

String telephoneNo; String xPath;

Yes -- we tend to call that Primitive Obsession. -- Jb Rainsberger


See also: What Isa Smell


What about Bad Smell In Data? Alot of data can also be an indication of too much or improperly specified factoring. -- Id Know.


Code Smells can often be caused by doing Information Programming in a Data Programming Context or Data Programming in an Information Programming Context. In many cases the opposite approaches are correct for these two situations. For example, coding things with letter is bad in data and good in information. Storing UI labels in a database is bad in data and good in information (but you need an Info Base to do it right). Using plurals for table names is bad in a database (which is designed to handle data organization entities - tables) and may be good when building information organization entities.

We need a term to describe a Code Smell that indicates that the code is good or well built. Perhaps call that a Good Code Smell.

See original on c2.com