Why final?

[celtric]

Does it make sense to mark it as such, knowing that it's a public library and users are expected to tinker with it? It's currently preventing us from adding everyday helpers like format() or toFloat(), and we don't really see the benefit.

[acasademont]

Hi @celtric! Sorry for the late answer, been quite busy recently. Let me explain our decicsion on the "final" keyword.

We feel that the current class fits all our needs and adding the "final" keyword prevents somebody to accidentally (or not so accidentally...) extend the class to add more methods.

One could argue that other organizations may haver other requirements and need more methods. Like you said you miss helpers like format() or toFloat. I would like to ask you why you need those methods inside the VO class. As you said, they're al helpers. We also have them of course (we're an ecommerce, we display prices a lot in the website) but they have their own classes. For example we have the MoneyFormatter class which handles all the complex logic that lies behind a correctly-formatted money value object. It's not as simple as an echo $value . ' ' . $currency. So I would suggest going that path and keep the formatting responsibility out of the domain model, formatting provides no business value.

And about the foFloat I would like to ask you why would you need that? This library is build using BCMath because of the lack of precision floating point arithmetic has. So...I don't really see the point in being 100% accurate when performing operations but then have the value converted to float. Again, If you really need that (and really, think about if you need that, Intl formatters can perfectly work with strings, there's no need to convert it to float before formatting) I would suggest putting that logic in the MoneyFormatter class.

Of course, you can prove me wrong with some other methods and we will gladly add it to the VO, this is just a proof of concept library that we're beginning to work with :)

[celtric]

Thanks for the reply! Some quick thoughts before leaving for dinner:

We feel that the current class fits all our needs and adding the "final" keyword prevents somebody to accidentally (or not so accidentally...) extend the class to add more methods.

Can't we trust people to follow LSP? Every library can be wrongly extended, but that doesn't mean their authors should mark them as final.

One could argue that other organizations may haver other requirements and need more methods. Like you said you miss helpers like format() or toFloat. I would like to ask you why you need those methods inside the VO class. As you said, they're al helpers. We also have them of course (we're an ecommerce, we display prices a lot in the website) but they have their own classes. For example we have the MoneyFormatter class which handles all the complex logic that lies behind a correctly-formatted money value object. It's not as simple as an echo $value . ' ' . $currency. So I would suggest going that path and keep the formatting responsibility out of the domain model, formatting provides no business value.

Speed is the main reason atm. We are still treating it as a VO, but a simple helper like

class Money extends UlaboxMoney
{
    public function format()
    {
        $formatter = new MoneyFormatter();

        return $formatter->format($this);
    }
}

would come handy while we are creating a new design from scratch. We also wanted to extract the interface to not couple to your library, but it being final would've meant creating and wiring the methods, writing some unit test to check they are correctly linked, etc.

If the library is kept final, we'll probably go this route once we find the time.

And about the foFloat I would like to ask you why would you need that? This library is build using BCMath because of the lack of precision floating point arithmetic has. So...I don't really see the point in being 100% accurate when performing operations but then have the value converted to float. Again, If you really need that (and really, think about if you need that, Intl formatters can perfectly work with strings, there's no need to convert it to float before formatting) I would suggest putting that logic in the MoneyFormatter class.

It was a (probably bad) example of a method we would probably add to reduce unnecessary code. We are using it store the value in MySQL, where we have a 2-decimal precision limit.

Of course, you can prove me wrong with some other methods and we will gladly add it to the VO, this is just a proof of concept library that we're beginning to work with :)

There are methods that may be useful to us but wouldn't be worth the effort adding them to the library. For example, we miss a static constructor where we don't have to instantiate a new Currency object every time (I'd've designed fromAmount() differently, and btw wouldn't the numeric check be in __construct()?), but we don't want to bother you when we don't know if we'll be using this library in a month, or even whether our contributions are worth considering.

Overall we are happy with it. We weren't aware that it's a proof of concept library (I don't think it's stated in the documentation), but we will continue using it in the future as we find it a useful and well-designed package.

[acasademont]

Thanks for your report! We're happy to discuss this :)

Can't we trust people to follow LSP? Every library can be wrongly extended, but that doesn't mean their authors should mark them as final.

I'd suggest using composition if you really need this, it's a cleaner approach than inheritance IMHO.

class MyCustomMoney
{
    private $money

    public function __construct()
    {
        $this->money = new Money();
    }

    public function toFloat()
    {
        return (float) $this->money->value();
    }

    public function __call($name, array $arguments)
    {
        return $this->money->{$name}(...$arguments);
    }
}

Speed is the main reason atm. We are still treating it as a VO, but a simple helper like

would come handy while we are creating a new design from scratch. We also wanted to extract the interface to not couple to your library, but it being final would've meant creating and wiring the methods, writing some unit test to check they are correctly linked, etc.

IMHO this is a not the way to go, you're coupling the formatter to the VO. If we did that I would be forcing you to use my formatter whereas you may have very different formatting requirements. That's why you should have the formatter as a totally independent class.

echo MyCustomMoneyFormatter::format($money)

You don't even need to instantiate that class, you don't need to preserve any state, use simple static calls.

It was a (probably bad) example of a method we would probably add to reduce unnecessary code. We are using it store the value in MySQL, where we have a 2-decimal precision limit.

Well, if you use Doctrine2 decimal values are restored as strings in PHP objects, not floats, (and this is great, no precision loss!) so no need for that! Decimal MySQL fields should never be casted to float when entering the PHP realm, they should be kept as strings an operate them using BCMath.

There are methods that may be useful to us but wouldn't be worth the effort adding them to the library. For example, we miss a static constructor where we don't have to instantiate a new Currency object every time (I'd've designed fromAmount() differently, and btw wouldn't the numeric check be in __construct()?), but we don't want to bother you when we don't know if we'll be using this library in a month, or even whether our contributions are worth considering.

Contributions are always worth considering ;) About instantiating a new Currency every time you might be right but then you would need some kind of CurrencyFactory to instantiate new currencies if they weren't instantiated before or return the instance of the EUR currency. I guess it could work but it's simpler this way and the performance gain would be minimal.

About the numeric check, we did it this way because when constructing new instances from the private methods (add, sub...) I am 100% sure that the type will be numeric, a check would be redundant. Hence we only assert the numeric type on public methods where we can't trust the input.

Overall we are happy with it. We weren't aware that it's a proof of concept library (I don't think it's stated in the documentation), but we will continue using it in the future as we find it a useful and well-designed package.

Of course! By proof of concept I meant that this is still an experiment but our plan is to use it "as is" quite heavily in the near future and we don't expect any major changes to it.

[celtric]

I'd suggest using composition if you really need this, it's a cleaner approach than inheritance IMHO.

We will do this once we have time. The problem is that we need static checking, and therefore we will have to manually create the methods and add some unit tests to check that everything is working as expected. This is more work than we were willing to put into an out-of-the-box money library, although now that we have settled for it we'll find some time to do it. Again, if it wasn't final we'd extend it and call it a day.

IMHO this is a not the way to go, you're coupling the formatter to the VO. If we did that I would be forcing you to use my formatter whereas you may have very different formatting requirements. That's why you should have the formatter as a totally independent class.

Our need is very specific, as we were testing three different money libraries and speed was a priority. Once we have the design down we'll correct any flaws and clean the kitchen.

I fully agree with you in that it should not be done this way, but by making it final you're not allowing for a fast prototyping phase (or any other usage that people may find useful). It makes sense to leave it this way when you have full control of an internal library and decide what goes in and what not, but once you make it a public library you have to assume that you're making it another person's internal library, and they should be able to control what fits their own use case.

Well, if you use Doctrine2 decimal values are restored as strings in PHP objects, not floats, (and this is great, no precision loss!) so no need for that! Decimal MySQL fields should never be casted to float when entering the PHP realm, they should be kept as strings an operate them using BCMath.

Yes, it wasn't a good example (although we're using it to save data to MySQL, not restore it). We'll check the code to find any floats being passed to BCMath, as I was not aware that it could lead to problems.

Contributions are always worth considering ;) About instantiating a new Currency every time you might be right but then you would need some kind of CurrencyFactory to instantiate new currencies if they weren't instantiated before or return the instance of the EUR currency. I guess it could work but it's simpler this way and the performance gain would be minimal.
About the numeric check, we did it this way because when constructing new instances from the private methods (add, sub...) I am 100% sure that the type will be numeric, a check would be redundant. Hence we only assert the numeric type on public methods where we can't trust the input.

I'd personally make the constructor public and restrictive (moving the assertNumeric() in there), and use the static constructors just to make life easier to the developer (not performing any checks but translating plain input into objects). Given the low performance hit of such a check, I wouldn't worry too much about the assertion being called on every instantation, but I understand that it's decision you've made consciously.

For the record, this is what I would change:

    /**
     * @param string $amount Amount, expressed as a string (eg '10.00')
     * @param Currency $currency
     *
     * @throws InvalidArgumentException If amount is not a numeric string value
     */
-   private function __construct($amount, Currency $currency)
+   public function __construct($amount, Currency $currency)
    {
+       self::assertNumeric($amount);
+
        $this->amount = $amount;
        $this->currency = $currency;
    }

    /**
     * Convenience factory method for a Money object
     *
     * <code>
     * $fiveDollar = Money::USD(500);
     * </code>
     *
     * @param string $method
     * @param array $arguments
     *
     * @return Money
     */
    public static function __callStatic($method, $arguments)
    {
-       self::assertNumeric($arguments[0]);
        return new self((string) $arguments[0], Currency::fromCode($method));
    }

    /**
     * Creates a Money object from its amount and currency
     *
     * @param numeric $amount
-    * @param Currency $currency
+    * @param Currency|string $currency
     *
     * @return Money
     */
-   public static function fromAmount($amount, Currency $currency)
+   public static function fromAmount($amount, $currency)
    {
-       self::assertNumeric($amount);
+       if (!$currency instanceof Currency) {
+           $currency = new Currency($currency);
+       }
+
        return new self((string) $amount, $currency);
    }

[celtric]

I've ended up creating a fork to solve other issues I had (clashing namespaces, numeric typehinting, etc.). I'm closing the issue then, thanks!

[acasademont]

@celtric keep us posted with your fork, maybe we can agree on some features and merge both forks again in the future! I'm sure that when we start to fully leverage the library we're going to find more issues and update the librearary a bit more.

Thanks for your time :)

[celtric]

Sure! If you consider these issues in the future I'll be happy to delete the fork and continue using your library, as I'd prefer to use you as a source than to maintain a fork myself.

Checking my commits, these are the things I've changed:

• Changed namespace: I previously had problems with the root Money namespace that clashed with mathiasverraes' Money, which made it impossible to use both libraries in the same project.
• Numeric typehint: it threw inspection errors on PHPStorm, and although there's probably a way to fix it, I've preferred to use a more standard approach.
• Allow sub-classing: I've removed the final keyword (sorry, I just had to!) for the reasons pointed out in previous comments.
• Made constructors public: they now strictly enforce their parameters instead of relying on other methods providing valid input. The static constructors are now used as relaxed constructors (right now the only change is that a currency can be passed as a string).
• Minor formatting fixes

I'll keep you updated if some of these decisions come back to bite me and have to rollback!