Shopify / money

Manage money in Shopify with a class that won't lose pennies during division
http://shopify.github.io/money
MIT License
323 stars 38 forks source link

money_column currency option #152

Open elfassy opened 6 years ago

elfassy commented 6 years ago

For the next major release we should look into changing money_column to support a single currency. Rather than both currency and currency_column, we could have only currency and make it accept something responding to call or to_s. Example:

class Product
  money_column :price, currency: ->(product) { product.currency }
  money_column :price_usd, currency: 'USD'
end

I think this is easier to reason about and will help clean up the code

cc: @bdewater

bdewater commented 6 years ago

I'm having a bit of deja vu 😄 we've had a similar discussion in the past: passing a symbol would refer to a method/attribute and a string would mean a hardcoded currency. We opted for the current solution with the currency_column and currency keyword arguments at the time.

I agree the money_column should be nicer but I'm not convinced a lambda is worth the performance impact:

require "benchmark/ips"

def lambda
  ->(x) { x.zero? }
end

def method_send
  0.send(:zero?)
end

def method_call
  0.zero?
end

Benchmark.ips do |x|
  x.report("lambda") { lambda.call(0) }
  x.report("method send") { method_send }
  x.report("method call") { method_call }
  x.compare!
end
$ ruby -v test.rb
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17]
Warming up --------------------------------------
              lambda   114.692k i/100ms
         method send   268.351k i/100ms
         method call   296.777k i/100ms
Calculating -------------------------------------
              lambda      1.598M (± 2.2%) i/s -      8.028M in   5.025326s
         method send      7.275M (± 2.8%) i/s -     36.496M in   5.020949s
         method call      9.247M (± 2.6%) i/s -     46.297M in   5.010301s

Comparison:
         method call:  9246686.8 i/s
         method send:  7274506.0 i/s - 1.27x  slower
              lambda:  1598367.5 i/s - 5.79x  slower
elfassy commented 6 years ago

another alternative would be hash specifically for the currency options something like

class Product
  money_column :price, currency: { column: 'currency', read_only: true }
  money_column :price_usd, currency: 'USD'
end