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.
Do check out my other posts in the series :)
Top comments (0)