create a base bank class #14

Closed
semmons99 opened this Issue Jul 16, 2010 · 10 comments

Projects

None yet

2 participants

@semmons99
RubyMoney member

We need a Bank::Base class that users can subclass to create their own implementations.

@semmons99
RubyMoney member

implemented in 4d688e7

@weppos
RubyMoney member

I was looking at your implementation. It looks great, however I've one additional suggestion.

The code in master can be simplified as follows

class Money
  class BaseBank
  end
  class VariableExchangeBank < BaseBank
  end
end

I like the idea to define an intermediate Bank namespace

class Money
  module Bank
    class Base
    end
    class VariableExchange < Base
    end
  end
end

This would allow the Bank module to contain all the bank-focused classes/objects/code not strictly tied to base or a bank implementation.
For instance Money::Bank::Base (or any subclass) could raise a Money::Bank::UnknownRate error instead of Money::BaseBank::UnknownRate.

The usage of a Base class as synonymous for simple interface is a really common pattern in the Ruby world, as you might probably know. Think about Sinatra, Rails, ...

What do you think?

@semmons99
RubyMoney member

Good idea, I like it. If you have time to implement it this weekend, feel free, otherwise I'll work on it Monday morning.

@weppos
RubyMoney member

I'm working on it. Could you reopen the task?

@weppos
RubyMoney member

Done. All the changes are available in the bank branch on my repo.
http://github.com/weppos/money/compare/weppos:master...weppos:bank

Here's the comparison with master on FooBarWidget repo.
http://github.com/weppos/money/compare/bank

Note that the bank branch includes also the changes suggested in #16.

@semmons99
RubyMoney member

The only thing I'm thinking is that we need to move get_rate, set_rate and rate_key_for into Money::Bank::Base. They're going to be needed quite often as helper methods in other Bank implementations.

@weppos
RubyMoney member

As I wrote in
http://github.com/weppos/money/commit/83bc6ca2c5f035fc132e7d1805886ced5a405e51
I can agree with moving get_rate and set_rate in the base class, but I don't know whether it is a good idea to actually implement them.

For instance, I might want my SuperCool bank to store rates in Redis or MongoDb instead of in memory. get_rate and set_rate might want to contact Redis instead of Mutex.

Also, I might need to create a custom key generation algorithm.

While I was working on the refactoring, I realized that VariableExchange bank can also be considered a more feature-rich base class.
I mean, if your code need a prepackaged solution to store and retrieve rates, you can extend VariableExchange instead of Base.

IMHO, the only reason to add get_rate and set_rate to Base, would be to encourage a consistent naming convention. In this case, both methods should raise NotImplementedError in Base.

Let me show you two concretes examples. In my code I have a Test bank I use while running the application in test mode.

module Converters
  class Test < Money::Bank::Base

    def exchange_with(from, to_currency)
      Money.new(from.cents * 2)
    end

  end
end

I've also a more powerful converter which works which interacts with a custom backend.

require "uri"
require "net/http"

module Converters
  class Service < Money::Bank::Base

    def initialize(key)
      @key = key
    end

    def exchange_with(from, to_currency)
      return from if same_currency?(from.currency, to_currency)

      Timeout::timeout(5) do
        # contact service and deal with the request
      end
    end

  end
end

As you can see, they both requires a super-small set of features and I found Base to be the perfect solution. For a variety of reasons, they don't offer a rate get/set mechanism.

I've also an other internal backed which, instead, gives the ability to manually configure rates. In this case, I extend Money::Bank::VariableExchange and override the exchange_with method with my custom logic.

That said, if you really want to move get_rate and set_rate to Base, the world won't end now. ;)
Just le me know, I will make the changes and prepare the branch for the merge.

@semmons99
RubyMoney member

What you're saying makes sense. Let's leave them where they are in VariableExchange.

@weppos
RubyMoney member

OK, I pushed all the changes. You can find them in my master branch.
I suggest you to merge the changes via Git to preserve the merge history. Otherwise, the GitHub Fork Queue feature will flatten the commits.

$ cd money
$ git checkout master
$ git remote add weppos git://github.com/weppos/money.git
$ git fetch weppos
$ git merge weppos/master
@semmons99
RubyMoney member

changes have been merged into trunk.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment