file-icons / atom

Atom file-specific icons for improved visual grepping.
MIT License
1.32k stars 250 forks source link

Refactor unused variable assignment #827

Closed ericcornelissen closed 4 years ago

ericcornelissen commented 4 years ago

Following up on #823, there is one alert remaining by LGTM for file-icons/atom. This time it's a super simple "problem" so I figured I can open a PR immediately :smile:

The Problem

In the file path-strategy.js there is an assignment to a variable which is not subsequently used. Namely, in the function matchIcon a value is assigned to the local variable icon and this is immediately returned, namely:

https://github.com/file-icons/atom/blob/850b9a24c3b3588c270fcceca9209f8756146b1f/lib/service/strategies/path-strategy.js#L34-L39

Luckily, this works as expected (as demonstrated by this oneliner - console.log(function() { let icon = null; return icon = "foobar"; }()) - which outputs foobar) but it looks a bit weird :unamused: The project history doesn't provide me with any insights into why this is the way it is as it was introduced in an enormous PR a few years back without any comments on this specific line/function/file.

The Solution

The problem can be fixed in two ways. Either the assignment to icon on line 35 is removed (returning its assigned value immediately) or the return is delayed to the end of the function, which requires moving the other return-statement out of the else-branch as well.

I choose the latter because 1) it reduced the number of return-statements in the function, and 2) the formatting of line 35 becomes really ugly without the assignment (perhaps that's why the assignment is there :thinking:).

If the former is preferred I'd be happy to update the PR. If the change is not desired that's fine too (just because LGTM reports it doesn't mean it has to be fixed).

Alhadis commented 4 years ago

the formatting of line 35 becomes really ugly without the assignment (perhaps that's why the assignment is there 🤔).

Heh. The assignment was added to sidestep ASI (as a plain return would simply return undefined). I could have used brackets too, but for some reason, I chose this. 🤷‍♂️

ericcornelissen commented 4 years ago

Heh. The assignment was added to sidestep ASI (as a plain return would simply return undefined). I could have used brackets too, but for some reason, I chose this. man_shrugging

Ah yes, sorry for the confusion. If you write line 35 as shown below the semicolon will be inserted after the return-statement - returning undefined.

return // semicolon will be inserted here
    IconTables.matchPath(resource.path, true) || 
    IconTables.matchName(resource.name, true) || 
    null;

What I was referring to there was formatting line 35 as, which would return correctly.

return IconTables.matchPath(resource.path, true) || 
    IconTables.matchName(resource.name, true) || 
    null;