# Can you improve on this code? **URL:** \ **Category:** Rails\ **Created:** [April 24, 2006, 10:53am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635 "2006-04-24T10:53:42Z")\ **Posts on this page:** 8\ **Page:** 1
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 10:53am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/1 "2006-04-24T10:53:42Z")
# File app/models/timesheet.rb, line 27 27: def totals 28: totals = Hash.new 29: totals[“Monday”] = totals[“Tuesday”] = totals[“Wednesday”] = totals[“Thursday”] = totals[“Friday”] = totals[“Saturday”] = totals[“Sunday”] = totals[“Totals”]=0 #initialise all to zero 31: 32: for item in items 33: totals[“Monday”]+=item.hoursday1 34: totals[“Tuesday”]+=item.hoursday2 35: totals[“Wednesday”]+=item.hoursday3 36: totals[“Thursday”]+=item.hoursday4 37: totals[“Friday”]+=item.hoursday5 38: totals[“Saturday”]+=item.hoursday6 39: totals[“Sunday”]+=item.hoursday7 40: totals[“Totals”]+=item.hours 41: end 42: totals 43: end ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 11:09am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/2 "2006-04-24T11:09:56Z")
On Monday 24 April 2006 09:53, Chris wrote: > 36:    totals[“Thursday”]+=item.hoursday4 > 37:    totals[“Friday”]+=item.hoursday5 > 38:    totals[“Saturday”]+=item.hoursday6 > 39:    totals[“Sunday”]+=item.hoursday7 > 40:    totals[“Totals”]+=item.hours > 41:   end > 42:   totals > 43:  end Untested so may contain stupid errors, but I’d write something like this:  DAYS = %w( Monday Tuesday Wednesday Thursday Friday Saturday Sunday )   def totals  totals = Hash.new(0) # default to 0 for undefined keys   items.each do |i|  DAYS.each do |day|  totals[day] += item.send(“hoursday#{DAYS.index(day) + 1}”)  end  totals[“Totals”]+=item.hours  end   end Ashley ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 11:37am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/3 "2006-04-24T11:37:43Z")
Ashley M. wrote: > > 35: totals[“Wednesday”]+=item.hoursday3 > > ``` > end > > ``` > > end > I’d refactor a bit, and set up item.hours\_per\_day to be a Hash (or maybe > an accessor method): DAYS = %w( Monday Tuesday Wednesday Thursday Friday Saturday Sunday ) def totals totals = Hash.new(0) items.each do |i| i.hours\_per\_day.each\_pair { |day, hours| totals[day] += hours } totals[‘Totals’] += i.hours end end That’s only because I don’t like using #send unless it’s absolutely necessary, though. ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 11:53am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/4 "2006-04-24T11:53:03Z")
I would but i’m using a legacy schema, so items must have: item.hoursday1 … … item.hoursday7 item.hours #total hours ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 11:43am UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/5 "2006-04-24T11:43:48Z")
Chris wrote: > # File app/models/timesheet.rb, line 27 > > 27: def totals > 28: totals = Hash.new > 29: totals[“Monday”] = totals[“Tuesday”] = totals[“Wednesday”] = > totals[“Thursday”] = totals[“Friday”] = totals[“Saturday”] = > totals[“Sunday”] = totals[“Totals”]=0 #initialise all to zero > 31: > 32: for item in items > 33: totals[“Monday”]+=item.hoursday1 > 34: totals[“Tuesday”]+=item.hoursday2 > 35: totals[“Wednesday”]+=item.hoursday3 > 36: totals[“Thursday”]+=item.hoursday4 > 37: totals[“Friday”]+=item.hoursday5 > 38: totals[“Saturday”]+=item.hoursday6 > 39: totals[“Sunday”]+=item.hoursday7 > 40: totals[“Totals”]+=item.hours > 41: end > 42: totals > 43: end For one thing, that hash init to zero code can be much cleaner by iterating through a DAYS array. You can also use that to trim down your total loop. The following code assumes the hoursdayX methods are actually database columns, and therefore should be accessible via [field\_name] notation. If they aren’t DB columns then you need a better way to get that data from your model. class ItemController \< ApplicationController DAYS = [ “Monday”, “Tuesday”, “Wednesday”, “Thursday”, “Friday”, “Satday”, “Sunday” ] def totals totals = {“Totals” =\> 0} DAYS.each do |day| totals[day] = 0 end ``` items.each do |item| 1.upto 7 do |i| totals[DAYS[i]] += item["hoursday#{i}"] end totals["Totals"] += item.hours end totals ``` end end ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 12:05pm UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/6 "2006-04-24T12:05:33Z")
Chris wrote: > I would but i’m using a legacy schema, so items must have: > > item.hoursday1 > … > … > item.hoursday7 > item.hours #total hours > You could use a wrapper method, but Alex W.'s suggestion is just as > good… ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 12:02pm UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/7 "2006-04-24T12:02:32Z")
On Apr 24, 2006, at 11:43, Alex W. wrote: > class ItemController \< ApplicationController > DAYS = [ > “Monday”, > “Tuesday”, > “Wednesday”, > “Thursday”, > “Friday”, > “Satday”, > “Sunday” > ] Date::DAYNAMES could be used there instead. I am not sure whether it would depend on the locale though. – fxn ---
**Author:** ![chris](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/c/958977/32.png) [@chris](https://www.ruby-forum.com/u/chris)\ **Post date:** [April 24, 2006, 2:34pm UTC](https://www.ruby-forum.com/t/can-you-improve-on-this-code/58635/8 "2006-04-24T14:34:10Z")
Hi – On Mon, 24 Apr 2006, Alex W. wrote: > > 34: totals[“Tuesday”]+=item.hoursday2 > > For one thing, that hash init to zero code can be much cleaner by > > “Monday”, > > DAYS.each do |day| > > totals[day] = 0 > > end See previous replies; it should be adequate to set the hash default to zero: totals = Hash.new(0) > items.each do |item| > 1.upto 7 do |i| > totals[DAYS[i]] += item[“hoursday#{i}”] You’d want DAYS[i-1] there. Or you might be able to do something with each\_with\_index: DAYS.each\_with\_index do |day, i| totals[day] += item.send(“hoursday#{i+1}”) etc. (I’ve reverted to the send thing based on the OP’s explanation of the constraints on the schema.) David – David A. Black (removed\_email\_address@domain.invalid) Ruby Power and Light, LLC ([http://www.rubypowerandlight.com](http://www.rubypowerandlight.com)) “Ruby for Rails” PDF now on sale! [Ruby for Rails](http://www.manning.com/black) Paper version coming in early May!