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

Custom ALPHABET ignored for base 10 #250

Closed trbrc closed 2 years ago

trbrc commented 4 years ago

ALPHABET appears to be hardcoded to '0123456789' when the base is 10.

const ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
BigNumber.config({ALPHABET});

for (let base = 2; base <= ALPHABET.length; base++) {
  console.log(
    base,
    new BigNumber('BA', base).toString(),
    new BigNumber('10', base).toString()
  );
}

Log:

2 "2" "NaN"
3 "3" "NaN"
4 "4" "NaN"
5 "5" "NaN"
6 "6" "NaN"
7 "7" "NaN"
8 "8" "NaN"
9 "9" "NaN"
10 "NaN" "10"
11 "11" "NaN"
12 "12" "NaN"
13 "13" "NaN"
14 "14" "NaN"
15 "15" "NaN"
16 "16" "NaN"
17 "17" "NaN"
18 "18" "NaN"
19 "19" "NaN"
20 "20" "NaN"
21 "21" "NaN"
22 "22" "NaN"
23 "23" "NaN"
24 "24" "NaN"
25 "25" "NaN"
26 "26" "NaN"
MikeMcl commented 4 years ago

Yes.

The problem is that I want to keep BigNumber creation as fast as possible when a base argument of 10 is passed to the constructor, because many users include it as a matter of course.

Currently, a time-consuming base conversion is avoided (and exponential notation is also accepted) using

// Allow exponential notation to be used with base 10 argument, while
// also rounding to DECIMAL_PLACES as with other bases.
if (b == 10) {
  x = new BigNumber(v);
  return round(x, DECIMAL_PLACES + x.e + 1, ROUNDING_MODE);
}

Yes, I could change that to

if (b == 10 && ALPHABET.slice(0, 10) === '0123456789') {

but I don't want to add further code to a very hot path in the constructor just to cover a case which nobody probably uses.

I may just make it explicit in the documentation that that first 10 characters of the alphabet are hard-coded to '0123456789' for base 10.

trbrc commented 4 years ago

Yes, I could change that to

if (b == 10 && ALPHABET.slice(0, 10) === '0123456789') {

but I don't want to add further code to a very hot path in the constructor just to cover a case which nobody probably uses.

Could it do this check once when a new ALPHABET is set from config, and just use a boolean flag here?

Just adding it to the documentation would work OK as well. On the other hand, since it is possible to opt out by not specifying the base, wouldn't it be better to fix it and document the performance implications instead?

MikeMcl commented 4 years ago

Could it do this check once when a new ALPHABET is set from config, and just use a boolean flag here?

Yes, I'll consider it.

MikeMcl commented 2 years ago

Fixed in v9.0.2. Thanks for your input.