path: reduce path code size #25278
path: reduce path code size #25278
Conversation
|
It would be nice to get a review here. |
|
I am not sure whom to ping for a review. Any suggestions from anyone? |
|
I tried to review a while ago after your last plea, but sorry, I didn't get anywhere. This adds about as many lines as it removes, and while some of the changes were clearly refactors, for a number, as someone who has never read path.js before, it wasn't at all clear from just the diff context that the changed code had the exact same behaviour as before. I think that is just the nature of the change, I'm not sure if there is much you can do about that, but you could pull out all the things that are clearly 1-1 refactors into a different PR, maybe? Maybe pull the benchmarks out as well? If you get the easy to review stuff out of this, what is left might be able to get more attention. Path is a sensitive module, tiny changes have cuased big user-land breaks in the past, so I suspect its not just me who is reluctant to rubber-stamp changes I don't fully understand. Have you git logged to see who has touched this file before? |
BridgeAR
force-pushed
the
refactor-path
branch
from
3b9f509
to
755b99c
Compare
|
CI https://ci.nodejs.org/job/node-test-pull-request/20348/
Yes, the only other collaborator who has recently worked on this was @targos and @Trott if I am correct. It would be nice if you could also have a look.
I believe the change should be done in a single PR but I split the big code parts into lots of smaller commits which should be significantly easier to read and I added a more detailed commit description about each code change done in such a commit. I hope that'll help reviewing the code (please check commit after commit). @mscdex it would be nice if you could also have a look at the code :) |
|
I'm afraid to review such a big change in the |
|
@targos would you feel comfortable reviewing some of the commits? :-) After splitting all changes into smaller commits it's hopefully also easier to see what changed and what not (some are of course easier to review than others). None of my changes should change any behavior. I only changed conditions after closely inspecting the logic to make sure everything keeps on working as before. |
|
@ reviewers, please also disable whitespace changes while comparing the code. That way a couple of commits are significantly easier to review. |
|
@nodejs/collaborators PTAL |
|
Test changes LGTM, but that's just 1 of 12 commits.... |
|
(The test changes seem independent of the rest of the changes, no? If you want to try to land this piecemeal, that can be moved into its own PR and would likely be reviewed and landed quickly.) |
|
I'll go ahead and land this on the 25th if no one disagrees about that until then and I am going to squash the commits into three (benchmark, test and path). |
1) This uses some ternary expressions instead of if else to assign some variables. 2) Use template strings instead of concat. 3) Use the object shortand notation. 4) Some var to let / const. 5) Removed some double line breaks. 6) Less brackets around statements if not necessary.
1) Refactor for loops to while loops that were only meant to count up a variable. 2) Refactor some `var` statements to `let` / `const`. 3) Simplify return conditions. 4) Use template strings where possible instead of concat. 5) Use ternary expressions for variable assignments instead of if / else. 6) Use the object shorthand notation for the function declarations. 7) Consolidate if else case where possible. 8) Remove double line breaks.
1) Consolidate format to a single function.
2) Move some code that can only be reached in some code branches
that was formerly executed in all cases.
3) Explicitly check for the string length of zero instead of
converting the string to a boolean.
4) Consolidate nested if statements if possible e.g.,
if (foo) { if (bar) { /* do stuff */ } }
to reduce indentation depth.
5) Simplify checks by removing extra length checks when comparing
two strings.
6) Use object shorthand notation where possible.
1) Consolidate nested if statements if possible
`if (foo) { if bar () { /* do stuff */ } }`)
to reduce indentation depth.
2) Remove obsolete else cases to reduce indentation.
This refactoring makes sure some code branches will not be hit if they do not have to be reached.
Either `end` is `-1` or `startPart` is not `0`. Therefore it's possible to move the conditions in a way that we eliminate a few code branches.
This moves the `if (len === 1)` case to the top of the function. That way it is possible to reduce the indentation level due to returning early in that case. On top of that the following was done: 1) For clarity refactored for loops which were meant to count up a variable into a while loop. 2) Used template strings instead of string concat. 3) Consolidating nested if statements. 4) Using tenary expressions if applicable when assigning variables to reduce the code overhead.
This moves a condition inside of a for loop which can only be triggered at the very end of the for loop outside of the loop. That way the for loop itself is much simpler and easier to understand and the code itself is less indented which should increase the readability. It also refactors some `var` to `let` and `const`.
BridgeAR
force-pushed
the
refactor-path
branch
from
755b99c
to
5da1b22
Compare
|
I decided to pull out some commits so that at least those get some more reviews. This reduced the overall code changes. I also reverted some The commits that are still in here did not change besides resolving @targos comments. @sam-github would you maybe have the time to take another look? |
BridgeAR
added
the
author ready
|
It looks mostly like a refactor now. It looks like targos already approved it, so I think you're good? |
|
@targos would you be so kind and confirm your LG? I would then go ahead and land this as is. |
|
I don't really have the time to review a second time. If all you did apart from fixing my comments is remove the benchmark and test changes, then I confirm my LG. |
1) This uses some ternary expressions instead of if else to assign some variables. 2) Use template strings instead of concat. 3) Use the object shortand notation. 4) Some var to let / const. 5) Removed some double line breaks. 6) Less brackets around statements if not necessary. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
1) Refactor for loops to while loops that were only meant to count up a variable. 2) Refactor some `var` statements to `let` / `const`. 3) Simplify return conditions. 4) Use template strings where possible instead of concat. 5) Use ternary expressions for variable assignments instead of if / else. 6) Use the object shorthand notation for the function declarations. 7) Consolidate if else case where possible. 8) Remove double line breaks. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
1) Consolidate format to a single function.
2) Move some code that can only be reached in some code branches
that was formerly executed in all cases.
3) Explicitly check for the string length of zero instead of
converting the string to a boolean.
4) Consolidate nested if statements if possible e.g.,
if (foo) { if (bar) { /* do stuff */ } }
to reduce indentation depth.
5) Simplify checks by removing extra length checks when comparing
two strings.
6) Use object shorthand notation where possible.
PR-URL: nodejs#25278
Reviewed-By: Michaël Zasso <[email protected]>
1) Consolidate nested if statements if possible
`if (foo) { if bar () { /* do stuff */ } }`)
to reduce indentation depth.
2) Remove obsolete else cases to reduce indentation.
PR-URL: nodejs#25278
Reviewed-By: Michaël Zasso <[email protected]>
This refactoring makes sure some code branches will not be hit if they do not have to be reached. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
Either `end` is `-1` or `startPart` is not `0`. Therefore it's possible to move the conditions in a way that we eliminate a few code branches. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
This moves the `if (len === 1)` case to the top of the function. That way it is possible to reduce the indentation level due to returning early in that case. On top of that the following was done: 1) For clarity refactored for loops which were meant to count up a variable into a while loop. 2) Used template strings instead of string concat. 3) Consolidating nested if statements. 4) Using tenary expressions if applicable when assigning variables to reduce the code overhead. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
This moves a condition inside of a for loop which can only be triggered at the very end of the for loop outside of the loop. That way the for loop itself is much simpler and easier to understand and the code itself is less indented which should increase the readability. It also refactors some `var` to `let` and `const`. PR-URL: nodejs#25278 Reviewed-By: Michaël Zasso <[email protected]>
|
Thanks everyone! Landed in 410eb97...0fd5b45 🎉 |
1) This uses some ternary expressions instead of if else to assign some variables. 2) Use template strings instead of concat. 3) Use the object shortand notation. 4) Some var to let / const. 5) Removed some double line breaks. 6) Less brackets around statements if not necessary. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
1) Refactor for loops to while loops that were only meant to count up a variable. 2) Refactor some `var` statements to `let` / `const`. 3) Simplify return conditions. 4) Use template strings where possible instead of concat. 5) Use ternary expressions for variable assignments instead of if / else. 6) Use the object shorthand notation for the function declarations. 7) Consolidate if else case where possible. 8) Remove double line breaks. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
1) Consolidate format to a single function.
2) Move some code that can only be reached in some code branches
that was formerly executed in all cases.
3) Explicitly check for the string length of zero instead of
converting the string to a boolean.
4) Consolidate nested if statements if possible e.g.,
if (foo) { if (bar) { /* do stuff */ } }
to reduce indentation depth.
5) Simplify checks by removing extra length checks when comparing
two strings.
6) Use object shorthand notation where possible.
PR-URL: #25278
Reviewed-By: Michaël Zasso <[email protected]>
1) Consolidate nested if statements if possible
`if (foo) { if bar () { /* do stuff */ } }`)
to reduce indentation depth.
2) Remove obsolete else cases to reduce indentation.
PR-URL: #25278
Reviewed-By: Michaël Zasso <[email protected]>
This refactoring makes sure some code branches will not be hit if they do not have to be reached. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
Either `end` is `-1` or `startPart` is not `0`. Therefore it's possible to move the conditions in a way that we eliminate a few code branches. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
This moves the `if (len === 1)` case to the top of the function. That way it is possible to reduce the indentation level due to returning early in that case. On top of that the following was done: 1) For clarity refactored for loops which were meant to count up a variable into a while loop. 2) Used template strings instead of string concat. 3) Consolidating nested if statements. 4) Using tenary expressions if applicable when assigning variables to reduce the code overhead. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
This moves a condition inside of a for loop which can only be triggered at the very end of the for loop outside of the loop. That way the for loop itself is much simpler and easier to understand and the code itself is less indented which should increase the readability. It also refactors some `var` to `let` and `const`. PR-URL: #25278 Reviewed-By: Michaël Zasso <[email protected]>
This is mainly a code cleanup. It reduces the overall code size and code indentation of the
pathmodule and since it's quite complicated code, I thought it makes sense to try to have the code base as small and readable as possible.The performance is pretty much identical. There are a view small plus or minus percent on some cases without any clear performance difference.
I changed the benchmarks as well to better reflect actual user input.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes