Get a wiff of this - RailsConf 2016 - Sandi Metz

This is my 3rd post in my talk notes series.
Sandi Metz is a well known name. She has helped many teams refactor their systems from software rot. These are my notes on her RailsConf Talk about Code Smells.

Code Smells

"Code smells" is a term coined by Kent Beck for regularly observed patterns in code.

Martin Fowler wrote the "Refactoring" book on how to avoid duplication by encapsulating these patterns.

Code smells are grouped based on similar traits as follows:

Bloaters

• Long methods
• Large classes
• Data clumps
• Long parameter lists
• Primitive obsession

Tool Abusers

• Switch Statements
• Refused Bequest : A sub class overrides a base class method and throws an exception to denote that it doesn't implement that behavior.
• Alternative classes with different interfaces
• Temporary fields: Can be a method instead

Change Preventers

• Divergent Change: A change requires modifications at different branches of the same hierarchy.
• Shotgun Surgery
• Parallel inheritance hierarchies

It's fine sometimes that theirs friction to change. It depends on context.

Dispensable

• Lazy classes: classes that don't do enough
• Speculative Generality: Making abstractions for anticipated future usecases
• Data Class: Classes with coupled data and behavior
• Duplicated Code

Couplers

• Feature Envy: Sending more messages to other objects than yourself i.e. coupling
• Inappropriate Intimacy: Reaching into other object's private methods
• Message Chains: Send a message, receive an object, send a message to received object and it goes on. If the type of object changes in between then it's more concerning.
• Middle Man: An object with the sole purpose of routing messages to other objects.

Refactoring

Refactorings have name and they come with very specific concrete recipes.

Every code smell wraps to the curated refactoring recipe. This is a solved problem.

Smells to Refactorings Cheatsheet - Industrial Logic

Example

class Sale < Persistence
end

class Expense < Persistence
end

class Foo
  def sales_total(params)
    Sale.where(date:(Date.parse(params[:starting]))..(Date.parse(params[:ending]))).sum('cost')
  end
end

class Bar
  def weekly_sales_total(params)
    start_date = (Date.parse(params[:starting]))
    end_date = start_date + 6
    Sale.where(date:start_date..end_date).sum("cost")
  end
end

class Baz
  def expense_total(params)
    start_date = (Date.parse(params[:starting])) rescue Date.today
    end_date = (Date.parse(params[:ending]) rescue start_date
    Expense.where(date:start_date..end_date).sum("cost")
  end
end

Data Clump

This code has a data clump for date range, after doing the "extract class" refactoring the code looks like this.

class DateRange
  attr_reader :starting, :ending
  def initialiaze(starting:, ending: nil)
    @starting = Date.parse(starting) rescue Date.today
    @ending = Date.parse(ending) rescue @starting
  end

  def range
    starting..ending
  end

  def week_range
    starting..(starting + 6)
  end
end

class Sale < Persistence
end

class Expense < Persistence
end

class Foo
  def sales_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Sale.where(date:range).sum('cost')
  end
end

class Bar
  def weekly_sales_total(params)
    range = DateRange.new(starting: params[:starting]).week_range
    Sale.where(date:range).sum('cost')
  end
end

class Baz
  def expense_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Expense.where(date: range).sum("cost")
  end
end

Notice that the behavior is coalesced inside the class and is isolated in one place.

Message Chains

Foo, Bar and Baz have message chains i.e. they make call to the objects returned by Expense and Sale. Now these classes are coupled with the internal working of their dependencies.

When you are testing you should only need to mock the immediate collaborators of the object but in this case we'll have to mock the returned object. This generally means that we should extract pattern inside a method.

class DateRange
  attr_reader :starting, :ending

  def initialiaze(starting:, ending: nil)
    @starting = Date.parse(starting) rescue Date.today
    @ending = Date.parse(ending) rescue @starting
  end

  def range
    starting..ending
  end

  def week_range
    starting..(starting + 6)
  end
end

class Sale < Persistence
 def self.total(within:)
   where(date: within).sum("cost")
 end
end

class Expense < Persistence
 def self.total(within:)
   where(date: within).sum("cost")
 end
end

class Foo
  def sales_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Sale.total(within: range)
  end
end

class Bar
  def weekly_sales_total(params)
    range = DateRange.new(starting: params[:starting]).week_range
    Sale.total(within: range)
  end
end

class Baz
  def expense_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Expense.total(within: range)
  end
end

We have extracted the chain inside the total method.

Related: what you want to test is a trade off. You can only emulate / introduce so much reality inside your tests.

Rocky Mountain Ruby 2012 - To Mock or Not to Mock by Justin Searls

Coupling your tests to objects that are really far away then the test suite is going to get slow. If you couple your objects with slow things then your tests will get slower.

Using "Dependency Injection" we can mock the "roles" that the objects play rather than the class they are. which is how we should think when doing Object Oriented Programming.

Duplicate Code

Both Expense and Sale implement the same total method. This duplication can be eliminated by the "pull up method" refactoring. Monkey patching the Persistence class seems like a bad idea. So instead we pull out the logic inside a module and extend it from both the classes.

class DateRange
  attr_reader :starting, :ending
  def initialiaze(starting:, ending: nil)
    @starting = Date.parse(starting) rescue Date.today
    @ending = Date.parse(ending) rescue @starting
  end

  def range
    starting..ending
  end

  def week_range
    starting..(starting + 6)
  end
end

module Totalable
  def total(within:, date_field: :date, on: "cost")
    where({date_field => within}).sum(on)
  end
end

class Sale < Persistence
 extend Totalable
end

class Expense < Persistence
 extend Totalable
end


class Foo
  def sales_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Sale.total(within: range)
  end
end

class Bar
  def weekly_sales_total(params)
    range = DateRange.new(starting: params[:starting]).week_range
    Sale.total(within: range)
  end
end

class Baz
  def expense_total(params)
    range = DateRange.new(starting: params[:starting], ending: params[:ending]).range
    Expense.total(within: range)
  end
end

We have converted total to have a more generalized API. This can be seen as "Speculative Generality" but until the methods are small this is a good trade off as the flexibility is much more than the added complexity.

Link to the original talk

Do check out my other posts in the series :)