Passionate Development

Refactoring Using Contracts

At my previous workplace, my colleague Chuck was a strong proponent of using Ruby Contracts. The gem is inspired Design By Contract

I didn’t see the point back then, it feels like we are shoehorning a statically typed language feature into Ruby. But I have come around now, especially after working at Fairfax Media (FFX). Given the size of FFX codebase as well as the number of developers working on it - having contracts on the right places helps with code clarity.

Let’s look at an example below where we fixed a bug, refactored the code and added Contracts to make the code clearer.

We have an EmployeeService wth employee? method, which is used to check whether someone is an employee of the company or not. For the purpose of this example, an employee is simply someone with email domain of passionatedevelopment.com.

class EmployeeService
  attr_reader :email_address

  DOMAINS_REGEXP = ['passionatedevelopment.com']

  def initialize(input)
    @email_address = case
                     when input.respond_to?(:email)
                       input.email
                     when input.respond_to?(:email_address)
                       input.email_address
                     else
                       input
                     end
  end

  def employee?
    domain = Mail::Address.new(email_address).domain
    DOMAINS_REGEXP.any? { |whitelisted_domain| whitelisted_domain.match(domain) } if domain
  end
end

There is a bug in this class as I mentioned earlier, the bug is in the following line in the employee method.

domain = Mail::Address.new(email_address).domain

The code expects that email_address is a String but after debugging, we found that it is not always the case.

In one occassion, the email_address is of class Nori::StringWithAttributes (coming from a member object serialized from an XML - but anyway let’s not go too far).

And when email_address is not a String, calling the Mail::Address.new(email_address).domain would just simply returns nil.

Well to be honest, looking at the initialize code above, you can be forgiven to assume that email_address is a String, but as the saying goes - do not assume as it makes an … you get my point. This is probably one of the few place where I see duck typing bites.

Anyway, now that we know where the bug is - let’s start refactor the code.

First, let’s extract the setting of @email_address inside initialize method into its own method, this will allow us to do 2 things:

  • Test the method in isolation
  • Put the contract around it

Now the code looks like this:

class EmployeeService
  attr_reader :email_address

  DOMAINS_REGEXP = ['passionatedevelopment.com']

  def initialize(input)
    @email_address = extract_email_address_from(input)
  end

  def employee?
    domain = Mail::Address.new(email_address).domain
    DOMAINS_REGEXP.any? { |whitelisted_domain| whitelisted_domain.match(domain) } if domain
  end

  private

  def extract_email_address_from(input)
    case
    when input.respond_to?(:email)
      input.email
    when input.respond_to?(:email_address)
      input.email_address
    else
      input
    end
  end
end

And then lets clarify with contracts, the interface of extract_email_address_from method.

Contract Contracts::Or[Member, SerializedMember, String] => Contracts::Or[String, Nori::StringWithAttributes]
def extract_email_address_from(input)
  case
  when input.respond_to?(:email) # Member
    input.email
  when input.respond_to?(:email_address) # SerializedMember
    input.email_address
  else
    input # String
  end
end

And now the bug fix, let’s call to_s on the result of the conditional as both String and Nori::StringWithAttributes responds to it. 

  Contract Contracts::Or[Member, SerializedMember, String] => Contracts::Or[String]
  def extract_email_address_from(input)
    email = case
            when input.respond_to?(:email)
              input.email
            when input.respond_to?(:email_address)
              input.email_address
            else
              input
            end
    email.to_s
  end

I like that code so much better now as it is clear that the method will return a string (and if it doesn’t Contracts gem will throw an error).

We could a little bit further - removing calls to respond_to? and simply use Ruby’s idiomatic use of || coupled with Rails’ try 

Contract Contracts::Or[Member, SerializedMember, String] => Contracts::Or[String]
def extract_email_address_from(input)
  (input.try(:email) || input.try(:email_address) || input).to_s
end

Oh, while we are at it - let’s refactor employee? and make its return stricter (boolean only).

Contract Contracts::None => Contracts::Boolean
def employee?
  domain = Mail::Address.new(email_address).domain
  return false unless domain
  DOMAINS_REGEXP.any? { |whitelisted_domain| whitelisted_domain.match(domain) }
end

And then the final code is:

class EmployeeService
  attr_reader :email_address

  DOMAINS_REGEXP = ['passionatedevelopment.com']

  def initialize(input)
    @email_address = extract_email_address_from(input)
  end

  Contract Contracts::None => Contracts::Boolean
  def employee?
    domain = Mail::Address.new(email_address).domain
    return false unless domain
    DOMAINS_REGEXP.any? { |whitelisted_domain| whitelisted_domain.match(domain) }
  end

  private

  Contract Contracts::Or[Member, SerializedMember, String] => Contracts::Or[String]
  def extract_email_address_from(input)
    (input.try(:email) || input.try(:email_address) || input).to_s
  end
end

Of course - we have specs to help us with the refactoring.

I hope I have shown you the value of having Contracts to help clarify the intent of your code (and especially if it is coupled with well written specs).

And that is it - happy coding!