brick / money

A money and currency library for PHP
MIT License
1.61k stars 96 forks source link

CurrencyMismatchException pain #15

Closed jkuchar closed 6 years ago

jkuchar commented 6 years ago

We are using money objects in single-currency environment. It is very uncomfortable that we need to check at every use of operation that requires more money objects for CurrencyMismatchException.

Then there are ShouldNeverHappend expceptions thrown everywhere, which does not make sense in our context.

I think that CurrencyMismatchException should be LogicException, which gives developer feedback that he has done something wrong and that he should not use it this way.

On the other hand, when someone "creates" currency on the fly from what someone set using GUI, it should make RuntimeException because it acts as validator of user input. "Hey, you cannot sum numbers of different currencies."

I'm not sure it it is responsibility of value-object to act as validator for user-input. What are your opinions?

Question: How to make single-currency usage more pleasant?

Same thing has happened with rounding on ::of() constructor. Should it throw Logic or Runtime exception? It is very uncomfortable to check for exceptions like these every time.

try {
  return Money::of(
    self::round($unitPrice->getBase(), $quantity, $unitPrice->getCurrency()),
    $unitPrice->getCurrency()
  );
} catch (RoundingNecessaryException $e) {
  throw new ShouldNeverHappenException($e);
}
BenMorel commented 6 years ago

Hi, I don't think these exceptions should extend LogicException ("should lead directly to a fix in your code"), or even RuntimeException ("an error which can only be found on runtime"); this last one can arguably be considered an unchecked exception that's not really supposed to be caught in the normal application flow.

IMO, these are library-specific exceptions that don't fit well within any of the SPL exceptions, and that should just extend the base Exception class.

About your issue, I usually do one of 2 things:

So your single currency usage falls within my second point, I would not catch it at all, I would let the uncaught exception handler deal with it!

BenMorel commented 6 years ago

I'm closing this issue as no feedback has been given. Please comment if you feel that it should be reopened!