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是一種代碼味道。