Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

money_column currency option #152

Open
elfassy opened this issue Apr 18, 2018 · 2 comments
Open

money_column currency option #152

elfassy opened this issue Apr 18, 2018 · 2 comments

Comments

@elfassy
Copy link
Contributor

elfassy commented Apr 18, 2018

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

@elfassy elfassy added this to the v1.0.0 milestone Apr 18, 2018
@bdewater
Copy link
Contributor

bdewater commented Apr 19, 2018

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
Copy link
Contributor Author

elfassy commented Apr 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants