Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom ALPHABET ignored for base 10 #250

Closed
trbrc opened this issue Nov 9, 2019 · 4 comments
Closed

Custom ALPHABET ignored for base 10 #250

trbrc opened this issue Nov 9, 2019 · 4 comments

Comments

@trbrc
Copy link
Contributor

trbrc commented Nov 9, 2019

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
Copy link
Owner

MikeMcl commented Nov 10, 2019

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
Copy link
Contributor Author

trbrc commented Nov 10, 2019

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
Copy link
Owner

MikeMcl commented Nov 10, 2019

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
Copy link
Owner

MikeMcl commented Dec 12, 2021

Fixed in v9.0.2. Thanks for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants