Rails Group_By Антипаттерн

Question

Я часто сталкиваюсь с запахом кода, когда использую метод Enumerable group_by. Некоторый старый код, который я рефакторинг, является хорошим примером

def punctuality_report
  params[:date] ? @date = Date.strptime(params[:date], "%m/%d/%Y") : @date = Date.today
  params[:through] ? @through = Date.strptime(params[:through], "%m/%d/%Y") : @through = @date + 1
  time_range = @date.to_formatted_s(:db)..@through.to_formatted_s(:db)
  @orders = Order.daily.includes(:store).where('orders.created_at' => time_range).group_by{|o| o.store}
  @orders.each_key.each do |s|
    eval "@s#{s.id}_early = @orders[s].collect{|o| o if o.early?}.compact"
    eval "@s#{s.id}_avg_early = @s#{s.id}_early.count > 0 ? @s#{s.id}_early.collect{|o| o.earliness}.sum / @s#{s.id}_early.count : '0'"         
    eval "@s#{s.id}_late = @orders[s].collect{|o| o if o.late?}.compact"
    eval "@s#{s.id}_avg_late =  @s#{s.id}_late.count > 0 ? @s#{s.id}_late.collect{|o| o.lateness}.sum / @s#{s.id}_late.count : '0'"         
    eval "@s#{s.id}_on_time = @orders[s] - (@s#{s.id}_early | @s#{s.id}_late)"
  end
end

Хорошо, я прохожу через это и ясно вижу, что нам нужно реорганизовать этот отчет из действия на контроллере заказов в собственную модель для очистки этой логики реализации. Там один запах кода, но я все еще борюсь с этим хешем orders.group_by.

Дело в том, что когда я нахожусь в слое вида, я действительно хочу этот хэш. Мне нужно получить сводку по заказам, но мне нужен доступ к магазинам. Использование метода группового запроса в Activerecord просто возвращает мне отношение, которое не так полезно, как перечисляемый хеш group_by. Я чувствую, что есть лучший способ получить то, что мне нужно, и уменьшить объем обработки запросов и рубинов.

Answer 1

Я не вижу ничего плохого в методе group_by. Я действительно вижу проблему со злоупотреблением eval, хотя [-; Eval - гораздо более сильный запах кода (ИМХО), чем group_by

Как говорится, я видел некоторые другие области для рефакторинга:

# Consider this refactor:

def punctuality_report
  # I find this slightly more readable
  @date = (params[:date]) ? Date.parse(params[:date]) : Date.today
  @through = (params[:through]) ? Date.parse(params[:through]) : @date + 1.day

  # the .in_time_range(range) method can be defined as a scope on the Order model, and you can
  # get rid of that logic here
  # 
  @orders = Order.daily.includes(:store).in_time_range(@date, @through).group_by(&:store)

end

class Order < ActiveRecord::Base

  scope :in_time_range, lambda { |date, through| where('orders.created_at' => (date..through)) }

  # It looks like you already know how to collect the orders for your needs... ie Order#early, etc

end

# Now, consider in your views the ability to do this:
@orders.each do |store, orders|
  # the orders now are the orders that met the above qualifications for the store
  orders.early
  Order.average_of_orders(orders.early)
  orders.late
  Order.average_of_orders(orders.late)
  orders.on_time    
end

Answer 2

Моя попытка:

def punctuality_report
  @date    = params[:date]    ? Date.strptime(params[:date], "%m/%d/%Y")    : Date.today
  @through = params[:through] ? Date.strptime(params[:through], "%m/%d/%Y") : @date + 1

  time_range = @date.to_formatted_s(:db)..@through.to_formatted_s(:db)

  @orders = Order.daily.includes(:store).where('orders.created_at' => time_range).group_by{|o| o.store}

  @orders.each_key.each do |s|
    all_early = @orders[s].collect{|o| o if o.early?}.compact
    all_late  = @orders[s].collect{|o| o if o.late?}.compact
    self.instance_variable_set("@s#{s.id}_early", all_early)   
    self.instance_variable_set("@s#{s.id}_avg_early", all_early.count > 0 ? all_early.collect{|o| o.earliness}.sum / all_early.count : 0)         
    self.instance_variable_set("@s#{s.id}_late", all_late) 
    self.instance_variable_set("@s#{s.id}_avg_late", all_late.count > 0 ? all_late.collect{|o| o.lateness}.sum / all_late.count : 0)         
    self.instance_variable_set("@s#{s.id}_on_time", @orders[s] - (all_early | all_late) )
  end
end

Короче говоря: в разделе eval были добавлены только переменные экземпляра, и мы можем использовать instance_variable_get и instance_variable_set. Можно избежать почти всех случаев eval, но я также считаю, что eval иногда может быть чрезвычайно полезным и очень мощным.

Код более четко выражает свое намерение: он добавит набор переменных экземпляра, которые сразу видны на виду.

Я определенно не считаю использование group_by запахом кода.

Answer 3

Я думаю, что group_by - совершенно правильный инструмент при правильном использовании. У меня был метод в задании cron, который использовал несколько вложенных операторов group_by, и я был убежден, что это вызывает значительное замедление. Я потратил 2 часа на переписывание метода, чтобы избавиться от group_by, и время выполнения сократилось с часа 8 минут до часа 5 минут. Вряд ли стоит затраченных усилий.

С другой стороны, у этого кода есть серьезные проблемы. Эти троичные операторы на самом деле заставили меня смеяться вслух, а заявления eval просто злые. Мало того, что они использовали Eval, они использовали его плохо. Вызов eval очень медленный, и нет никаких причин, по которым все 5 операторов eval не могли бы быть объединены в одно.

Тем не менее, даже один eval-оператор в этом методе - это слишком много. Я уверен, что есть время и место для eval, но я никогда не нуждался в нем в своем коде. По моему общему опыту, практически все вызовы eval можно заменить на send, что намного быстрее и безопаснее.