moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.62k stars 440 forks source link

abstract CurrencyPair to an interface #742

Closed morloderex closed 1 year ago

morloderex commented 1 year ago

Currently we can create our own Exchange objects, but we are forced to use this library's CurrencyPair ValueObject.

This Pull Request tries to lift that restriction by extracting the methods inside the CurrencyPair into an interface that can be used as a return value while implementing our own Exchange values.

The use-case is to allow calling converter#convertAndReturnWithCurrencyPair and have it return my custom implementation back to me if i implement my own Exchange implementation.

Previously it was impossible to retrieve any other information from the CurrencyPair other than the currencies involved and the Rate for it. With this proposal a Developer would be able to provide their own implementation and that implementation could consist of e.g. the date or the underlaying provider used for the exchange.

Superseds #710.

Maybe we should even provide a trait for very basic logic that currently goes into creating the CurrencyPair ValueObject?

What do you think of this?

frederikbosch commented 1 year ago

While I get where you are going with this, I wonder why you must talk to the Exchange interface defined in this package.


namespace Vendor;

interface Exchange {
  public function quote(Currency $baseCurrency, Currency $counterCurrency): MyReturnType;
}

final class MyExchange implements Exchange {
  public function __construct(private Money\Exchange $moneyExchange) {}

  public function quote(Currency $baseCurrency, Currency $counterCurrency): MyReturnType
  {
       $currencyPair = $this->moneyExchange->quote($baseCurrency, $counterCurrency);
       return $this->convertToOrAddSpecificsAboutMyReturnType($currencyPair);
  }
}
morloderex commented 1 year ago

@frederikbosch

Yes i can decorate the exchange implementation but that also means that i still have to maintain my own implementation of \Money\Converter class sense calling convertAndReturnWithCurrencyPair on that object would mean that the exchange service would be hit twice sense i cannot manually set which CurrencyPair on it to use and by pass the call to \Money\Exchange->qoute(). Which to me seems strange when we have the ability to lift this restriction from a given developer by type-hinting the \Money\CurrencyPair class type to an interface type on the converter class.

We are getting very deep in the code but the basic idea from my point of view is to allow easier access to whatever data from a CurrencyPair without making a developer also have to implement and maintain their own \Money\Converter class entirely which is what i am currently doing.

From a logic point of view I don't care about how the actual \Money\Money object is constructed using the counter currency that logic should stay in the Money\Converter class. I just care about that i can get a CurrencyPair and a \Money\Money instance back using the counter currency. While also getting some additional metadata from the CurrencyPair back.

Unfortunately for me the CurrencyPair valueObject from this package returns less meta information than i need.

To me this seems like a much more expandable approach to my problem than #710 implemented which was much more specific targeted. With this pr a developer could add their own CurrencyPair and return whatever metadata they want while also not having to care about the usage of the \Money\Converter class usage at all.

frederikbosch commented 1 year ago

I disagree.


namespace Vendor;

interface MyExchange {
  public function quote(Currency $baseCurrency, Currency $counterCurrency): MyReturnType;
}

final class MyConverter {
  public function __construct(private MyExchange $myExchange, private Money\Converter $moneyConverter) {}

  /** @return array{0: Money, 1: MyReturnType} */
  public function convertAndReturnWithMyReturnType(Money $money, Currency $counterCurrency, int $roundingMode = Money::ROUND_HALF_UP): array
  {
      $myPair = $this->myExchange->quote(
          $money->getCurrency(),
          $counterCurrency
      );

      $moneyCurrencyPair = new CurrencyPair($myPair->getBaseCurrency(), $myPair->getCounterCurrency(), $myPair->getConversionRatio());

      return [$this->moneyConverter->convertAgainstCurrencyPair($money, $moneyCurrencyPair, $roundingMode), $myPair];
  }
}
morloderex commented 1 year ago

@frederikbosch That's an interesting way of decorating the money converter yes.

But i still feel like it its somewhat over over engineering this whole thing, but it might just be me.

I'll close this if you really don't like my design idea.

frederikbosch commented 1 year ago

@morloderex I'd rather not provide an interface for value objects. Then the value object loses its purposes. Money\Exchange and MoneyConverter are tied to Money\CurrencyPair by design. If it does not fit you, you can create own exchanges, converters and currency-pair-alike objects. Or, as I have suggested, have your own exchange and converter next to ours. Per use-case you can decide what to inject Money\Converter, with limited conversion meta data, or Vendor\Converter with extended meta data. I do this quite often in my own applications.