Conversation
By The way, the algorithm this PR used should be the most efficient so far. There are two reference about repeating one char N times can prove it. 1) Stackoverflow 2) ES6 String.repeat() implementation Any comments are welcome! |
if (!ch && ch !== 0) ch = ' '; | ||
|
||
len = len - str.length; | ||
if (len <=0) return str; |
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.
missing whitespace before 0
while (++i < len) { | ||
str = ch + str; | ||
ch = ch + ''; | ||
pad = ''; |
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.
var
must be added, or pad will be a global variable. Don't pollute global variables.
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.
That's why I add 'use strict'
in all my js files. You should too.
perfect. +1 |
Did some benchmarks here https://github.com/standy/bencmark-leftpad |
How about adding some test cases for those Unicode characters they have now? |
@haoel Would you like to submit a PR with benchmark tests so we can compare current and future PRs? Thanks. |
Benchmark Report3 Left-Pad VersionsI have three version of left-pad functions will be tested
(Note: the 3 Test CasesI will have three groups of test cases as below:
Each test case will be run 1 million times for each version of left-pad. And we will have 5 rounds. Test ResultWe can see (time: ms) Test Source Code
Please feel free let me know any of concerns. --Hao |
One-char pad time measurement is mostly noise, consider increasing loop count and taking But hey, why should we use correct implementation in Benchmark.js that is available for years. |
Some thoughts of mine after reviewing the code.
|
@polkovnikov-ph Thanks for the comments. And the reasons I didn't use
So, I just want to keep it simple & stupid. ;-) However, the fact is I didn't know benchmark.js before you mentioned it, because I am a C/C++/Java/Go/Python developer instead of JS developer, ;-) Anyway, I re-run the performance test based on the Benchmark.js, the result as below:
|
@@ -3,15 +3,18 @@ module.exports = leftpad; | |||
function leftpad (str, len, ch) { | |||
str = String(str); |
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.
str = '' + str;
how about?
http://jsperf.com/number-to-string/2
@haoel Wow, it's really faster than a builtin function. It seems JS interpreter is using something like a copy-on-write rope, and won't instantiate the whole string unless needed. There is an indirect evidence of this by the fact that (I'm a Scala/C++ guy too. I've just needed it a year ago while rewriting hyphenator.js. Microbenchmarking is an extremely tricky thing, in no circumstances I'd opt doing it myself.) |
While there is not any dramatic difference, typecasting does slow things down a little bit. It may seem like splitting hairs, but while Haoel's function is possibly the smartest way to left-pad, the following is marginally faster. For those who depend on shaving milliseconds, I present to you Haoel's left-pad, ever so slightly improved.
The only real difference is I've removed the typecasting for |
@dainbrump your version looks good, I will update this pull request. |
… to make it a bit faster
What about caching common use-cases like what camwest#5 does? (Is |
"license": "WTFPL" | ||
"license": "WTFPL", | ||
"dependencies": { | ||
"benchmark": "^2.1.0" |
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.
this should be devDependencies
, i've already created a pull-request haoel#1
@haoel: Considering what @stevemao shared and considering the default use case is to pad with spaces, what do you think about using an array of pre-padded strings ranging from 1 to 10 spaces and a conditional block when the default case is needed. Seems to be faster when I tested.:
|
chore: move benchmark to devdep, add bench script
@dainbrump the cache is a good idea. The only concern seems we only can do it for some special chars (like the space in your example). Some other cases might need to pad '0' or other chars. In my opinion, if we cannot generalize the code, then we have to balance between the performance and the clean code. For this case, I am inclined to go the way of the clean code. But, I am fully openning for any of other different opinions. |
Can you remove the comments? I'm happy to merge this if no one oppose? |
.run(); | ||
|
||
/* | ||
function test(fn, str, len, times) { |
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.
Sorry, keep the useful comments. I'm talking about this block.
Sorry for misunderstanding. and it's done! |
@@ -0,0 +1,69 @@ | |||
function leftpad_orginal (str, len, ch) { |
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.
Sorry, I've been busy with other stuff. If you have time could you put this function in a standalone js file and name it O(n)
? Similar to the functions below. and put them in a bench
folder. If not I'll do it myself. Thanks.
@stevemao I am busy on other project as well, please feel free do any change you like! :-) |
this should finish what @stevemao said in left-pad#11
@haoel you have to merge my PR against yours first and you're welcome |
Using while-loop to keep adding the padding char one by one is not efficient.
Actually we can do it better. We can use bit operation to keeping doubling the padding chars which can significantly reduce the string concatenation operation.
The final code as below ( Update on March 29 ):