Math Calculus in ruby

Question

I have several calculus to do in my ruby app. My code is currently working but I find it very ugly.

@guestreviews = GuestReview.where(:reviewed_id => @user.id)
@hostreviews = HostReview.where(:reviewed_id => @user.id)
@hospitalityrate = 0
@foodrate = 0
@placerate = 0
@hostreviews.try(:each) do | rate |
 @hospitalityrate += rate.people_rate
 @foodrate += rate.food_rate
 @placerate += rate.place_rate
end
@hospitalityrate = (@hospitalityrate / @hostreviews.size).round
@foodrate = (@foodrate / @hostreviews.size).round
@placerate = (@placerate / @hostreviews.size).round
@averagerate = ((@hospitalityrate + @foodrate + @placerate) / @hostreviews.size).round

Any idea how to make it still readable but less ugly ?

• tokland: why are all of these instance variables? why not use some local variables?
• user229044: You're misusing try. @hostreviews is guaranteed to be an array of 0 or more items, it will respond to each. Don't use try here.
• bl0b: @meagar I don't use try anymore, but hostreviews can also be nil.
• user229044: @bl0b No, it can't be nil. where will always return a chainable scope which responds to each, which will behave like an array of 0 or more elements. In the code you've posted, it's impossible for @hostreviews to be nil by the time it reaches the each statement.
• bl0b: @meagar You're right. I juste checked my code and I don't use the .each anymore and it wasn't nil but an empty array. Thank you

Answer 1

It's actually easier to do this in the database layer. It's exceedingly good at these kinds of things.

@guest_reviews = GuestReview.where(:reviewed_id => @user.id)
@host_reviews = HostReview.where(:reviewed_id => @user.id)
@hospitality_rate = @guest_reviews.average('people_rate')
@food_rate = @guest_reviews.average('food_rate')
@place_rate = @guest_reviews.average('place_rate')
@average_rate = [@hospitality_rate, @food_rate, @place_rate].inject(:+).to_f / @host_reviews.count

It'll result in more database queries, but overall it's more efficient.

I've left out the round() call on the @average_rate only because that seems like a presentation concern to me. Let the controller figure out the average (decimals and all), and let the view decide how to round and display it.

You could also add a has_many :guest_reviews, :foreign_key => "reviewed_id" to the User model, so you can just call @user.guest_reviews.average(...). Same for host_reviews

If you do that, you could also consider simply moving the calculations to the User model, so you don't need to do it all in the controller. And perhaps you could cache the results on the user record and only recalculate when reviews are added/deleted/changed. Assuming this happens less often than the averages being displayed, it'll speed up things a bit.

Lastly, (and this is just small stuff) I'd probably rename some of the variables. I.e. call it @food_rate_average instead of @food_rate in order to a) indicate that it's an average, and b) so it's not confused with the database column of the same name.
Similarly, I'd call the first average people_rate_average rather than hospitality_rate; the column is called people_rate so it's probably best to keep it consistent with that name (or rename the database column, if "hospitality" is a more fitting name)

(Edit: Actually, it's more natural to prefix the average_ rather than suffixing it, i.e. average_food_rate, average_rate, etc.)

In the code above, I've also underscored the names, just to keep things consistent with general Rails/Ruby conventions (FooBar → foo_bar).

---

Edit: Just for fun, you could go fully meta (though it's probably more confusing than anything - the above is much more readable)

@guest_reviews = GuestReview.where(:reviewed_id => @user.id)
@host_reviews = HostReview.where(:reviewed_id => @user.id)
@average_rate = %w(people_rate food_rate place_rate).map { |column|
 instance_variable_set(:"@average_#{column}", @guest_reviews.average(column))
}.inject(:+).to_f / @host_reviews.count

That'll set @average_people_rate, @average_food_rate and so on

• bl0b: I would give 1000 upvote if I could. That's such a good answer !
• Flambino: @bl0b Thanks. I had added a few more notes here and there if you're interested
• bl0b: Thanks. I was actually using the foreign key system, no idea why I wasn't using it. Thanks for the reminder. Also, I won't cache the result right now because we don't have enough user/visitor yet to worry about it(sadly) but I'll keep it in mind. I'll change my variables name, you're right it makes more sense with the way you said it.