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哈希有用。我觉得有更好的方法来获得我需要的东西，并减少很多这种查询和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非常缓慢，没有理由说所有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)..@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是一种代码味道。