-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Infinity and early conditions #11
base: master
Are you sure you want to change the base?
Infinity and early conditions #11
Conversation
// cover common, quick use cases | ||
if (num === 1) return str; | ||
if (num === 2) return str + str; | ||
|
||
var max = str.length * num; | ||
|
||
if (max === Infinity) { |
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.
Nice work. Please move this check to be just after the string check (line 46 or so) and change it to if (num === Infinity)
.
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 reason I put it on that line was to leverage the implicit "parseFloat()" when str.length
and num
are multiplied together e.g.
5 * Infinity;
//=> Infinity
5 * 'Infinity';
//=> Infinity
5 * ' Infinity ';
//=> Infinity
That was also motivated by some of the tests that seem to suggest that num
can be given as a string too. I'm happy to make the change but should I still care about the case of Infinity
as a string then?
By the way it seems that if num
can be given as a string then the two shortcuts at L50,51 are most likely ignored. Should this be addressed?
@customcommander nice work! I only had the one comment, everything else is great. thank you! |
This pull request has a few things going on:
var
andconst
. No need to have both so replaced withconst
.Infinity
Infinity
The native
String#repeat
method does throw an error whenInfinity
is given as a parameter. So this changes aligns with the native behaviour. Also there's an issue with howInfinity
works right now: with the same string to repeat, it will produce a different string:This didn't seem right to me so took the liberty to throw an error instead.