Refactoring in model: what's going wrong?
-
23-08-2019 - |
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
Solution
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
OTHER TIPS
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]
.
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