Refactoring in model: what's going wrong?

Question

I'm currently trying to DRY up this initial verbose code:

def planting_dates_not_nil?
    !plant_out_week_min.blank? || !plant_out_week_max.blank? || !sow_out_week_min.blank? || !sow_out_week_max.blank?
  end

  def needs_planting?(week)
    if !plant_out_week_min.blank? && !plant_out_week_max.blank?
      (plant_out_week_min..plant_out_week_max).include? (week) 
    end
  end

  def needs_sowing?(week)
    if !sow_out_week_min.blank? && !sow_out_week_max.blank?
      (sow_out_week_min..sow_out_week_max).include? (week)
    end 
  end

  def needs_harvesting?(week)
    if !harvest_week_min.blank? && !harvest_week_max.blank?
      (harvest_week_min..harvest_week_max).include? (week) 
    end
  end

here's my intial attempt:

  def tasks_for_week(week,*task_names)
    task_names.each do |task_name|
      to_do_this_week = []
        unless read_attribute(task_name).nil?
          if (read_attribute("#{task_name}_week_min")..read_attribute("#{task_name}_week_max")).include? (week)
            to_do_this_week << task_name
          end
        end
      end
  end

However, when I run this code in the console as follows:

p.tasks_for_week(Date.today.cweek, :plant_out, :sow_out])

I get an unexpected result...even though the plant does not need to be planted out, I still get an array of both task names returned ( [:plant_out, :sow_out]

Can anyone let me know how I'd clean this up and have the tasksforweek method return the expected results?

TIA

Answer 1

Your method is returning the result of task_names.each. each always returns what it started with. So you need to actually return your result.

Also, you are recreating you to_do_this_week array on every iteration of the loop, which would wipe it clean.

def tasks_for_week(week, *task_names)
  to_do_this_week = []
  task_names.each do |task_name|
    if some_condition
      to_do_this_week << task_name 
    end
  end
  to_do_this_week
end

Or this:

def tasks_for_week(week, *task_names)
  returning [] do |to_do_this_week|
    task_names.each do |task_name|
      if some_condition
        to_do_this_week << task_name 
      end
    end
  end
end

But I think this is probably your best best:

def tasks_for_week(week, *task_names)
  task_names.find_all do |task_name|
    some_condition
  end
end

That last one uses find_all which iterates over an array and will return a new array populated with any objects that the block return a true value for.

Lastly, your conditional logic is a bit crazy too. You can use the [] accessors for active record fields in a dynamic way. And it's usually clearer to use the positive case instead of the double negative of unless something.nil?. And if this is a common use for creating a range, it might be best to farm that out to a method:

def week_range_for_task(task)
  self["#{task_name}_week_min"]..self["#{task_name}_week_max"]
end

...

self[task_name] && week_range_for_task(task_name).include?(week)

Making the whole method:

def tasks_for_week(week, *task_names)
  task_names.find_all do |task_name|
    self[task_name] && week_range_for_task(task_name).include?(week)
  end
end

Answer 2

One thing to note with the above is that self[task_name] seems to get the raw data from the database, ignoring any custom getter methods you may written.

If you want to use custom getters or if you have any methods you'd like to treat as attributes, you can use self.send(task_name) instead of self[task_name].

Answer 3

This is the slightly amended code with a new condition method in there.

  def whole_range_exists?(method_name)
        self["#{method_name}_week_min"] && self["#{method_name}_week_max"]
      end

      def week_range_for_task(task_name)
        self["#{task_name}_week_min"]..self["#{task_name}_week_max"]
      end

      def tasks_for_week(week, *task_names)
        task_names.find_all do |task_name|
          whole_range_exists?(task_name) && week_range_for_task(task_name).include?(week)
        end
      end