4
\$\begingroup\$

I have a booking with a method total which calculates the total price of the booking from the sum of all the prices of activities that belong to appointments. A booking has many appointments and the following seems to give the right answer, but it seems horrid. How can I improve this?

  def total 
    # TODO  shouldn't this be a db query?
    total = 0
    self.appointments.each do |appointment|
        unless appointment.activity.price.nil? 
            total += appointment.activity.price.to_f
        end
    end     
    total
  end
\$\endgroup\$
  • \$\begingroup\$ Hello, and welcome to Code Review! I don't know RoR enough to answer, but yes you need to understand how to do a sum query, which will avoid both the loop and the nil handling. \$\endgroup\$ – Quentin Pradet Feb 27 '13 at 7:21
4
\$\begingroup\$

You can do this:

def total
  self.appointments.joins(:activity).sum(:price)
end

You could perhaps simplify it a little, by making an explicit 2nd order has_many on your booking model:

has_many :appointments
has_many :activities, :through => :appointments

Then you can just do:

def total
  self.activities.sum(:price)
end

In either case, it'll end up as a single DB-query.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That has sped things up quite a bit even in my dev environment with a small data set & is obviously much more idomatic / simple / nicer to look at. Active record continues to amaze. Thanks. \$\endgroup\$ – Will Feb 27 '13 at 21:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.