-
Notifications
You must be signed in to change notification settings - Fork 741
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
Store alphabet internally as object (fixes #309 and #250) #310
base: master
Are you sure you want to change the base?
Conversation
Your implementation is quite neat - I like the way the BigNumber constructor function is simplified with the removal of two variables etc. - but: const log = x => { console.log(x.toString()) };
// Default ALPHABET.
BigNumber.set({ ALPHABET: '0123456789abcdefghijklmnopqrstuvwxyz' });
log(new BigNumber('ff', 16)); // '255'
log(new BigNumber('FF', 16)); // '255'
log(new BigNumber('Ff', 16)); // '255'
BigNumber.set({ ALPHABET: '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ$_' });
log(new BigNumber('ff', 16)); // '255'
log(new BigNumber('FF', 16)); // 'NaN' !?
log(new BigNumber('Ff', 16)); // 'NaN' !? Is this intended? Surely not. The hexadecimal alphabet is the same in both ALPHABETs above, so the values logged should be the same for each. To be honest, I am not sure it is worth you spending any more time on this. I am happy with the existing behaviour, and have documented it now and also fixed #250. |
for (j = 0; j < i; j++) { | ||
d = alphabet.charAt(j); | ||
if (c.toLowerCase() === d.toLowerCase()) { | ||
caseSensitive = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to break
out of the loops as soon as caseSensitive
is true
.
|
||
// case-insensitive version of indexOf | ||
function charIndex(c, b) { | ||
var i = caseSensitive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of the charIndex
function could depend on the value of caseSensitive
, rather than checking its value every call.
This change does not modify the external contract of the library.
This change replaces the internal ALPHABET string with an object of the form:
I've tested this locally and everything seems to function as I expect. I also modified the existing unit tests for the BigNumber constructor to use mixed-case strings.