Move It To The Model and Use Tiny Methods

I’m updating an application that I first wrote two years ago and last updated one year ago. It has been amazing to see how much I have learned in the past two years. One of the requirements of the application is for the users to update their information each year. Just last year, I had code like this. Before you look at the code below, I want to warn you that it is not for the faint of heart. Never, ever do this.

<%- if current_user.roles.select { |r| r.title.include?('Pastor') }.size > 0 && 
    (current_user.email.blank? || current_user.address.blank? || 
     current_user.city.blank? || current_user.state.blank? || 
     current_user.zip.blank? || updated_at.year != Time.now.year) -%>
  <!-- show the update form -->
<%- else -%>
  <!-- show the list of forms to be completed -->
<%- end -%>

I would not lie to you. It was literally like that. A year later, I can barely tell what I was trying to do here. Imagine someone who didn’t know the application inside and out trying to work with code like that. Yikes! Here is my refactored version, that is far more descriptive.

<%- if current_user.needs_to_update_information? --%>
  <!-- show the update form -->
<%- else -%>
  <!-- show the list of forms to be completed -->
<%- end -%>

The benefit of this implementation is that any programmer can immediately tell what my intent is and that all the logic of determining whether or not the current user needs to update their information is moved to the model, where it belongs and can more easily be tested. So what are the steps that I took to get to that new method? First, I changed the if statement to the one above and then I moved all the logic into the user model like this.

class User
  def needs_to_update_information?
    roles.select { |r| r.title.include?('Pastor') }.size > 0 && 
      (email.blank? || address.blank? || 
         city.blank? || state.blank? || zip.blank? || 
         updated_at.year != Time.zone.now.year)
  end
end

This is only step 1. We are not done by any means. I can assure you that if you leave a method like this in your code, I will verbally attack you if I come across it. Let’s break this method down a bit, starting with the first conditional.

roles.select { |r| r.title.include?('Pastor') }.size > 0

This is horribly nondescript. What is my intent? Only pastors need to update their information and that is what this conditional checks. How about we add a pastor? method and use that in place. Also, I tweaked the condition to use Enumerable’s any? method (as suggested by David).

class User
  def needs_to_update_information?
    pastor? && 
      (email.blank? || address.blank? || 
         city.blank? || state.blank? || zip.blank? || 
         updated_at.year != Time.zone.now.year)
  end
  
  def pastor?
    roles.any? { |r| r.title.include?('Pastor') }
  end
end

That is much better, but why stop there? The next chunk of code that I see that we can pull out is the address stuff.

address.blank? || city.blank? || state.blank? || zip.blank?

The intent of these conditionals is to make sure that the user’s address is not blank, so I created an address_needs_updated? method.

class User
  def needs_to_update_information?
    pastor? && 
      (email.blank? || address_needs_updated? || 
         updated_at.year != Time.zone.now.year)
  end
  
  def pastor?
    roles.any? { |r| r.title.include?('Pastor') }
  end
  
  def address_needs_updated?
    address.blank? || city.blank? || state.blank? || zip.blank?
  end
end

Next up, I addressed updated_at.year != Time.zone.now.year. The intent of this conditional is to see if the user’s record has been updated in the current year. I pulled this out into an updated_this_year? method.

class User
  def needs_to_update_information?
    pastor? && (email.blank? || address_needs_updated? || !updated_this_year?)
  end
  
  def pastor?
    roles.any? { |r| r.title.include?('Pastor') }
  end
  
  def address_needs_updated?
    address.blank? || city.blank? || state.blank? || zip.blank?
  end
  
  def updated_this_year?
    updated_at.year == Time.zone.now.year
  end
end

That is probably about as far as I would go with this. So what are the benefits of all these changes?

The Benefits

• Moved business logic to the model where it is easier and more fun to test.
• More descriptive as to the intent of what is happening. Intent is really important. It is far more important for someone to understand why you are doing something than how you are doing something. There are many ways to do something, but there is only one intent.
• Smaller methods are easier to test than larger ones (and more fun). Testing large methods feels burdensome and often doesn’t provide enough coverage.
• Did I mention testing? It is easier and more fun to test models than views and small methods than large methods (yes, testing can be fun).

Getting back into this app, I learned that I have learned a lot in the past few years and I hope the error of my ways was beneficial for you too. I am in no way a refactoring expert and haven’t read the book on it, but if you are curious about refactoring, the techniques I used above appear to be Decompose Conditional and Extract Method.

Note: I’ve written two simple JavaScript refactoring articles over at Addicted To New. The first is on introducing explaining variables and the second is on moving functions into an object.