Code Smell

Highly experienced and knowledgeable developers have a "feel" for good design. Having reached a state of "UnconsciousCompetence," 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 CodeSmell is a hint that something might be wrong, not a certainty. A perfectly good idiom may be considered a CodeSmell because it's often misused, or because there's a simpler alternative that works in most cases. Calling something a CodeSmell is not an attack; it's simply a sign that a closer look is warranted. so CodeSmell is more instinctive that intuitive? -- S

[Copied from GotoConsideredHarmful:]

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

• Pragmatic: CodeSmells should be considered on a case by case basis
• Purist: all CodeSmells should be avoided, no exceptions

If I recall correctly, this is part of the reason why (was it KentBeck or MartinFowler) chose this name for the term. They wanted to emphasize the pragmatic view of CodeSmells 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

• CodeStench, but note that all CodeSmells are CodeStenches to purists!
• CodeSmellTemplate

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:

• DuplicatedCode - See: OnceAndOnlyOnce, SwitchStatementsSmell
• Methods too big - See: ComposedMethod, TallerThanMe, LongMethodSmell
• Classes with too many instance variables - See: ExtractComponent?, ValueObject, and WholeValue?
• Classes with too much code - See: OneResponsibilityRule, GodClass
• Strikingly similar subclasses - See: BehaviorToState
• ParallelInheritanceHierarchies - Violates OnceAndOnlyOnce.
• An instance variable that is only set in some circumstances
• Comparing variables to null - See: NullObject, NullConsideredHarmful
• Too many private (or protected) methods - MethodsShouldBePublic.
• Many messages to the same object from the same method - See: MoveMethod.
• VerbSubject construction - See: MoveMethod
• ExcessiveOverloading - this probably like VerbSubject construction - See: MoveMethod
• SameNameDifferentMeaning
• ExpensiveSetUpSmell - As in the "setUp()" of JavaUnit.
• WithBlocks (WithBlockCodeSmell) - ExtractMethod 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: YouDontNeedItAnymore
• Similar looking code sections that vary only a few percent - CodeGenerationIsaDesignSmell unless it's ActiveCodeGeneration, in which case it's a valid workaround for a LanguageSmell.

• Classes with too few instance variables
• Classes with too little code - See: OneResponsibilityRule
• Methods with no messages to self - See: UtilityMethod?
• Empty catch clauses - See: EmptyCatchClause
• 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.

• CeePreprocessorStatements
• Comments - See: ToNeedComments, TooMuchDocumentation
• ExcessiveLogging? - Lots of logs are needed to figure out what the heck the code is doing!
• BoredomIsaSmell - If you're bored, you might be doing something wrong.

• Sporadic ChangeVelocity - Different rates of change in the same object.
• ShotGunSurgery? - The same rate of change in different objects, particularly if they are disconnected.A conceptually simple change that requires modification to code in many places. The result of CopyAndPasteProgramming.

• ContrivedInterfaces - A smell of PrematureGeneralization.
• AsymmetricalCode/Imbalance
  • VariableClumps
• ArrowAntiPattern (nested if statements), Complex Conditionals
• LawOfDemeter 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 SpecialFormatting to be readable
• BackPedalling (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. e.g createWhateverFromWhateverOtherClass(OtherClass? creator) vs creator.createWhatever(). See UsingGoodNamingToDetectBadCode.
• VagueIdentifiers
• Procedural code masquerading as objects. See DontNameClassesObjectManagerHandlerOrData
• 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.
• PassingNullsToConstructors? - use a FactoryMethod
• TooManyParameters, LongParameterList
• VariableNameSameAsType
• WhileNotDoneLoops
• 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 UnsupportedOperationException? for the other half. The second implementor implements the other half and throws UnsupportedOperationException? for the first half. It would be better to split the interface into two. Similar to RefusedBequest?.
• 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.

• DivergentChange?
• ShotgunSurgery?
• FeatureEnvy
• DataClumps?
• PrimitiveObsession?
• SwitchStatementsSmell
• ParallelInheritanceHierarchies
• LazyClass? (CollapseHierarchy)
• SpeculativeGenerality
• TemporaryField?
• MessageChains?
• MiddleMan?
• InappropriateIntimacy?
• Alternative Classes with Different Interfaces
• IncompleteLibraryClass?
• DataClass?
• RefusedBequest?

Some Principles for judging whether code smells bad or good are defined in PrinciplesOfObjectOrientedDesign.

See Also:

• CodeDeodorant
• ListenToTheCode - For another sensory metaphor.
• CodeSmellsIllustratedWithJavaAwt - More examples.
• CodingCostModel - Asking for an objective metric for "better code" (i.e.: "not smells")
• SmallLint - a SmalltalkLanguage style checker, with some rules for "good" and "bad" constructs.
• BiggerRefactoringThoughts - CodeSmells prompt RefactorMercilessly, which often ends up rediscovering DesignPatterns.
• Finding smells: ChangeBrainstrom?, CommentBrainstorm
• The Other Smellable Things: ModelSmells, TextSmells
• CodeSensing

[This list needs refactoring to above:] CodeSmells referenced in:

• RefactoringImprovingTheDesignOfExistingCode - a book including KentBeck's comments on CodeSmells and refactoring.
• WikiPagesAboutRefactoring - related pages.
• BeautyAintMyBusinessNoSir - KentBeck's comment on parallel inheritance hierarchies, [a minor example of code that smells].
• DavesRealExampleWhereThinkingAheadWouldHaveHelped - An argument over YouArentGonnaNeedIt: After DoTheSimplestThingThatCouldPossiblyWork the CodeSmells because it violates the OnceAndOnlyOnce rule.
• ExtremeNormalFormDefined - another metric to drive refactoring?
• ExtremeNormalFormDefinitions - (same as ExtremeNormalFormDefined)
• ExtremeReuse - reuse is good. But you may have to ignore CodeSmells in 3rd party library, unless you want maintenance and upgrade problems.
• XpProductivityMeasurementProblem - and does it, and refactoring, violate YouArentGonnaNeedIt?
• MyJavaStudents - (...just a reading recommendation.)
• ValueObjectsShouldBePassedByValue - ''a vague feeling of "wrongness."
• CodeSmellMetrics - Objective signals that your code might benefit from some refactoring
• TestResistance? - If writing UnitTests for your code is difficult, your code is probably not well-factored. Pay attention when you become reluctant to write necessary tests.

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 ListenToTheCode.

. . . . . .

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... -- RussellGold

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 BadThings. 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 GoodThing? 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".

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 greymatter 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. -- EricScheid

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)

CodeSmell can creep up on you. Look at EmpiricalPatternDiscovery as a development tool to guide you in the process of SlowRefactoring? as complement to doing what we call RefactorMercilessly. -- NiclasOlofsson

While chatting with CharlesMedcoff over drinks, we came up with the idea of ProcessSmell?'s. He gave the example of a company that has more QA staff than developers. Maybe this should be called MassiveQualityAssuranceRequired, or maybe something shorter. Hmmm...

-- EricRunquist

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

Yes, but 'smell' is not neutral enough for my liking. I suppose CodeSmell is more neutral than CodeStench or CodePerfume?.

-- DavidVincent

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

CodeSuspicionButNotRavingParanoia?? :-)

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++ (EditHint: do we need a separate page for this?)

TailCallOptimization 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.

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.

CategoryCodeSmell CategoryJargon

This page mirrored in WikiPagesAboutRefactoring as of April 29, 2006