Rails Group_按反模式

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)[email protected]_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 哈希有用。我觉得有一个更好的方法来获得我需要的东西并减少大量的查询和 ruby​​ 处理。

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

我认为如果使用得当，group_by 是一个完全有效的工具。我在 cron 作业中有一个方法，它使用了一些嵌套的 group_by 语句，我确信这会导致速度显着减慢。我花了2个小时重写了去掉group_by的方法，运行时间从1小时8分钟变成了1小时5分钟。几乎不值得付出努力。

另一方面，该代码存在一些严重的问题。那些三元运算符实际上让我笑出了声，而 eval 语句简直是邪恶的。他们不仅使用了 eval，而且使用得很糟糕。调用 eval 非常慢，并且没有理由不能将所有 5 个 eval 语句合并为一个。

也就是说，即使此方法中只有一条 eval 语句也太多了。我确信有 eval 的时间和地点，但我的代码中从来不需要它。根据我的一般经验，几乎所有对 eval 的调用都可以用 send 代替，这样更快、更安全。

Answer 3

我的尝试：

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)[email protected]_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 是一种代码味道。