-
-
Notifications
You must be signed in to change notification settings - Fork 729
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
remember index to reuse in else statement; avoid recalling indexOf #182
Conversation
Travic CI failure is due to new linting seeing unnecessary escapes. I fixed those in a new PR #184. Also, linting required a more verbose form. // instead of this:
var pos = (pos = part.indexOf(']=')) === -1 ? part.indexOf('=') : pos + 1;
// had to change it to this:
var pos = part.indexOf(']=') + 1;
if (pos === 0) {
pos = part.indexOf('=');
} |
788a2a7
to
641c32d
Compare
lib/parse.js
Outdated
var pos = part.indexOf(']=') === -1 ? part.indexOf('=') : part.indexOf(']=') + 1; | ||
|
||
var pos = part.indexOf(']=') + 1; | ||
if (pos === 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 isn't the same condition - the original check here was that pos
was -1
.
Yes, I changed it intentionally. It could have been: var pos = part.indexOf(']=');
if (pos === -1) {
pos = part.indexOf('=');
} else {
pos++;
} I removed that else block by having it do the +1 in the first operation and altering the comparison value by +1, making -1 into 0. |
Right, but |
When it's above zero, then it found What it does is:
So, it will either end up with a Would you like me to change it to |
I'm not sure if i'm being clear for the spot you're looking at. The test for zero is a test for -1. It just happens to be zero because I did +1 to the result of the indexOf operation because, if it finds it, we want the +1 value so it points at the equal sign, not the bracket. So, the |
@elidoran aha, i see what you mean now. Thanks for clarifying. I think I'd prefer the if/else for clarity, given my confusion here :-) |
@elidoran are you still interested in completing this PR? If so, please either check the "allow edits" checkbox in the righthand column, or make the change to if/elses and rebase :-) Thanks! |
(I've just noticed today that the "allow edits" checkbox is now checked, thanks) |
641c32d
to
e25af9b
Compare
Avoid running the same
indexOf(']=')
operation twice by storing the result and reusing in the else statement.