finnish-bank-utils
finnish-bank-utils copied to clipboard
Another go at the decimal check in formatFinnishVirtualBarCode
Quite a long post, but bear with me here..
I was a bit hasty with the fix in #11. First of all, the method should always fail if too many decimals are given. This is enforced in line 344 using
object.sum != Number(object.sum.toFixed(2))
However, due to javascript decimal handling (or lack thereof, where are thou BigDecimal?) the check fails under certain circumstances: try the following code in chrome dev tools to see the problem:
450.459999999999999927 != Number(450.459999999999999927.toFixed(2))
One would assume that
450.459999999999999927 != 450.46
but for some reason false is returned. I think it is due to the internal representation of .46 to be 45999...
Combine this to the naive floor I added, it will yield one cent off results in some case. (Should use Math.round()
)
Anyway, we can either fix the check, or make the method round the cents to be sure it displays it correctly. This would however change how the method works.
On the other hand I fear that if we fix the check, there will be certain sums that will not be accepted even though the caller does everything correctly. (I feed the sum like this Number(numero.round(2, RoundingMode.RoundHalfUp).toFixed(2))
to the function where numero
is bigdecimal from big.js: https://github.com/MikeMcl/big.js/, and I get corrupted virtual codes.)
So my proposal to fix this is to remove the decimal place check and round the cents to get correct results.
I'll prepare PR (with additional tests) for this and you can decide how to proceed.
The long term fix would be to accept the number in string format '123.45' and require that exact format with two decimal places. Then it would be up to the caller to round etc. But this would be a breaking change.
Decided to change the sums from number to string. Let me know if you don't want to incorporate this change, so I'll know to publish the fork also to npm with different name.