MikeMcl / bignumber.js

A JavaScript library for arbitrary-precision decimal and non-decimal arithmetic
http://mikemcl.github.io/bignumber.js
MIT License
6.69k stars 742 forks source link

inconsistent behavior when using DECIMAL_PLACES config #216

Closed Ratniex closed 5 years ago

Ratniex commented 5 years ago

Upon carefully reading the docs it works as advertised, however I cannot understand why DECIMAL_PLACES config is not taken into account when:

bn.config({DECIMAL_PLACES: 2}) const n = bn(3.3333333333)

console.log(n) // BigNumber { s: 1, e: 0, c: [ 3, 33333333330000 ] }

console.log(n.toNumber()) // 3.3333333333 console.log(n.toString()) // 3.3333333333 console.log(n.toFixed()) // 3.3333333333

// only the next line is rounded console.log(n.div(2)) // BigNumber { s: 1, e: 0, c: [ 1, 67000000000000 ] } console.log(n.times(2)) // BigNumber { s: 1, e: 0, c: [ 6, 66666666660000 ] } console.log(n.plus(2)) // BigNumber { s: 1, e: 0, c: [ 5, 33333333330000 ] } console.log(n.minus(2)) // BigNumber { s: 1, e: 0, c: [ 1, 33333333330000 ] }


I understand this is a somewhat serious breaking change, however I would still like to have a discussion on this. Perhaps this is a feature request for another config option.
MikeMcl commented 5 years ago

I see no reason to revisit a fundamental design decision which makes it easy to avoid losing precision.

If a limited number of decimal places is desired for the result of an operation not involving division, then it is as simple as, for example

n.toString();                              // "3.3333333333"
n.times(2).toFixed(2);                     // "6.67"
n.times(2).decimalPlaces(2).toString();    // "6.67"
n.times(2).dp(2).toString();               // "6.67"
Ratniex commented 5 years ago

For my use case I will wrap the library and decorate each arithmetic method by calling .decimalPlaces(bn.config().DECIMAL_PLACES) after it. It seems this is what you propose.

If having more precision is the goal itself than I cannot understand why such a config opt is necessary at all (I cannot imagne a realistic use case where someone needs more than 20 significant digit precision and is using JS). In this case I would propose to change the API documentation to be more explicit about working with 'only' operations involving division. Also do not use '5' as an example.

I am sorry if I am a being salty. This is a great lib.

MikeMcl commented 5 years ago

To add, for example, a new add method which rounds its result to the current DECIMAL_PLACES value, it would be more efficient to use the following, as specifying base 10 means a value will be rounded like it would with any other base conversion.

BigNumber.prototype.add = function (n) {
  return new BigNumber(this.plus(n), 10);
}

// Example usage

BigNumber.set({ DECIMAL_PLACES: 3 });
x = new BigNumber(1234.56789);

x.plus(1).toString();           // "1235.56789"
x.add(1).toString();            // "1235.568"

If you also want the argument passed to the add method to be rounded before it is added, then it would be

  return new BigNumber(this.plus(n, 10), 10);

In the same way, if you want a value to be rounded when creating a new instance, how about

BigNumber.create = function (n) {
  return new BigNumber(n, 10);
};

// Example usage

BigNumber.set({ DECIMAL_PLACES: 3 });
x = BigNumber.create(1234.56789);
x.toString();                   // "1234.568"

See the Wiki for an example of how to create a custom BigNumber with configuration and added prototype methods.

The Use section of the README is essential reading and it shows clearly that only operations involving division are rounded. Note that if, as you would prefer, all operations were rounded, then a user could never be sure of getting an exact result unless the size and precision of the values involved was known beforehand.

It would be possible to add something like a ROUND_ALL config option, but there has been no demand for that before, and it is simple and more explicit to just add a decimalPlaces call.

I don't know where you are referring to when you say "don't use 5 as an example".

By the way, the sister library decimal.js does round all operations to the current precision setting.

Thanks for the feedback.