Need help cleaning up a model method

Rails
1

Hi,

I need a little help cleaning up the method below. In my .NET days I
would have let this pass, but I am trying to be a good ruby citizen by
keeping things simple.

This is from a seller model which has many vehicles:

def total_retail_price
if @@total_retail_price.nil?
@@total_retail_price = self.vehicles.sum(:retail_price)
@@total_retail_price = 0 unless @@total_retail_price
end
@@total_retail_price
end

If I didn’t have to worry about the seller having 0 vehicles I could
just use:

def total_retail_price
@@total_retail_price ||= self.vehicles.sum(:retail_price)
end

Does anyone have any suggestions on how I can fix improve on this?

Thanks,
GiantCranes

-ps, i mistakenly posted this in the ruby forum earlier (sorry)

2

Giant C. wrote:

@@total_retail_price = self.vehicles.sum(:retail_price)

end

Does anyone have any suggestions on how I can fix improve on this?

First up, why are you using class variables (the @@ is a class
variable)? I’m willing to be you want instance variables (single @).

Second up, I don’t think the way you’re summing right now actually
works. What you’re passing is the identity, what you want to pass is a
block like: self.vehicles.sum { |vehicle| vehicle.retail_price }.

Third up, I believe this will actually work in itself without doing
anything else. So you should be able to simply do:

def total_retail_price
@total_retail_price ||= self.vehicles.sum {|vehicle|
vehicle.retail_price }
end

If there are no vehicles this should set total to 0.

–
Cheers,

  • Jacob A.
3

Thanks for your help Jacob, you are correct on each point.