Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

If 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, where a booking hasmany appointments 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     
share|improve this question
    
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. –  Quentin Pradet Feb 27 '13 at 7:21

1 Answer 1

up vote 1 down vote accepted

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.

share|improve this answer
    
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. –  Will Feb 27 '13 at 21:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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