Definition (Code Smells) Code smells are common code anti-patterns which could and should be refactored.
This doc is summarized from Martin Flower's book Refactoring (the 2nd Edition).
- You may inverse previous refactorings as software grows
- You may change names of a function/parameter/varible/... as you learn
Go to a section of code smell, and the refactoring techniques to that smell are written in Pascal Case
under the Solution section. Refer to the book's catalog to get detailed examples and explainations for each refactoring technique.
- Mysterious Name
- Duplicated Code
- Long Function
- Long Parameter List
- Global Data
- Mutable Data
- Divergent Change
- Shotgun Surgery
- Feature Envy
- Data Clumps
- Primitive Obsession
- Repeated Switches
- Loops
- Lazy Element
- Speculative Generality
- Temporary Field
- Message Chains
- Middle Man
- Insider Trading
- Large Class
- Alternative Classes with Different Interfaces
- Data Class
- Refused Bequest
- Comments
Motivation
- A good name can save hours of puzzled incomprehension in the future
- When you can't think of a good name for something, it's often a sign of a deeper design malaise
Solution
Change Function Declaration
Rename Variable
Rename Field
Issue
- Duplication wastes time on comparing and modifying the duplicates
Solution
- Same expressions:
Extract Function
- Similar but not identical expressions:
Slide Statements
, thenExtract Function
- Duplicate fragments in subclasses of a common base class:
Pull Up Method
In our experience, the programs that live best and longest are those with short functions.
-- Martin Fowler and Kent Beck
Motivation
- Small function supports explanation, sharing, and choosing Prerequisite: good naming, So that you don't need to look at the function body to understand.
When to decompose functions
Heuristics, not rules.
- Whenever we feel the need to comment something
Solution
- For a single logic group:
Extract Function
- IF Non-pure:
Replace Temp with Query
- IF Long lists of parameters:
Introduce Parameter Object
;Preserve Whole Object
- IF still not resolved:
Replace Function with Command
- IF Non-pure:
- For conditional:
- For conditional expressions: Decomposed Conditional
- For big a switch statement:
Extract Function
on its legs - More than one switch statements switching on the same condition:
Replace Conditional with Polymorphism
- For loops:
- IF your loop is doing multiple things (can't be well named):
Split Loop
- ELSE: Extract the loop and its body into its own method.
- IF your loop is doing multiple things (can't be well named):
Issue
Long parameter list is better than using global data, but still can be confusing.
Solution
- IF a parameter can be derived by the other:
Replace Parameter with Query
- IF parameters were pulled out from an object:
Preserve Whole Object
- IF several parameters always fit together:
Introduce Parameter Object
- IF a parameter is a flag to dispatch different behavior:
Remove Flag Argument
- IF multiple functions share several parameter values:
Combine Functions into Class
Issue
The problem with global data is that it can be modified from anywhere in the code base, and there’s no mechanism to discover which bit of code touched it.
Forms of Global Data
- Global variables
- Class variables
- Singletons
Solution
Encapsulate Variable
Issue
- You can do harmful things by changing mutable data without realizing it.
- A rich source of confusion and bugs.
Solution
Encapsulate Variable
- IF a variable is being updated to store different things:
Split Variable
- IF there are side-effect-free code together with dirty code, THEN try to separate side-effect-free code:
Slide Statements
+Extract Function
- IF a function with side-effect has a return value: Separate Query from Modifier (Any function that returns a value should not have observable side effects)
- Always if possible:
Remove Setting Method
- IF a function modifies non-local data:
Replace Derived Variable with Query
- IF much code is needed to update a variable:
Combine Functions into Class
;Combine Functions into Transform
- IF an object contains data object in its property, replace instead of modifying it in place:
Change Reference to Value
Symptom
- You often change a single module in different ways for different reasons
Issue
- You have to always understand/remember more than you need to make a simple change
Cause
- The module has too many different responsibilities!
- Context boundaries are unclear in the early days of a program; Continue to shift as a software system's capabilities change.
Solution
- IF changes have different contexts: split the module into separate ones
- IF two aspects naturally form a sequence:
Split Phase
and have a clear data structure between phases - IF there is more back-and-forth in the calls: create appropriate modules, then split them by
Move Function
- IF one function has two types of processing: split them via
Extract Function
- IF the module is a class: split it via
Extract Class
The opposite of Divergent Change.
Symptom
- Every time you make a change, you have to make a lot of little edits to a lot of different classes
Issue
- You might forget to change somewhere
Solution
Move Function
andMove Field
, to put all the changes into a single module- IF a set of functions operate on similar data:
Combine Functions into Class
- IF a set of functions transform or enrich a data structure:
Combine Functions into Transform
- IF the common functions can combine their logic for a consuming phase of logic:
Split Phase
❓ Inline Function
orInline Class
, if possible, to avoid jumping around. (May even find more suitable split after putting them together)
Put things together that change together. (data and the behavior that references that data)
Symptom
- A function in one module spends more time communicating with functions or data inside another module than it does within its own module.
Motivation
When modularize a program, we want to:
- Maximize the interaction inside a zone
- Minimize the interaction between zones
Solution
- a function invoking half-a-dozen getter methods on another object to calculate some value: move the function to the data using
Move Function
- IF only a part of a function suffers from envy:
Extract Function
, thenMove Function
- IF a function envies several modules: break the function into several pieces by
Extract Function
, thenMove Function
. Or directly move to the module which the function envies the most. - Strategy and Visitor design pattern.
Symptom
- You see three or four data items together in lots of places: as fields in multiple classes, or as parameters in multiple functions.
Solution
- For repetitive class fields: use
Extract Class
to turn them into an object. - For repetitive function parameters: use
Introduce Parameter Object
orPreserve Whole Object
to slim them down.
Note
- Don't worry about data clumps that use only some of the fields of the new object. You'll benefit from it as long as you're replacing two or more fields with the new object.
- Test if your extraction makes sense: deleting one of the object properties makes the others become non-sense.
Benefit
- Improves readability: you can shrink a lot of parameter lists and simplify method calling
- Helps solving Feature Envy problem
Symptom
- You're using primitive data type to do something more than simple printing.
- Common cases: use string to store money, coordinates, ranges, phone numbers, etc.
Solution
- In most cases: Replace Primitive with Object
- IF the primitive acts as type code controlling conditional behavior:
Replace Type Code with Subclasses
, thenReplace Conditional with Polymorphism
Benefit
- Having a class makes it easier to put behavioral specific code
Symptom
- The same conditional switching logic pops up in different places
Issue
- Whenever you add a clause, you have to find and update all the duplicate switches
Solution
Replace Conditional with Polymorphism
Solution
- Replace Loop with Pipeline
Benefit
- Readability: Using pipelines help us quickly see the elements that are included in the processing and what is done with them.
Symptom
- A function whose content and name are equally easy to read
- A class/subclass that is essentially one simple function
Cause
- We want to add structure so badly
- You expect the function/class to grow in the future (YAGNI)
- The class/function was reduced after refactoring
Issue
- Unnecessary structures reduce code readability
Solution
Inline Function
, orInline Class
- For subclass:
Collapse Hierarchy
Symptom
- Have over-general purposed code because you think you will need it In the future (YAGNI)
Issue
- Often difficult to understand and maintain
Solution
- Abstracted classes that aren't doing much:
Collapse Hierarchy
- Unnecessary delegation:
Inline Function
,Inline Class
- Functions with unused/unneeded parameters:
Change Function Declaration
- Then only users of the function/class are test cases: delete test, then Remove Dead Code
Symptom
- A field of a class is set only in certain circumstances
Issue
- Code is difficult to understand since you will wonder why a property isn't needed
Solution
- For the poor orphan fields:
Extract Class
; then move related function withMove Function
- For conditional code: Introduce Special Case
Symptom
- A client asks one object for another object, which asks for yet another object, and so on
Issue
- The client is coupled to the structure of the navigation: any change to the intermediate object/structure means the client has also to change
Solution
Hide Delegation
. (May create Middle Man)- Even better: observe, and see if you can
Extract Function
andMove Function
to avoid the chain
Symptom
Half of a class's methods are delegating to another class.
Solution
Remove Middle Man
- ❓IF only a few class methods are not delegating: use
Inline Function
to inline them into the caller - If there is additional behavior other than delegation:
Replace Superclass with Delegate
;Replace Subclass with Delegate
Symptom
- Modules are trading data around too much
Issue
- Increased coupling
Solution
- Modules whisper to each other: separate them by
Move Function
andMove Field
- Modules have common interests: create a third module for the commonality, or Hide Delegate
Replace Subclass with Delegate
orReplace Superclass with Delegate
Symptom
- A class has too many fields
- A class has too much code
Root Cause
- The class is doing too much
Solution
- IF some variables make sense to be in a component:
Extract Class
- Common prefixes/suffixes are also a sign of grouping
- IF the component makes sense with inheritance:
Extract Superclass
orReplace Type Code with Subclasses
- IF a class has too much code: find and remove duplicated code
Issue
- You want different classes to be able to substitute with each other.
Solution
Change Function Declaration
to make functions match upMove Function
to move behavior into classes until they match- IF leads to duplication:
Extract Superclass
Symptom
- A class has fields, getters and setters for the fields, and nothing else
Issue
- Data classes are often being manipulated in far too much detail by other classes
Root Cause
- Usually: You put the behavior on the data in the wrong place: they should be moved into the data class itself.
- Exception: the result is being used by a distinct function invocation (for example, client)
Solution
Encapsulate Record
Remove Setting Method
on any filed that should not be changed- IF these getters and setters are used by other classes:
Move Function
to move behavior into the data class. IF not directly movable:Extract Function
on the movable part first.
Symptom | Root Cause | Solution |
---|---|---|
Subclasses don't need the methods and data of their parents | The hierarchy is wrong | Create a new sibling class, then Push Down Method and Push Down Field |
Subclasses reuse the behavior but does not want to support the superclass's interface | Replace Subclass with Delegate Replace Superclass with Delegate |
In most cases: comments are there because the code is bad.
Issue
- Comments rot
- Comments are a deodorant for bad code
Solution
- IF you need to explain what a block of code does:
Extract Function
- IF you need to explain what a method does:
Change Function Declaration
to rename the function - IF you need to state some rules about the required state of the system:
Introduce Assertion
Exceptions
- Comments can indicate areas in which you aren't sure. You explain why you did something, and the comment will be helpful for future modifiers.