04 June 2009

Before | After

Before:



def self.find_expiring_soon(date_time = Time.now)
@@expiring_soon ||= find(:all, :conditions => ["type_id = 1 AND completed = 0 AND created_at < :date", { :date => (4.months + 14.days).ago(date_time) }])
end

def self.find_expiring_soon_with_donations(date_time = Time.now)
@@expiring_soon_with_donations ||= find(:all, :conditions => ["type_id = 1 AND completed = 0 AND donated_amount > 0 AND created_at < :date", { :date => (4.months + 14.days).ago(date_time) }])
end

def self.find_expiring_soon_without_donations(date_time = Time.now)
@@expiring_soon_with_donations ||= find(:all, :conditions => ["type_id = 1 AND completed = 0 AND donated_amount = 0 AND created_at < :date", { :date => (4.months + 14.days).ago(date_time) }])
end

def self.find_expiring_very_soon(date_time = Time.now)
@@expiring_very_soon ||= find(:all, :conditions => ["type_id = 1 AND completed = 0 AND created_at < :date", { :date => (6.months - 3.days).ago(date_time) }])
end



After:



def self.find_expiring_soon_with_donations
find(:all,
:include => :donations,
:conditions => [
"expires_soon = 0
#{with_expiring_criterias}
AND donated_amount > 0", { :date => four_months_and_a_half_ago }])
end

def self.find_expiring_soon_without_donations
find(:all,
:conditions => [
"expires_soon = 0
#{with_expiring_criterias}
AND donated_amount = 0", { :date => four_months_and_a_half_ago }])
end

def self.find_expiring_very_soon
find(:all,
:conditions => [
"expires_very_soon = 0
#{with_expiring_criterias}
", { :date => (6.months - 3.days).ago(date_time) }])
end

def self.find_expiring_now
find(:all,
:conditions => [
"expired = 0
#{with_expiring_criterias}", { :date => 6.months.ago }])
end

def self.with_expiring_criterias # DRY
"AND type_id = #{ElementType::MONEY}
AND completed = 0
AND activated_at < :date" end

private_class_method :with_expiring_criterias

def self.four_months_and_a_half_ago
@@FOUR_MONTHS_AND_A_HALF_AGO ||= (6.months - 6.weeks).ago(Time.now)
end

private_class_method :four_months_and_a_half_ago


  • Much more lines of code (LOC), does it improve readability and understading?
  • hardcoded values replaced by objects (ElementType::MONEY), does it make easier to understand the meaning of numbers without knowing the DB model
  • duplicate SQL DRyed in a private method
Which version do you prefer?

1 comment:

Marcus Derencius said...

Why not create a named_scope for the common criteria, including the default date? So all those finds become just calls to this scope.

named_scope expiring_soon, lambda {|date|
date ||= Date.now
{:conditions => ['type_id = ? and completed = ? and activated_at < ?', ElementType::MONEY, false, date]}
}