Refactoring the Law of Demeter

It is Friday, this means refactoring day!

Here is the code I tackled today:

class Crm::CreateLead

  attr_reader :username, :lead

  def initialize(username, lead)
    @username = username
    @lead = lead
  end

  def generate_data
    property = lead.property
    street = property && property.street && 
      property.street.encode(:xml => :text)
    city = property && property.city && 
      property.city.encode(:xml => :text)
    zipcode= property && property.zip && 
      property.zip.encode(:xml => :text)
    state= property && property.state && 
      property.state.encode(:xml => :text)
  end

  #There is more to the class ...

end

As you can see in the generate_data method, we continuously checked if there was a property and if the property had a specific attribute, if it did then we encoded it. Whenever you see this kind of foo && foo.something it is a warning sign that you might be doing something wrong, something doesn't belong or something is missing. In this case, I realized we were breaking the Law of Demeter. For an excellent post describing the law, you should read Demeter: It's not just a good idea, It's the law by Avid Grimm.

The culprit for all this mess was on line 11 property = lead.property. The problem was not that we were trying to access the property, it was that we were trying to perform actions on the attributes of the property (lines 12 through 15). In addition, and the bigger problem, is that lead.property returns a property object, while property.street returns a string object. In the words of Peter Van Rooijen (read Avdi's blog), we were not only trying to play with our toys but we were also trying to take them apart.

My first pass at cleaning this up was to just pass in the property. After all the law say that I can play with toys that were given to me.

class Crm::CreateLead

  attr_reader :username, :lead, :property

  def initialize(username, lead, property)
    @username = username
    @lead = lead
    @property = property
  end

  def generate_data
    street = property.street && property.street.encode(:xml => :text)
    city = property.city && property.city.encode(:xml => :text)
    zipcode= property.zip&& property.zip.encode(:xml => :text)
    state= property.state&& property.state.encode(:xml => :text)
  end

  #There is more to the class ...

end

This was a little better, but I still had to check if there is a street and then try to encode that string. To make this nicer I added some methods to the Property class.
class Crm::CreateLead
 
class Crm::CreateLead 
  attr_reader :username, :lead, :property

  def initialize(username, lead, property)
    @username = username
    @lead     = lead
    @property = property
  end

  def generate_data
    street  = property.encode_street
    city    = property.encode_city
    zipcode = property.encode_zip
  end

  #There is more to the class ...

end

class Property
  
  def encode_street
    @street.to_s.encode(:xml => :text)
  end

  def encode_city
    @city.to_s.encode(:xml => :text)
  end

  # more encode methods

end

Ahhh much nicer now.