Implementation / performance

[BenMorel]

Part of the list of things I want to work on before tagging a release, as mentioned in #2. Comments welcome.

The library is fully based on Brick\Math's BigDecimal. This is good, as it uses precise arithmetic. However, I feel like it's a bit overkill when you typically deal with monies of reasonable size, that could be easily represented with integers: you can fit up to ~20 million USD with 2 decimals on 32-bit, and an unfathomable amount on 64-bit.

It's interesting to note that the Java Money reference implementation provides two classes: Money based on BigDecimal, and FastMoney based on long.

Sure, we don't have the same speed requirements as a Java application: we won't use this library for stuff like real-time trading, but using GMP or BCMath under the hood when you're just adding/subtracting everyday monies is questionable.

Note that even if we had an integer-based implementation, integer arithmetic could be used only for the simplest calculations: we'd still need to use BigDecimal for things like division, some multiplication, and probably rounding.

My current view on this is: if the library is kept as it is today (a single Money class), I don't think it's worth having two implementations. However, as suggested in #3 Scales and Money classes, if we followed the Money/BigMoney route, we could consider going with int for Money, and with BigDecimal for BigMoney.

[fprochazka]

I personally prefer correctness over speed. So having 1 impl with BigDecimal makes sense to me :+1:

[BenMorel]

Correctness would be 100% guaranteed with integers as well (no floats involved), and backed by unit tests. Within the int boundaries of course, but in the unlikely event that you'd overflow the integer, you'd get an exception. But I get the point of the simplicity of a single implementation.

[jkuchar]

I had ever no problem with current speed of brick\money.

If you feel that speed optimization is needed, why not to delegate this optimization to BigInteger. It can use integers when possible and seamlessly switch do strings before overflow. This is what python does.

[BenMorel]

It is true that the difference is not huge. I did measure at 5-10x performance improvement when adding two experimental monies internally using int, but we're talking about going from 1 million to 5 million operations per second, so unless you're heavily (ab)using the library, the difference will be hard to tell.

Plus we have to internally convert to BigDecimal when adding something other than a Money or an int, when multiplying, dividing, and for pretty much all other operations, and the conversion can even make some operations a bit slower, so maybe it's not really worth the effort.

I think what really motivated me to attempt an int-based money, is to natively have an int at hand to store my monies in the database, as mentioned in #3. Due to the different possible scales for different currencies, I store my monies in the db as integers, in minor units (cents), and not as (fixed scale) decimals:

Basically what currently goes into my db is:

(int) $money->getAmountMinor(), $money->getCurrency()->getCurrencyCode()

And I can reload it from the DB with:

Money::ofMinor($minorAmount, $currencyCode);

Currently getAmountMinor() returns a string, and its behaviour depends on the scale of the Money: 1.23 USD will return '123', while 1.2345 USD will return '12345', which can lead to confusion when re-loading with Money::ofMinor() if you don't pay attention to the scale.

Hence my suggestion to split into a Money that would be int-based, have a static scale based on the currency, and have a minor amount that would always be returned as an integer (that's how the amount would be stored internally) -- and a BigMoney, that would be based on BigDecimal. But this does open a can of worms as well. I need to review your use case in #3 in more detail and maybe find a better solution.

[jkuchar]

I do not like that idea that Money should manage details of how numbers are added. That is job of brick\math. Why not to delegate usage of integers or strings to BigInteger/BigDecimal? They can use integers under the hood and if needed they can anytime switch to string representation.

This means there will be always the same Money API independently if there will or won't be used integers internally. If you know that you do not need bigger numbers than int you can always use $money->getAmountMinor()->toInteger(), which will perform no conversion if it is stored as int internally and throws exception if number is bigger then int.

[BenMorel]

You're probably right about that. Note that I will probably change the return type of Money::getAmountMinor() to BigInteger, so that I can call toInt() on it, which would be cleaner than casting a string to int (and immune to integer overflow, if any).

Also, I think that it's a mistake that currently, getAmountMinor() returns the unscaled value of the internal BigDecimal.

For example, USD 1.2300 should return 123 as minor amount, not 12300. It should always be expressed in terms of official minor currency units, not relative to the current scale (we should probably provide another method for this, though).

This will ensure that even if I accidently get a Money with a custom scale, the correct value will be sent to the database.

That's also what Joda Money does, even in BigMoney: it always uses the currency's default scale.

Note that this is a point against allowing multiple instances of a given currency with different scales (#3): we need to know, from the Currency object, what the official scale is, for ofMinor() / getAmountMinor() to work properly.

[jkuchar]

Agree on both points.

[jiripudil]

One very specific thing to consider is support for cryptocurrencies. As I've noted in #5, Ethereum has an alarming scale of 18, which means that, if you were to follow the specification to the letter, you couldn't fit a $0.0000007 worth of ethers in a 32-bit integer, and a 64-bit integer would overflow at 9.223372036854775808 ETH.

Anyway, I haven't had any performance issues with Brick/Money either, so I think the gain from using integers is not worth the trouble – in Money library, that is. :+1: for delegating the decision to the underlying math library though

[BenMorel]

Wow that is a good point against integers, thanks for bringing it up!

I agree with you both then, we should keep the implementation based on Brick\Math only. As the discussion tends to lean towards keeping a single Money class, too, it would make less and less sense to have an int-based implementation anyway.

[VasekPurchart]

I agree that the performance should probably not be a problem and only want to add, that the integer space can be depleted much faster, than you think even for "usual" currencies, because since you wan to operate in minor amount most of the time, that means two orders already out of the window. I remember we had a lot of problems with this even in CZK with 32bit on single item representations - they were over 20M. And some currencies have almost "ridiculous" nominal value, where you can literally have millions in your pocket on daily bases, I don't know how accurate is this, but in general terms probably is: http://www.telegraph.co.uk/personal-banking/current-accounts/worlds-least-valuable-currencies/vietnamese-dong/.

[BenMorel]

Thanks for jumping in. It's indeed a potential issue, especially as long as PHP ships with 32-bit executables. And as @jiripudil said, even 64-bit integers can be depleted fast with some cryptocurrencies! Better stay on the safe side indeed.

[VasekPurchart]

I think that it's a mistake that currently, getAmountMinor() returns the unscaled value of the internal BigDecimal.

Also definitely agree on this one, this would probably surprise me at some point (probably unpleasantly :D).

[BenMorel]

To summarize:

• we're keeping everything based on Brick\Math internally
• we'll expose a getMinorAmount() method that returns a BigDecimal always based on the Currency's default scale (USD 1.23 => 123, USD 1.2345 => 123.45); this can be converted toBigInteger() or toInt() if needed, throwing an exception if not convertible.
• we'll expose a getUnscaledAmount() method that returns a BigInteger with what was previously returned by getAmountMinor()