Can you improve on this code?


#1

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


#2

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


#3

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.


#4

I would but i’m using a legacy schema, so items must have:

item.hoursday1


item.hoursday7
item.hours #total hours


#5

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


#6

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…


#7

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


#8

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)

“Ruby for Rails” PDF now on sale! http://www.manning.com/black
Paper version coming in early May!