create a base bank class

[semmons99]

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

[semmons99]

implemented in 4d688e7

[weppos]

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]

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]

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

[weppos]

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]

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]

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]

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

[weppos]

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]

changes have been merged into trunk.