-
Notifications
You must be signed in to change notification settings - Fork 12
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
Substr #10
Substr #10
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 28 47 +19
=====================================
+ Hits 28 47 +19
Continue to review full report at Codecov.
|
@nainemom hi, why did you close the PR? :) |
@sallar Hi. I just try to understand why @codecov-io did this to me :D. My code coverege is 97.56% and misses one thing! I opened that again, but how can i do this test, before submitting PR? |
@nainemom when you run |
@sallar Ok i will fix that :) |
@sallar check this again. |
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.
I have requested some (mostly styling) changes. Also please add the proper documentation for this function to README
. Thank you.
src/index.js
Outdated
} | ||
// Calculating postive version of negative value. | ||
else if(begin < 0) { | ||
begin+= strLength; |
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.
Space needed between begin
and +=
src/index.js
Outdated
} | ||
|
||
let end = undefined; | ||
if(typeof len === "undefined"){ |
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 whole section needs a bit of reformatting. suggestion:
let end;
if (typeof len === "undefined"){
end = strLength;
} else {
// Fix type
if (typeof len !== "number"){
len = parseInt(len, 10);
}
if (len >= 0) {
end = len + begin;
} else {
end = begin;
}
}
src/index.js
Outdated
|
||
// Fix type | ||
if (typeof begin !== "number") { | ||
begin = parseInt(begin); |
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.
parseInt(begin)
needs to have a radix of 10. parseInt(begin, 10)
src/index.js
Outdated
return ""; | ||
} | ||
// Calculating postive version of negative value. | ||
else if(begin < 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.
Since the previous if
returns something, this doesn't need to be else if
and a simple if
is enough. Also space needed between if
and (
. Please use ESLint to see all the styling issues.
src/index.js
Outdated
} | ||
|
||
// Return zero-length string if got oversize number. | ||
if(begin >= strLength){ |
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.
Space needed between if
and (
;
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.
👍 Great job.
As you know, the substring and substr function are different. substring parameters are begin and end point from string, but substr accept begin and length. Also some people use substr more than substring (like me), so i write it for stringz.