모델의 리팩토링 : 무엇이 잘못되고 있습니까?

Question

나는 현재이 초기 장황 코드를 건조 시키려고 노력하고 있습니다.

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

내 정직한 시도는 다음과 같습니다.

  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

그러나이 코드를 다음과 같이 콘솔에서 실행할 때 :

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

예상치 못한 결과를 얻습니다 ... 식물을 심을 필요는 없지만 여전히 두 작업 이름의 배열을 반환합니다 ([: plant_out, : sow_out

누구든지 내가 어떻게 이것을 청소하고 Wasksforweek 방법이 예상 결과를 반환하도록하는지 알려줄 수 있습니까?

티아

Answer 1

귀하의 방법은 결과를 반환합니다 task_names.each. each 항상 시작한 것을 반환합니다. 따라서 실제로 결과를 반환해야합니다.

또한, 당신은 당신을 재현하고 있습니다 to_do_this_week 루프의 모든 반복에 대한 배열은 깨끗하게 닦아냅니다.

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

아니면 이거:

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

그러나 나는 이것이 아마도 당신의 최고라고 생각합니다.

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

마지막으로 사용됩니다 find_all 배열 위로 반복되고 블록이 진정한 값을 반환하는 객체로 채워진 새 배열을 반환합니다.

마지막으로, 조건부 논리도 약간 미쳤다. 당신은 사용할 수 있습니다 [] 역동적 인 방식으로 활성 레코드 필드에 대한 액세서. 그리고 일반적으로 이중 음수 대신 긍정적 인 경우를 사용하는 것이 더 명확합니다. unless something.nil?. 그리고 이것이 범위를 만들기위한 일반적인 사용이라면,이를 방법으로 농장하는 것이 가장 좋습니다.

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)

전체 방법 만들기 :

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

위의 점에 주목해야 할 것은 그 것입니다 self[task_name] 데이터베이스에서 원시 데이터를 가져 와서 작성할 수있는 사용자 지정 게터 메소드를 무시하는 것 같습니다.

사용자 정의 getters를 사용하려고하거나 속성으로 취급하려는 방법이 있으면 사용할 수 있습니다. self.send(task_name) 대신에 self[task_name].

Answer 3

이것은 새로운 조건 메소드가있는 약간 수정 된 코드입니다.

  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