HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Math Calculus in ruby

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mathrubycalculus

Problem

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 ?

Solution

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 (FooBarfoo_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

Code Snippets

@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
@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

Context

StackExchange Code Review Q#25172, answer score: 5

Revisions (0)

No revisions yet.