Rule proposal: Use `Array#includes()` instead of repeated conditional checks

[jamesgeorge007]

Say that we've to show up help information when both -h and --help flags are fired in.

The Approach

const [, , ...args] = process.argv;
if (args[0] === '-h' || args[0] === '--help') {
  outputHelp();
} 

What Intended

const [, , ...args] = process.argv;
if (['-h', '--help'].includes(args[0])) {
  outputHelp();
}

[sindresorhus]

if (args[0] === '-h' || '--help') {

I don't think I've ever seen this done before.

[sindresorhus]

I think this could be part of the prefer-includes rule.

[jamesgeorge007]

if (args[0] === '-h' || '--help') {

I don't think I've ever seen this done before.

Oops! that was a typo.

[sindresorhus]

Let's first see if the other maintainers agree on this change.

[futpib]

I think we could report expressions like x === a || x === b || ... when there is more than 3 (or a configurable number) of comparisons to suggest that the list of options has likely grown enough to deserve it's own constant definition.

const options = new Set([a, b, ...]);

options.has(x);

But we should not fix this into [a, b, ...].includes(x) (or into anything else) unconditionally because IMO the best fix depends on complex factors such as:

• Overall complexity/readability of the expression.
• Probability of future edits to this peace of code or reuse of this collection of values. This mostly influences whether I'd inline the options into the expression or extract them into a constant. For instance, I would happily inline a condition like direction === 'up' || direction === 'down' as these two options are probably there forever. I would not inline word === 'screw' || word === 'suka' as this clearly looks like an ever-expanding set of options. This is why I'm fine with args[0] === '-h' || args[0] === '--help' (except one should use a CLI arguments parser library).
• Whether the order of elements is important (Array#includes vs Set#has).

[MrHen]

Personally, I've never been a big fan of using includes in this manner for JavaScript because it causes the code to read "backwards". I want to see the variable on the left and the compared value on the right.

That being said, I think this is a valid stylistic check. Super long conditionals are irritating and whenever there are two ways it smells like a valid lint rule.

I agree with @futpib, though, that we should not auto-fix them.

[voxpelli]

I don't think should be a rule that's active by default, especially not if applying even to 2 values.

I would also push for making it possible to configure the threshold to something higher than 2, so one could eg. require this just for checks against at least 3 or 4 values

[jamesgeorge007]

I would love to submit a PR :+1: @sindresorhus it would be great if you could guide me through as I'm new to the codebase :clap:

[fisker]

@jamesgeorge007 https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/new-rule.md#creating-a-new-rule

[himynameisdave]

FWIW I just wanted to add a minor point here: The performance of the .includes version is always going to be slightly worse than a string of conditional checks (presumably because of the array initialization). I still think we should add this as an option to the prefer-includes rule, but maybe the default should be for it to not be on?

Here's a simple benchmark if you're interested in playing around with it.

[fisker]

This function flatLogicalExpression and getCommonReferences can be useful for this rule.

[dimaMachina]

I thought also about this rule, every time make refactoring like this