finnish-bank-utils icon indicating copy to clipboard operation
finnish-bank-utils copied to clipboard

Another go at the decimal check in formatFinnishVirtualBarCode

Open swftvsn opened this issue 7 years ago • 1 comments

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.

swftvsn avatar Jan 15 '18 09:01 swftvsn

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.

swftvsn avatar Jan 15 '18 11:01 swftvsn