Rule proposal: `try-maximum-statements`

Question

Rule proposal: `try-maximum-statements`

Opened this issue 4 years ago · 29 comments

The rule name may be called unicorn/try-maximum-statements.

The idea is to avoid try on whole code blocks and encourage the developer to target the exact ~~line~~ statement of code that is expecting to throw.

Should we add a parameter to customize the maximum number of ~~line~~ statement or hardcode it to 1 line of code? 🤔

Note that I'm not a native english speaker so please fix the rule/option names.

Enforce a maximum number of lines statements to try

Applies to try clauses.

The maximum number of triable ~~lines~~ statements of code configurable, but defaults to 1.

This rule is not fixable.

Fail

try {
  doSomething();
  doSomethingElse();
} catch (err) {
  // …
}

Pass

try {
  doSingleThing();
} catch (err) {
  // …
}

Options

`maximumNumberOfStatement`

Type: number

You can set the maximum number of statements like this:

"unicorn/try-maximum-statement": [
	"error",
	{
		"maximumNumberOfStatement": 2
	}
]

Answer 1 · 2020-03-31T10:49:19.000Z

Should this pass? 🤔

try {
  return doSomething() || doSomethingElse();
} catch (err) {
  // …
}

This may pass. I think we can limit this rule to count the number of lines, not the number of instructions.

If we don't want this to pass we may rename the rule to unicorn/try-maximum-instructions.

Answer 2 · 2020-04-03T14:21:06.000Z

I don't like this but I think it can help. I usually stick a lot of code inside try just because I don't want to use let:

try {
	const a = maybethrows();
	twistIt(a);
	bopIt(a);
	pullIt(a);
} catch {
	// handle
}

The right way would require me to use let

let a;
try {
	a = maybethrows();
} catch {
	// handle
}

twistIt(a);
bopIt(a);
pullIt(a);

Answer 3 · 2020-04-03T16:13:10.000Z

Yep the purpose of that rule is to avoid having bunch of code inside a try 🤷‍♂

I don't see the problem with let 🤔

Answer 4 · 2020-04-03T16:40:37.000Z

I don't see the problem with let 🤔

const is preferred because it forces you to double-check any changes you make to the variable.
Also const is one line while the alternative is 2:

let a;
try {
	a = maybethrows();

If const allowed this it'd already solve the first issue:

const a; // Currently an error
try {
	a = maybethrows();

Answer 5 · 2020-04-03T17:24:45.000Z

I'm lukewarm on this. I fear it will be more annoying than useful.

For example, here, I don't want to expose an intermediate variable outside the try scope.

let foo
try {
  const bar = getBar();
  foo = bar.slice(1, 2);
} catch (err) {
  // …
}

console.log(foo);

Maybe it should instead enforce only one function call inside "try"?

Answer 6 · 2020-04-03T17:29:20.000Z

Maybe it should instead enforce only one function call inside "try"?

.slice is also a function.

Maybe it would just need to be sensible (i.e. no more than 3/4 lines) so it's not too annoying

Answer 7 · 2020-04-03T17:40:11.000Z

That's the problem though. What is a sensible line limit? There's no universal answer. I used to have the built-in ESLint line limit rules enabled, but ended up having to turn them off as measuring things by line numbers is too limiting and annoying.

Answer 8 · 2020-04-03T18:15:40.000Z

@sindresorhus

For example, here, I don't want to expose an intermediate variable outside the try scope.

But why? 🤔

This rule is for people caring about well structured code and then supposed to be split into small functions?

Also your example could be written on one line with no readability loss

let foo
try {
 foo = getBar().slice(1, 2);
} catch (err) {
  // …
}

console.log(foo);

The whole concept of this rule is: Only do a "single" thing in your try

Maybe the code sample is too easy? Can you provide us with a "real" code sample that justify multiple lines? (personally I never had to use multiple lines in my try).

Also in a perfect world it would be better not to limit the lines of codes, but the number of "instructions". But I'm not sur to be able to define a JavaScript "instruction" with my limited knowledge

Answer 9 · 2020-04-04T09:14:51.000Z

The example was obviously artificial just to show that sometimes it makes sense to use multiple lines.

Here are some examples I grepped from my projects:

try {
	fs.statSync('/.dockerenv');
	return true;
} catch (_) {
	return false;
}

test('non-integer count should throw exception', async t => {
	try {
		await beeper(1.5);
		t.fail();
	} catch (error) {
		t.pass();
	}
});

try {
	const value = await Promise.all([total, element.value]);
	next(reducer(value[0], value[1], index++));
} catch (error) {
	reject(error);
}

try {
	const stats = await promisify(fs[fsStatType])(filePath);
	return stats[statsMethodName]();
} catch (error) {
	if (error.code === 'ENOENT') {
		return false;
	}

	throw error;
}

try {
	await subprocess;
	callback();
} catch (_) {
	callback(new PluginError('gulp-ava', 'One or more tests failed'));
}

try {
	for (const handler of this._cancelHandlers) {
		handler();
	}
} catch (error) {
	this._reject(error);
}

try {
	const {stdout} = await execa(shell || defaultShell, args);
	return parseEnv(stdout);
} catch (error) {
	if (shell) {
		throw error;
	} else {
		return process.env;
	}
}

const handleKill = async input => {
	try {
		input = await parseInput(input);

		if (input === process.pid) {
			return;
		}

		if (input === 'node') {
			const processes = await psList();
			await Promise.all(processes.map(async ps => {
				if (ps.name === 'node' && ps.pid !== process.pid) {
					await kill(ps.pid, options);
				}
			}));
			return;
		}

		await kill(input, options);
	} catch (error) {
		if (!exists.get(input)) {
			errors.push(`Killing process ${input} failed: Process doesn't exist`);
			return;
		}

		errors.push(`Killing process ${input} failed: ${error.message.replace(/.*\n/, '').replace(/kill: \d+: /, '').trim()}`);
	}
};

Answer 10 · 2020-04-04T09:15:04.000Z

How would it handle nested try/catch?

Answer 11 · 2020-04-04T09:51:18.000Z

The example was obviously artificial just to show that sometimes it makes sense to use multiple lines.

Here are some examples I grepped from my projects:

try {
	fs.statSync('/.dockerenv');
	return true;
} catch (_) {
	return false;
}

try {
	fs.statSync('/.dockerenv');
} catch (_) {
	return false;
}
return true;

test('non-integer count should throw exception', async t => {
	try {
		await beeper(1.5);
		t.fail();
	} catch (error) {
		t.pass();
	}
});

If t.fail() it self throws, you don't want to catch it and make the test to pass.

You may use expect().toThrow() test assertion (Jest as an example). I'm pretty sure you testing framework as a proper solution to assert errors are thrown.

test('non-integer count should throw exception', async () => {
    await expect(
	  async () => beeper(1.5)
    ).rejects.toThrow();
});

try {
	const value = await Promise.all([total, element.value]);
	next(reducer(value[0], value[1], index++));
} catch (error) {
	reject(error);
}

let value;
try {
	value = await Promise.all([total, element.value]);
} catch (error) {
	reject(error);
    return;
}
next(reducer(value[0], value[1], index++));

try {
	const stats = await promisify(fs[fsStatType])(filePath);
	return stats[statsMethodName]();
} catch (error) {
	if (error.code === 'ENOENT') {
		return false;
	}

	throw error;
}

Not sure what exact instruction you actually are trying to try (that's the whole point of the rule). But let's say:

// I assume we don't want to catch this
const statFunc = promisify(fs[fsStatType]);

let stats;
try {
	stats = await statFunc(filePath);
} catch (error) {
	if (error.code === 'ENOENT') {
		return false;
	}

	throw error;
}
return stats[statsMethodName]();

try {
	await subprocess;
	callback();
} catch (_) {
	callback(new PluginError('gulp-ava', 'One or more tests failed'));
}

try {
	await subprocess;
} catch (_) {
	callback(new PluginError('gulp-ava', 'One or more tests failed'));
    return;
}
callback();

try {
	for (const handler of this._cancelHandlers) {
		handler();
	}
} catch (error) {
	this._reject(error);
}

for (const handler of this._cancelHandlers) {
   try {
    	handler();
   } catch (error) {
		this._reject(error);
		return;
  }
}

Or you may wrap your loop in a dedicated function

try {
	const {stdout} = await execa(shell || defaultShell, args);
	return parseEnv(stdout);
} catch (error) {
	if (shell) {
		throw error;
	} else {
		return process.env;
	}
}

I assume you don't want to catch parseEnv 🤷‍♂ (another advantage of this rule is to spot exactly what is expected to throw).

let stdout;
try {
	stdout = (await execa(shell || defaultShell, args)).stdout;
} catch (error) {
	if (shell) {
		throw error;
	} else {
		return process.env;
	}
}
return parseEnv(stdout);

const handleKill = async input => {
	try {
		input = await parseInput(input);

		if (input === process.pid) {
			return;
		}

		if (input === 'node') {
			const processes = await psList();
			await Promise.all(processes.map(async ps => {
				if (ps.name === 'node' && ps.pid !== process.pid) {
					await kill(ps.pid, options);
				}
			}));
			return;
		}

		await kill(input, options);
	} catch (error) {
		if (!exists.get(input)) {
			errors.push(`Killing process ${input} failed: Process doesn't exist`);
			return;
		}

		errors.push(`Killing process ${input} failed: ${error.message.replace(/.*\n/, '').replace(/kill: \d+: /, '').trim()}`);
	}
};

I'm pretty sur this may be refactored with function that wraps to cod that is in try

Answer 12 · 2020-04-04T09:52:04.000Z

How would it handle nested try/catch?

The same way.. only n lines of code allowed in try

So basically no nested try if only 1 line of code allowed (I can't figure out a use case that needs to nest try in a single function)

Answer 13 · 2020-04-25T16:48:43.000Z

try {
	fs.statSync('/.dockerenv');
} catch (_) {
	return false;
}
return true;

While this works, I still prefer early return. As I do with if statements. What if there was more logic after the try-catch. That would make it more difficult to handle.

for (const handler of this._cancelHandlers) {
   try {
    	handler();
   } catch (error) {
		this._reject(error);
		return;
  }
}

Try-catch comes with certain overhead, so it would be slower to put it in the loop.

Answer 14 · 2020-04-25T16:52:59.000Z

@yvele Instead of lines, how about we limit it to one statement? And maybe also an option to allow a return statement at the end in addition.

I still feel like "number of lines" is too naive.

For example, the below is still one statement:

try {
	result = foo
		.someMethodChainMethod1()
		.someMethodChainMethod2()
		.someMethodChainMethod3()
} catch {}

Answer 15 · 2020-04-30T13:36:02.000Z

@yvele Instead of lines, how about we limit it to one statement?

...

I still feel like "number of lines" is too naive.

For example, the below is still one statement:
try {
	result = foo
		.someMethodChainMethod1()
		.someMethodChainMethod2()
		.someMethodChainMethod3()
} catch {}

Yep I initially suggested a limit to one instruction, but one statement is ok

And maybe also an option to allow a return statement at the end in addition.

Why not. I personally will not use it.

Try-catch comes with certain overhead, so it would be slower to put it in the loop.

I know that catching error comes with lots of overhead (building the call stack, etc.) but I wasn't aware that simply using try also comes with overhead even when no error is thrown. 🤔

Do you have any documentation to share about that try overhead? I tried Googling https://www.google.com/search?q=javascript+try+overhead but nothing relevant.

Answer 16 · 2020-05-01T06:24:55.000Z

Do you have any documentation to share about that try overhead? I tried Googling google.com/search?q=javascript+try+overhead but nothing relevant.

🤷‍♂️ Just from memory. Might not even be valid anymore.

Answer 17 · 2020-05-01T06:25:03.000Z

This is now accepted. PR welcome.

Answer 18 · 2020-05-01T06:25:32.000Z

Name: try-maximum-statements

Answer 19 · 2020-05-01T07:23:03.000Z

I like this, should be easy to implement.

Answer 20 · 2020-05-02T17:07:47.000Z

@yvele You suggest

let foo
try {
  const bar = getBar();
  foo = bar.slice(1, 2);
} catch (err) {
  // …
}

console.log(foo);

Into:

let foo
try {
 foo = getBar().slice(1, 2);
} catch (err) {
  // …
}

console.log(foo);

But if it's

let foo
try {
  const bar = getBar();
  foo = bar.slice(0, Math.floor(bar.length / 2));
} catch (err) {
  // …
}

console.log(foo);

Answer 21 · 2020-05-02T20:58:13.000Z

The fixes I've suggested are hypothetical.

If there is multiple statements in the try we cannot determine which exact statement is expected to throw. It's also possible (even unlikely) that multiple statements are expected to throw (in which case statements must be split into dedicated try).

This rule cannot be fixable.

Does that make sense?

Answer 22 · 2020-06-03T03:41:36.000Z

Another possibility is that developers won't target the right statement (if there is one) and instead group statements into a single one:

const doTwoThings = () => {
  doSomething();
  doSomethingElse();
};

try {
  doTwoThings();
}

I kinda like this fact except no rule can ensure that the new function has a meaningful name (and that it can have one, given that it was created to dodge a syntactic rule).

This also shows that the number of statements does not mean much really.

What about this code:

try {
	const result = await getResult();
	return result; // Could be `subscriber.next(result);`, etc.
}

Do I have to create an outer mutable variable and assign to it in try?

let result;

try {
	result = await getResult();
} catch (...) {...}

return result;

Should I check if result is not undefined before returning? What if getResult returned undefined? Should I add another flag variable just to get the same result as above?

I much prefer the original code.

I hate when there is redundant code in a try block, but I don't think this rule will help with it enough to bear it's annoyance.

Maybe this rule is useful in some projects (I imagine), but not generally (at least coming from my experience).

Answer 23 · 2020-06-03T08:09:03.000Z

What about this code:

try {
	const result = await getResult();
	return result; // Could be `subscriber.next(result);`, etc.
}

try {
  return await getResult();
}

🤷‍♂️

Do I have to create an outer mutable variable and assign to it in try?

Yep I think so

Maybe this rule is useful in some projects (I imagine), but not generally (at least coming from my experience).

I've seen too many developers having a large block of code in try and that's a very good practice to encourage developers to exactly try what is needed. This improve code readability, maintenance, etc.

In my opinion those advantages outweigh the rare cases where "outer mutable variable" are needed.

Answer 24 · 2020-06-04T02:58:22.000Z

I also like this rule, however I think allowing an extra return statement that can be statically guaranteed to not throw (such as return true) is a must.

Answer 25 · 2020-06-04T20:15:07.000Z

I also like this rule, however I think allowing an extra return statement that can be statically guaranteed to not throw (such as return true) is a must.

Yes maybe we can add an option not to count return as a statement 🤔 whether it may throw or not.

Answer 26 · 2021-06-03T06:09:09.000Z

Let's use the same algorithm from complexity rule, and name it try-complexity?

Answer 27 · 2021-06-03T09:48:35.000Z

@fisker Sounds good. Let's start with that and we can potentially iterate based on real world experience with it.

Answer 28 · 2021-06-06T02:23:20.000Z

@fisker @sindresorhus What are your opinions on allowing an extra return someValue; apart from the single statement?

Answer 29 · 2021-06-06T04:26:06.000Z

Hard to tell before check real world cases.