TheDailyRefactoring.com Blog

Consolidate Duplicate Delegation (new)

Tue Oct 14 15:01:13 -0400 2008

Consolidate Duplicate Delegation (rails)

Group alike delegated attributes and make them into one statement

By COREY GRUSDEN and SANDRO TURRIATE

Published: 03:01 PM EDT

This bears some resemblance to Replace Inheritance With Delegation and is for Rails-only. This can also be considered a Rails-idiom. In Rails, calls can be delegated to an object by doing the following:

class SubjectMatterExpert
  delegate :awards, :to => :profile
  delegate :recommendations, :to => :profile
  delegate :address, :to => :address
  delegate :grades, :to => :profile
  delegate :schools_attended, :to => :profile
end

Most of the items above are delegated to the profile attribute. This is probably due to copy-and-pasting lines to “save time”. With Consolidate Duplicate Delegation we’re aiming to clean up some of those lines to remove duplication by utilizing the language.

First group the alike delegations together:

class SubjectMatterExpert
  delegate :address, :to => :address

  # all these calls going to profile
  delegate :awards, :to => :profile
  delegate :recommendations, :to => :profile
  delegate :grades, :to => :profile
  delegate :schools_attended, :to => :profile
end

Next, put them all inline and have them all goto profile:

class SubjectMatterExpert
  delegate :address, :to => :address

  delegate :awards, 
           :recommendations, 
           :grades, 
           :schools_attended, :to => :profile
end

Pull Up Method

Mon Oct 13 15:45:29 -0400 2008

Pull-Up Method (ruby)

By COREY GRUSDEN and SANDRO TURRIATE

Published: 03:45 PM EDT

With the concept of Skinny Controller, Fat Model a lot can be missed when moving data around into your models. A really good indicator of logic that can be put into the parent class is if you have a method that is changing self variables.

class SubjectMatterExpert < User
 include HashCodeCreatorModule
 def make_activation_code
   self.deleted_at = nil
   self.activation_code ||= make_hash_code
 end
end

class Administrator < User
 include HashCodeCreatorModule
 def make_activation_code
   self.deleted_at = nil
   self.activation_code ||= make_hash_code
 end
end

We were able to reduce the code in the 2 classes SubjectMatterExpert and Administrator by using the Pull-Up Method to move the methods into the parent class User.

class SubjectMatterExpert < User
end

class Administrator < User
end

class User
 include HashCodeCreatorModule
 def make_activation_code
   self.deleted_at = nil
   self.activation_code ||= make_hash_code
 end

Now our code still works in our tests:

it 'should create an activation code' do
  admin = Factory(:administrator)
  admin.make_activation_code
  admin.activation_code.should_not be_empty
end

Warning: Luckily this method was very simple to Pull-Up. We ran across another method that we wanted to Pull-Up, but it had calls to a State-Machine event. So pay attention as to what is going on in the method.

Consolidate Conditional Expression

Fri Oct 10 17:14:12 -0400 2008

Consolidate Conditional Expression (Simple)

By COREY GRUSDEN and SANDRO TURRIATE

Published: 05:14 PM EDT

While trying to figure out the logic that was going on in a very important method, we decided to make an attempt on refactoring some of the expressions into something we could actually grok.

The following method was the culprit (yes, I realize the names are bad, but obfuscation is king):

def password_required?
   if !new_special_sme? && (from_intl_expert_profile? || recently_forgot_password? || ((new_record? || password_reset_code.blank?) and special_sme?))
     return false
   elsif !password_reset_code.blank? and ((special_sme? and terms_accepted?) or new_special_sme?)
     return true
   else
     return !special_sme? && user_password_required?
   end
end

The line that we’re concerned about currently is the last return:

   else
     return !special_sme? && user_password_required?
   end

We extract it out into it’s own method:

  def not_special_sme_and_requires_password
    !special_sme? && user_password_required?
  end

And replace the conditional statement with the method we just made:

def password_required?
   if !new_special_sme? && (from_intl_expert_profile? || recently_forgot_password? || ((new_record? || password_reset_code.blank?) and special_sme?))
     return false
   elsif !password_reset_code.blank? and ((special_sme? and terms_accepted?) or new_special_sme?)
     return true
   else
     return not_special_sme_and_requires_password
   end

With that small refactoring, we were able to replace quite a few statements in other areas of our code to clean it up and put the logic in one spot.

Replace Loop with Closure Collection Method

Tue Oct 07 14:30:01 -0400 2008

Cleaning up Flash Messages in our Views

From Ruby Refactoring: Replacing a Loop with Closure Collection Method

By COREY GRUSDEN, SANDRO TURRIATE, and ZACH INGLIS

Published: 02:30 PM EDT

We had conditional code that was appearing in multiple views and getting rid of it was a high priority to us since duplicated code is known as a Code Smell and does more harm than good. So we decided to move the code into a Module and call the method from there.

  def render_flashes
    flashes = ""
    [:notice, :error].each do |type|
      unless flash[type].nil?
        flashes += content_tag('div', flash[type], :id => "flash_#{type}", :class => type)
      end
    end
    flashes
  end

We then realized that we could use Replacing a Loop with Closure Collection Method to get rid of the local variables as well as utilize the power of Ruby’s enumeration closures.

The .collect method will build an array from what’s returned from the block instead of concatenating a string from content_tag
Removing the flash[type].nil? is possible since enumerating values in the hash will not return anything if values are nil
When the code is output to the view, it will be nicely formatted underneath since we can call .join on the array that is created from .collect

  def render_flashes
    flash.collect do |key, message|
      content_tag :div, message, :id => "flash_#{key}", :class => key
    end.join('\n')
  end

Extract Method

Tue Oct 07 10:28:42 -0400 2008

Removing the duplication of Flash Messages

From Refactoring: Extract Method

By COREY GRUSDEN, SANDRO TURRIATE
Published: 10:17 AM EDT

We had the following conditional code that was appearing in multiple views, so we wanted to cut down on the duplication and be able to modify the code in one spot.

We used Extract Method on the following code:

  - if flash[:notice]
    .notice
      = flash[:notice]

and moved it into a module:

  module ViewHelper
    def render_flashes
      flashes = ""
      [:notice, :error].each do |type|
        unless flash[type].nil?
          flashes += content_tag('div', flash[type], :id => "flash_#{type}", :class => type)
        end
      end
      flashes
    end
  end

Looks like a lot of extra functionality that we added when we used extract method, right? It is. But now we can replace the code in our views after we mixin our new module into the class that is rendering our view and make changes in one spot instead of across multiple views.

  = render_flashes