Best way to write this calculation in ruby.

Question

amt = self.alt_inv - (self.alt_tax ? self.alt_tax : 0)
    - (self.alt_freight ? self.alt_freight : 0)
    - (self.misc1_amt ? self.misc1_amt : 0)
    - (self.misc2_amt ? self.misc2_amt : 0)

Answer 1

Looks ok to me with two exceptions:

1) Ruby makes assumptions about statement endings.

Since

amt = self.alt_inv - (self.alt_tax ? self.alt_tax : 0)

is itself a valid statement, you need to either keep the minus on the prior line or escape the newline.

2) Ruby assumes "self" as the owner except for left side assignments. I suggest:

amt = alt_inv - 
  (alt_tax     ? alt_tax     : 0) -
  (alt_freight ? alt_freight : 0) -
  (misc1_amt   ? misc1_amt   : 0) -
  (misc2_amt   ? misc2_amt   : 0)

Answer 2

In ruby, if something is not nil, it will return true in a boolean expression and its value in a calculation. I'm not explaining it really well, but you can do something like this:

amt = alt_inv - 
  (alt_tax     || 0) -
  (alt_freight || 0) -
  (misc1_amt   || 0) -
  (misc2_amt   || 0)

This is a more concise way of doing the ternary you were originally using.

Edit:

I actually like Jed Schneider's answer better than my own. I'll not copy it here, since his answer deserves the upvote for it's elegance.

Answer 3

It might be cleaner if you could automatically initialize your object's values to 0 when the object was created, if those values were not supplied in the constructor. Otherwise, you need to do this sort of conditional logic everywhere in your objects these values are needed. Wouldn't you rather just do this?

amt = alt_inv - alt_tax - alt_freight - misc1_amt - misc2_amt 

Answer 4

since the amt is the difference between the sum of all the attributes and the alt_inv you can sum the deductions and then subtract from alt_inv. Reduce offers a clean way to do this. Compact removes any nil values before performing the reduction. of course attributes in this case can be expanded to whatever your needs are, even created dynamically from data, and in real life, i wouldn't use the variable attributes, obviously.

attributes = [alt_tax, alt_freight, misc1_amt, misc2amt]
amt = alt_inv - attributes.compact.reduce(:+)

reduce: http://apidock.com/ruby/Enumerable/reduce

compact: http://apidock.com/ruby/Array/compact

Answer 5

If you haven't local variable with same name, so instead of self.alt_tax you can write simply alt_tax without self.

Also || operator returns first operand when it's not nil and second otherwise, so instead of

alt_tax ? alt_tax : 0

you can write

alt_tax || 0

Answer 6

A little variations from the solution of Jed, which is not needlessly complex, but more succinct IMHO. Using compact and inject (the most common synonym of reduce) are not complex, and thoroughly used among Ruby programmers.

amt = [alt_tax, alt_freight, misc1_amt, misc2_amt].inject(alt_inv) do |result, attribute|
  result - (attribute || 0)
end

or

amt = alt_inv - [alt_freight, misc1_amt, misc2_amt].compact.inject{|sum, n| sum + n }

which is an alternate way to write the inject(&:+) if you're uncomfortable with that syntax. If you're using Rails, you can substitute the inject with the sum method, or implement it yourself.

Answer 7

amt = alt_inv - alt_tax.to_f - alt_freight.to_f - misc1_amt.to_f - misc2_amt.to_f

nil.to_f = 0.0