Refactoring: Bad Smells

« previous | Chapter 3: Bad Smells | next »

Duplicated Code

Same code structure exists in more than one place or two sections of code perform same task.

• Duplicated code is identical: Extract Method & Pull Up Method
• Code is similar, but not the same: Form Template Method
• Two sections of code do same thing, but differently: Substitute Algorithm using clearer algorithm
• Duplication occurs in middle of a method: Extract Surrounding Method
• Duplicated code across two classes or modules: Extract Class / Extract Module or refer to appropriate method in other class.

Long Method

Function goes on for lines and lines; generally more than one screen of code

• Extract Method
• Many temporary variables: Replace Temp with Query or Replace Temp with Chain
• Many passed arguments to an extracted method: Introduce Parameter Object and Method Object
• Lots of loops and conditionals: Decompose Conditional, Replace Loop with Collection Closure Method, and potentially Extract Method on call to closure method and closure itself

Large Class

Class is trying to do too much: too many instance variables

• Instance variables would make sense as their own object: Extract Class or Extract Subclass (derived)
• Instance variables are related, but not sub-grouped: Extract Module

Long Parameter List

Function needs too many input parameters in order to function

• Data in one parameter can be obtained by requesting an already-known object: Replace Parameter with Method
• Variables in parameter "belong to" an object: Preserve Whole Object
• Variables in parameter would make sense as an object: Introduce Parameter Object or Refactoring: Refactorings/Introduce Named Parameter

Divergent Change

Making a change to the code is difficult: instead of jumping to place where change should be made, one small change results in the modification of several methods

• Identify what changes each time, then use Extract Class to tie them together

Shotgun Surgery

Adding a feature or change results in modification of several classes or methods all over the place.

• Move Method & Move Field to put changes into class (create one if necessary)
• Inline Class

Feature Envy

A function uses so many getter functions of a different class to calculate a value (it wishes it were in that class)

• Whole function is envious: Move Method
• Part of function is envious: Extract Method then Move Method

Data Clumps

Similar data structures/items found in many places (i.e. instance variables, parameters)

• Data members should belong to their own class: Extract Class
• Parameters often passed together: Introduce Parameter Object or Preserve Whole Object

Primitive Obsession

Code has heavy use of primitive data types rather than use of objects

• Replace Data Value with Object
• Conditionals depend on a type code: Replace Type Code with Polymorphism, Replace Type Code wth Module Extension, or Replace Type Code with State/Strategy
• Group of primitive instance variables that should go together: Extract Class
• Group of primitive parameters: Introduce Parameter Object
• Array keeps getting "picked apart": Replace Array with Object

Case Statements

Use of cases or conditionals in several areas

• Extract Method to isolate cases; Move Method to put in correct class; then Replace Type Code with Polymorphism, Replace Type Code wth Module Extension, or Replace Type Code with State/Strategy
• Cases affect a single method and aren't expected to change: Replace Parameter with Explicit Methods
• One conditional cases is a null: Introduce Null Object

Parallel Inheritance Hierarchies

Similar to shotgun surgery: making subclass of one class requires you to make a subclass of another class. Classes usually have same prefixes in each class derivative/inheritance hierarchy.

• Move Method and Move Field until one hierarchy disappears

Lazy Class

Class doesn't do enough to be "worth it's cost of upkeep" (Think: Class = $$$). Usually result of refactoring

• If subclasses or modules are lazy: Collapse Hierarchy
• Lazy components in general: Inline Class or Inline Module

Speculative Generality

Result of future features that never took shape: "Oh, I think we need the ability to this kind of thing someday. And this, and this, and this..." Machinery just gets in the way!

• Incomplete subclasses/modules that are used elsewhere: Collapse Hierarchy
• Unnecessary delegation (semi-inheritance): Inline Class
• Methods with unused parameters: Remove Parameter
• Meaningless method names: Rename Method

Temporary Field

Instance variable used only under certain conditions. Objects should need/use all of their variables

• Create a home for "poor orphan variables": Extract Class
  • Special case: Instance variables required only for one method: Create a Method Object
• Perhaps eliminate conditional code: Introduce Null Object

Message Chains

Client asks an object for another object, which then asks for another object, etc. to get to a function. Shows up as a long line of get_this methods or a lot of temps

• Hide Delegate Be careful! avoid turning objects into middle-men
  • Extract Method, then Move Method down chain to where its needed.

Middle Man

Encapsulation of private variables (hiding them from rest of world) gone too far: one class's methods delegate to another class

• Most/all methods delegate: Remove Middle Man (Talk to object that really knows what's going on)
• Only some methods delegate: Inline Method
• Additional behavior required: Replace Delegation with Hierarchy

Inappropriate Intimacy

"Sometimes classes become far too intimate and spend too much time delving into each other's private parts." — enough said.

• Break up these "lover" classes: Move Method and Move Field
• Change Bidirectional Association to Unidirectional
• Classes have some things in common: Extract Class (move commonalities into safe place) -OR- Hide Delegate
• Subclasses need to "leave home": Replace Inheritance with Delegation

Alternative Classes with Different Interfaces

Two classes do the same thing, but have different calls

• Rename Method, Move Method
• Too much Renaming and Moving code: Extract Module or Introduce Inheritance

Incomplete Library Class

Certain algorithms/behaviors of one class need to be available to other classes

• Move Method into library

Data Class

Classes that have attributes, but nothing else; manipulated too much by other classes

• Some variables that shouldn't be touched are being modified: Remove Setting Method
• Collection instance variables: Encapsulate Collection

Refused Bequest

Subclasses don't use/need inherited data and function members or doesn't want to support public methods

• Hierarchy is wrong: Push Down Method
• Subclass needs to be "gutted": Replace Inheritance with Delegation

Comments

Comments that are used to explain confusing code. By all means, write comments, but only when necessary (questions or concerns)

• Make a self-explanatory function name: Extract Method or Rename Method
• Rules about required state of system: Introduce Assertion

Metaprogramming Madness

Ruby's dynamic nature is misused: classes aren't defined in their own files, but scattered across many files. Key sign: method_missing

• Remove method_missing: Replace Dynamic Receptor with Dynamic Method Definition or Extract Method
• If method_missing is needed: Isolate Dynamic Receptor

Disjointed API

Many configuration options for given Library API methods

• Same configuration options often used over and over: Introduce Gateway
• Make API interaction more fluent: Introduce Expression Builder

Repetetive Boilerplate

Example: attr_reader (read-only), attr_accessor (read/write), and attr_writer (write-only) — getters and setters are so common in object-oriented languages that Yukihiro Matsumoto, creator of Ruby, decided to use shorthand to creating them.

• Code is simple enough to annotate (call method from class definition like attr_reader): Introduce Class Annotation