Issue 1434 - Remove some non-standard JS usage from buildtools.

Issue 1434 -  Remove some non-standard JS usage from buildtools.

[tschuster, 2014-11-16 11:51:32]

On 2014/09/24 13:31:18, tschuster wrote:
I guess I never requested a review here, just found this patch in my queue. :)

[Wladimir Palant, 2014-11-17 20:05:17]

I'm not really sure whether I consider your changes beyond replacing for each
(.. in ..) by for (.. of ..) necessary. While storing intermediate values might
make the code easier to read, here you change variable type from string to array
- not sure whether this is a good idea.

http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
File lib/keySelector.js (right):

http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
lib/keySelector.js:97: modifiers = modifiers ?
modifiers.toUpperCase().match(/\w+/g) : null;
If you are doing it this way, why not use [] instead of null? Then you don't
need the if block below any more.

http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
lib/keySelector.js:119: if (!variants)
This condition can never be true - String.split() always returns an array with
at least one entry.

http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
lib/keySelector.js:139: if (!variant)
Same here, this if block is pointless.

[tschuster, 2014-11-17 20:46:17]

On 2014/11/17 20:05:17, Wladimir Palant wrote:
> I'm not really sure whether I consider your changes beyond replacing for each
> (.. in ..) by for (.. of ..) necessary. While storing intermediate values
might
> make the code easier to read, here you change variable type from string to
array
> - not sure whether this is a good idea.
> 
>
http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
> File lib/keySelector.js (right):
> 
>
http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
> lib/keySelector.js:97: modifiers = modifiers ?
> modifiers.toUpperCase().match(/\w+/g) : null;
> If you are doing it this way, why not use [] instead of null? Then you don't
> need the if block below any more.
> 
But match could still return null.
>
http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
> lib/keySelector.js:119: if (!variants)
> This condition can never be true - String.split() always returns an array with
> at least one entry.
> 
Thanks, I forgot about this.
>
http://codereview.adblockplus.org/5695444839563264/diff/5629499534213120/lib/...
> lib/keySelector.js:139: if (!variant)
> Same here, this if block is pointless.

[Wladimir Palant, 2014-12-09 21:24:49]

http://codereview.adblockplus.org/5695444839563264/diff/5757334940811264/lib/...
File lib/keySelector.js (right):

http://codereview.adblockplus.org/5695444839563264/diff/5757334940811264/lib/...
lib/keySelector.js:97: let modifiers = keyModifiers ?
keyModifiers.toUpperCase().match(/\w+/g) : null;
Comment from previous round not addressed or replied to:

On 2014/11/17 20:05:17, Wladimir Palant wrote:
If you are doing it this way, why not use [] instead of null? Then you don't
need the if block below any more.

[tschuster, 2014-12-13 12:11:58]

On 2014/12/09 21:24:49, Wladimir Palant wrote:
>
http://codereview.adblockplus.org/5695444839563264/diff/5757334940811264/lib/...
> File lib/keySelector.js (right):
> 
>
http://codereview.adblockplus.org/5695444839563264/diff/5757334940811264/lib/...
> lib/keySelector.js:97: let modifiers = keyModifiers ?
> keyModifiers.toUpperCase().match(/\w+/g) : null;
> Comment from previous round not addressed or replied to:
> 
> On 2014/11/17 20:05:17, Wladimir Palant wrote:
> If you are doing it this way, why not use [] instead of null? Then you don't
> need the if block below any more.

keyModifiers.toUpperCase().match(/\w+/g) can return null. Unless of course the
empty string is impossible here.

[Wladimir Palant, 2017-03-15 15:17:58]

I'm closing this review, the issues have been addressed elsewhere since.