Skip to content

alternate implementation, support decimals, extra tests, benchmarking #7

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elidoran
Copy link

I recently needed a library to add commas to numbers and I found your library helpful. Thank you.

I ran into trouble with numbers which had decimal values. So, I've written an alternate implementation which supports decimal values and custom decimal characters for internationalization.

I added tests for decimals and custom decimal characters as well as changed them to use unique digits instead of lots of zeroes (because one bug I had grabbed the wrong part but still worked because flipping around all zeroes was still producing the correct result...).

I also added a benchmark script (using BenchmarkJS) which can be used to compare the previous implementation with the newer implementation. Before changing the current implementation during development, copy it to benchmark/original.js. Then, it's possible to benchmark that version against new changes.

Run the benchmark via node benchmark/run.js. I didn't add an npm run script to package.json because I'm not sure if you'd like it there or not, or if you'd like to add other scripts. If you'd prefer, I could add one named "benchmark" to run the script. Up to you.

The benchmark/original.js file currently has the v1.1.0 implementation. So running the benchmark compares that with my alternate implementation. The image below shows the comparison when run on my dev machine. The first few results of each group are a lot faster because the new implementation takes a shortcut when the number isn't large enough to need separators.

The speed improvement for the rest of the results are due to avoiding the split/reverse/join operations and instead splitting the stringNumber into groups of 3 digits from where the first separator should be.

So, this PR provides:

  1. benchmarking for all future changes (benchmark stuff is ignored by npm so it won't be published)
  2. support for decimal values
  3. customizing the decimal character
  4. and a faster implementation

Please let me know if you're interesting in accepting this PR and if you'd like me to revise it before the merge.

Thank you

screen shot 2016-12-21 at 11 02 49 pm

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

Successfully merging this pull request may close these issues.

1 participant