Code Smell

출처: http://en.wikipedia.org/wiki/Code_smell

In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem.

Often the deeper problem hinted by a code smell can be uncovered when the code is subjected to a short feedback cycle where it is refactored in small, controlled steps, and the resulting design is examined to see if there are any further code smells that indicate the need of more refactoring. From the point of view of a programmer charged with performing refactoring, code smells are heuristics to indicate when to refactor, and what specific refactoring techniques to use. Thus, a code smell is a driver for refactoring.

The term appears to have been coined by Kent Beck on WardsWiki in the late 1990s. Usage of the term increased after it was featured in Refactoring. Improving the Design of Existing Code.^[1] Code Smell is also a term used by agile programmers. ^[2]

Determining what is and is not a code smell is often a subjective judgment, and will often vary by language, developer and development methodology. There are tools, such as Checkstyle, PMD and FindBugs for Java, to automatically check for certain kinds of code smells. 

Common Code Smells

• Duplicated code: identical or very similar code exists in more than one location.
• Long method: a method, function, or procedure that has grown too large.
• Large class: a class that has grown too large. See God object.
• Too many parameters: a long list of parameters in a procedure or function make readability and code quality worse.
• Feature envy: a class that uses methods of another class excessively.
• Inappropriate intimacy: a class that has dependencies on implementation details of another class.
• Refused bequest: a class that overrides a method of a base class in such a way that the contract of the base class is not honored by the derived class. See Liskov substitution principle.
• Lazy class / Freeloader: a class that does too little.
• Contrived complexity: forced usage of overly complicated design patterns where simpler design would suffice.
• Excessively long identifiers: in particular, the use of naming conventions to provide disambiguation that should be implicit in the software architecture.
• Excessively short identifiers: the name of a variable should reflect its function unless it's obvious.
• Excessive use of literals: these should be coded as named constants, to improve readability and to avoid programming errors. Additionally, literals can and should be externalized into resource files/scripts where possible, to facilitate localization of software if it is intended to be deployed in different regions. 

출처:  http://c2.com/cgi/wiki?CodeSmell

 A code smell is a hint that something has gone wrong somewhere in your code. Use the smell to track down the problem. KentBeck (with inspiration from the nose of MassimoArnoldi) seems to have coined the phrase in the "OnceAndOnlyOnce" page, where he also said that code "wants to be simple". Bad Smells in Code was an essay by KentBeck and MartinFowler, published as Chapter 3 of RefactoringImprovingTheDesignOfExistingCode.

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.

[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

...And therefore a CodeSmell 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 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.
• FeatureEnvySmell, 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
• Trivial modules or layers - they only call the next layer down
• InstanceofInConditionals
• 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.

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: OneResponsibilityRule
• Methods with no messages to self - See: UtilityMethod
• EmptyCatchClauses
• 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. -- MarkAufflick

Not actually the code:

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

Problems with the way the code is changing:

• Sporadic ChangeVelocity - Different rates of change in the same object.
• ShotgunSurgery - The same rate of change in different, disconnected objects.

Other code problems:

• 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. For example, 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.

From BadSmellsInCode?:

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

The list of canonical smells found in RefactoringImprovingTheDesignOfExistingCode can be found at: http://jexp.de/papers/refactoring/refactoring/node26.html (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 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, LanguageSmells
• 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.
• AbstractionDistraction - Use of different abstraction levels in the same code.