i encounter code smell when use enumerable group_by method. old code i'm refactoring example
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 |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
okay i'm coming through , see need refactor report out of action on orders controller , model of own clean implementation logic. there's 1 code smell out, still struggle orders.group_by hash.
the thing when in view layer want hash. need summaries orders need access stores. using group query method in activerecord returns me relation, not useful enumerable group_by hash. feel there's better way need , reduce lot of querying , ruby processing.
i don't see wrong group_by method. see issue overuse of eval though [-; eval strong code smell (imho) group_by
that being said, did see other areas refactor:
# consider refactor: def punctuality_report # find more readable @date = (params[:date]) ? date.parse(params[:date]) : date.today @through = (params[:through]) ? date.parse(params[:through]) : @date + 1.day # .in_time_range(range) method can defined scope on order model, , can # rid of 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)) } # looks know how collect orders needs... ie order#early, etc end # now, consider in views ability this: @orders.each |store, orders| # orders orders met above qualifications store orders.early order.average_of_orders(orders.early) orders.late order.average_of_orders(orders.late) orders.on_time end
Comments
Post a Comment