Is there a better way to write this code?

[Megafu_Anthony]

def get_sub_total
       cart_total = 0
       ebook_cart_total = 0

       if !shopping_cart_empty?
          if any_products_in_cart?
             products_in_cart.each do|key, item|
                product = Product.find(Base64.decode64(key).chomp.to_i)
                cart_total += product.price.to_f * item.to_f
             end
          end

          if any_ebooks_in_cart?
             ebooks_in_cart.each do|key, item|
                product = Product.find(Base64.decode64(key).chomp.to_i)
                ebook_cart_total += product.ebook_price.to_f * item.to_f
             end
          end
       end

       # subtotal
       cart_total + ebook_cart_total
	end

[megazoid]

Hi
It would help if you tell us what is wrong with that code and why do you look for another way to write it

[Megafu_Anthony]

Hi Megazoid,
Thanks for the response. There is absolutely nothing wrong with the code but I feel it is a little verbose.

Considering Ruby’s stand on brevity, there should be a more concise way to write it

[Mittineague]

@Megafu_Anthony

There is absolutely nothing wrong with the code

I don’t know what level of precision / accuracy you need, but considering this is working with currency I’m wondering if BigDecimal would be better than Float?

Class: BigDecimal (Ruby 1.9.3)

normal binary floating point arithmetic often introduces subtle errors because of the conversion between base 10 and base 2

[Megafu_Anthony]

Thanks a lot Mittineague. Will put that into consideration

[Leighthall]

Without knowing the internal structure of your cart collections (and without having access to your test suite), you could do something like the following - I’ll show two stages.

First, extract the common code into a utility method, make use of ruby’s sum method, and make use of ternary operations to reduce those if blocks:

---

def get_sub_total
    !shopping_cart_empty? ? (cart_total + ebook_cart_total) : 0
end

def cart_total
    any_products_in_cart? ? total_items(products_in_cart) : 0
end

def ebook_cart_total
    any_ebooks_in_cart? ? total_items(ebooks_in_cart) : 0             
end

def total_items(products, total)
    products.sum {|key, item| retrieve_product_price(key) * item.to_f)}
end

def retrieve_product_price(key)
    product = Product.find(Base64.decode64(key).chomp.to_i)
    product.blank? ? 0 : product.price.to_f
end

---

If we get a little defensive in the total_items method, we can get rid of the ternary operators and reduce this to (this also presumes that an empty shopping cart can return an empty product cart and an empty ebook cart):

---

def get_sub_total
    cart_total + ebook_cart_total
end

def cart_total
    total_items(products_in_cart)
end

def ebook_cart_total
    total_items(ebooks_in_cart)
end

def total_items(products)
    return 0 if products.blank?
    products.sum {|key, item| retrieve_product_price(key) * item.to_f)}
end

def retrieve_product_price(key)
    product = Product.find(Base64.decode64(key).chomp.to_i)
    product.blank? ? 0 : product.price.to_f
end

---

Again, without access to the rest of your code and tests no guarantees it will work, but I hope it helps (and ditto on using BigDecimal as Mittineague suggested - or alternatively a money or currency gem that takes care of all of that for you).

Also, depending on your class API, all of these utility methods could be marked private, just leaving the get_sub_total method public.

Cheers,

Leigh.