Rifattorizzare nel modello: cosa c'è di sbagliato?

Question

Al momento sto cercando di prosciugare questo codice verbose iniziale:

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

Ecco il mio tentativo intial:

  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

Tuttavia, quando si esegue questo codice nella console come segue:

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

ottengo un risultato inaspettato ... anche se la pianta non ha bisogno di essere piantato fuori, ho ancora una serie di entrambi i nomi delle attività restituito ([: plant_out,: sow_out]

Qualcuno può farmi sapere come avrei pulire questo e hanno il metodo tasksforweek restituisce i risultati attesi?

TIA

Answer 1

Il tuo metodo sta tornando il risultato di task_names.each. each restituisce sempre ciò che è iniziato con. Quindi è necessario tornare in realtà il risultato.

Inoltre, si sta ricreando si to_do_this_week serie su ogni iterazione del ciclo, che avrebbe pulirlo.

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

O questo:

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

Ma penso che questo è probabilmente la migliore migliore:

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

Questo ultimo utilizza find_all che itera su un array e restituisce un nuovo array popolato con oggetti che il blocco restituisce un valore vero per.

Infine, la logica condizionale è un po 'folle troppo. È possibile utilizzare le funzioni di accesso per i campi [] record attivo in modo dinamico. E di solito più chiaro di utilizzare il caso positivo invece del doppio negativo unless something.nil?. E se questo è un uso comune per la creazione di una serie, potrebbe essere meglio per coltivare che su un metodo:

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)

Fare l'intero metodo:

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

Una cosa da notare con quanto sopra è che self[task_name] sembra ottenere i dati grezzi dal database, ignorando i metodi getter personalizzato potrebbe essere scritta.

Se si desidera utilizzare getter personalizzati o se avete dei metodi ti piacerebbe trattare come gli attributi, è possibile utilizzare al posto di self.send(task_name) self[task_name].

Answer 3

Questo è il codice leggermente modificato con un nuovo metodo di condizione in là.

  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