December 30, 2008

Posted by John

Tagged refactoring

Older: The Rails Beatitudes

Newer: Look at the Size of My Footer

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.

10 Comments

  1. If we’re trying to make the code more readable, I’d replace

    
    def pastor?
      roles.select { |r| r.title.include?('Pastor') }.size &gt; 0
    end
    

    with

    
    def pastor?
      roles.any? { |r| r.title.include?('Pastor') }
    end
    
  2. @David – Agreed. Updated the article.

  3. Scott Raymond Scott Raymond

    Dec 30, 2008

    Using Enumerable#any? will also be faster (at least in some Ruby implementations), because once it comes across the ‘Pastor’ role, it won’t bother looking at the rest of the array.

  4. Thanks John, great post!

  5. @Scott – Good point.

    @Oren – Glad you liked it.

  6. Great read. It’s always good to come across an article like this once in a while to keep your coding style up to par. Even more when your day job is coding in PHP.

  7. Peter Barszczewski Peter Barszczewski

    Dec 31, 2008

    A great reminder of good programming practice. I go for more terse method names, so I would choose update_information? over needs_to_update_information? and update_address? over address_needs_updated? But that falls to personal preference …

  8. @Peter – Don’t tell anyone but the method in the app I’m working on is actually named update_information?. I exaggerated in the post to get my point across. :)

  9. Rich Sturim Rich Sturim

    Jan 01, 2009

    Kudos, a very nice example of refactoring that clarifies your intent without getting too cute with code compression tricks. I always try to keep in mind what the next developer in line will be thinking when they happen upon my code.

  10. Very usefull post! For my live examples are the best.

Sorry, comments are closed for this article to ease the burden of pruning spam.

About

Authored by John Nunemaker (Noo-neh-maker), a web developer and programmer who has fallen deeply in love with Ruby. More about John.

Syndication

Feed IconRailsTips Articles - An assortment of howto's and thoughts on Ruby and Rails.

Feed IconRails Quick Tips - Ruby and Rails related links.