Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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
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
up vote 3 down vote accepted

You can do this:

def total

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

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


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.