refactor: reduce repeated code in which()#659
Conversation
src/which.js
Outdated
| } else { | ||
| return p.split(':'); | ||
| } | ||
| return p.split(path.delimiter); |
There was a problem hiding this comment.
Or a good ol' ternary statement:
return p ? p.split(path.delimiter) : [];
src/which.js
Outdated
| } | ||
|
|
||
| // Search for command in PATH | ||
| pathArray.forEach(function (dir) { |
There was a problem hiding this comment.
Here, we're looping through the path array, and setting where under certain conditions, then returning from the forEach callback. On the next call, we check if which was set, and if it was, we don't continue. Finally, at the end of checkPath, we return where if it was set. To me, it seems like a regular for loop would be a better choice here, because we could return where from the function immediately.
There was a problem hiding this comment.
Yeah, I agree. Index-based for loops kind of suck in JS, but it's probably still better than this exit-early-if-we-found-it approach. I will adjust.
src/which.js
Outdated
|
|
||
| return where; | ||
| // Command not found anywhere? | ||
| return null; |
There was a problem hiding this comment.
Hmm... Maybe an empty string should be returned here? All of the other paths return strings, and '' is falsy like null is.
There was a problem hiding this comment.
null is better here because it won't get wrapped by ShellString. The empty string would get wrapped, and ShellString('') is actually truthy, unlike ''.
f3c4540 to
64ec9f1
Compare
|
@freitagbr PTAL. Significantly refactored things, hopefully this is a cleaner approach, with less code duplication. |
src/which.js
Outdated
| // This is the value to return if we haven't found any matches | ||
| noMatches: options.all ? [] : null, | ||
| value: [], | ||
| }; |
There was a problem hiding this comment.
This is better than it was before, but this is still a little complicated. On the one hand, this generalizes the two options more clearly, but on the other hand, dealing with one-off objects is a bit messy.
There was a problem hiding this comment.
Sure, I'll take away some of the abstraction
src/which.js
Outdated
| return; | ||
| } | ||
| // Otherwise, see if it exists without the extension | ||
| var newAttempt = attempt + ext; |
There was a problem hiding this comment.
This is a bit confusing. We're checking if it exists "without the extension", but then we add the extension?
There was a problem hiding this comment.
Yeah, I'll see if I can make that better. The basic idea is this:
- Handle the case where they call
which('node.exe') - If they instead did
which('node'), then check fornode.exe,node.bat,node.cmd, etc.
64ec9f1 to
59704cf
Compare
|
@freitagbr please see if this next set of changes improves clarity |
src/which.js
Outdated
| } else { | ||
| // Assume it's Unix-like | ||
| var match = attempt.match(/\.[^<>:"/\|?*.]+$/); | ||
| if (match && pathExtArray.search(match[0]) >= 0) { // this is Windows-only |
There was a problem hiding this comment.
I think indexOf should be used here, instead of search.
There was a problem hiding this comment.
Ah, this was a mistake. Array.prototype.search isn't even defined... indexOf is what I was looking for. Thanks
Use path.delimiter instead of explicit conditional. Try to reuse repeated code blocks. Fixes #656
59704cf to
a4717e0
Compare
|
@freitagbr PTAL |
|
Ok, this is much better. LGTM. |
Use path.delimiter instead of explicit conditional. Try to reuse
repeated code blocks. Remove logical negation at the end
of the function to improve readability.
Blocked on #655
Fixes #656