Forum: Ruby on Rails Can you improve on this code?

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Cd8c9864d88bcafc164d8fdb820cc451?d=identicon&s=25 Chris (Guest)
on 2006-04-24 10:53
# 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
A90204c955db033cd975f7bb0ec9600b?d=identicon&s=25 Ashley Moran (Guest)
on 2006-04-24 11:09
(Received via mailing list)
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
Ad7805c9fcc1f13efc6ed11251a6c4d2?d=identicon&s=25 Alex Young (Guest)
on 2006-04-24 11:37
(Received via mailing list)
Ashley Moran 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.
D0cd6b10e01bacb976b3b815a9c660bc?d=identicon&s=25 Alex Wayne (Guest)
on 2006-04-24 11:43
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
Cd8c9864d88bcafc164d8fdb820cc451?d=identicon&s=25 Chris (Guest)
on 2006-04-24 11:53
I would but i'm using a legacy schema, so items  must have:

item.hoursday1
...
...
item.hoursday7
item.hours #total hours
7223c62b7310e164eb79c740188abbda?d=identicon&s=25 Xavier Noria (Guest)
on 2006-04-24 12:02
(Received via mailing list)
On Apr 24, 2006, at 11:43, Alex Wayne 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
Ad7805c9fcc1f13efc6ed11251a6c4d2?d=identicon&s=25 Alex Young (Guest)
on 2006-04-24 12:05
(Received via mailing list)
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 Wayne's suggestion is just as
good...
1fba4539b6cafe2e60a2916fa184fc2f?d=identicon&s=25 unknown (Guest)
on 2006-04-24 14:34
(Received via mailing list)
Hi --

On Mon, 24 Apr 2006, Alex Wayne 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 (dblack@wobblini.net)
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!
This topic is locked and can not be replied to.