Get a wiff of this - RailsConf 2016 - Sandi Metz
Tags: oops, refactoring
Published: 2019-03-09 01:19:30

---
title: Get a wiff of this - RailsConf 2016 - Sandi Metz
published: true
description: Notes from Sandi Metz's "Get a wiff of this" Tech talk at RailsConf 2016
series: Conference Talk Notes
tags: OOPS, refactoring
---
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](https://www.industriallogic.com/blog/smells-to-refactorings-cheatsheet/)

# Example

```ruby
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.

```ruby
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.

```ruby
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](https://www.youtube.com/watch?v=TU3glG08BJI)

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.

```ruby
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](https://www.youtube.com/watch?v=PJjHfa5yxlU)

Do check out my other posts in the series :)