Rule proposal: `try-maximum-statements` #651

yvele · 2020-03-31T10:27:20Z

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
	}
]

The text was updated successfully, but these errors were encountered:

yvele · 2020-03-31T10:49:19Z

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.

fregante · 2020-04-03T14:21:06Z

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);

yvele · 2020-04-03T16:13:10Z

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

I don't see the problem with let 🤔

fregante · 2020-04-03T16:40:37Z

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();

sindresorhus · 2020-04-03T17:24:45Z

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"?

fregante · 2020-04-03T17:29:20Z

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

sindresorhus · 2020-04-03T17:40:11Z

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.

yvele · 2020-04-03T18:15:40Z

@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

sindresorhus · 2020-04-04T09:14:51Z

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()}`);
	}
};

sindresorhus · 2020-04-04T09:15:04Z

How would it handle nested try/catch?

yvele · 2020-04-04T09:51:18Z

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

yvele · 2020-04-04T09:52:04Z

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)

sindresorhus · 2020-04-25T16:48:43Z

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.

sindresorhus · 2020-04-25T16:52:59Z

@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 {}

yvele · 2020-04-30T13:36:02Z

@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.

sindresorhus · 2020-05-01T06:24:55Z

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.

sindresorhus · 2020-05-01T06:25:03Z

This is now accepted. PR welcome.

sindresorhus · 2020-05-01T06:25:32Z

Name: try-maximum-statements

fisker · 2020-05-01T07:23:03Z

I like this, should be easy to implement.

fisker · 2020-05-02T17:07:47Z

@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);

yvele · 2020-05-02T20:58:13Z

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?

futpib · 2020-06-03T03:41:36Z

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).

yvele · 2020-06-03T08:09:03Z

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.

papb · 2020-06-04T02:58:22Z

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.

yvele · 2020-06-04T20:15:07Z

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.

fisker · 2021-06-03T06:09:09Z

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

sindresorhus · 2021-06-03T09:48:35Z

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

papb · 2021-06-06T02:23:20Z

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

fisker · 2021-06-06T04:26:06Z

Hard to tell before check real world cases.

yvele changed the title ~~New rule request: Enforce a maximum number of lines to try~~ Rule proposal: try-maximum-lines Mar 31, 2020

sindresorhus added evaluating new rule labels Apr 3, 2020

sindresorhus removed the evaluating label May 1, 2020

yvele changed the title ~~Rule proposal: try-maximum-lines~~ Rule proposal: try-maximum-statements May 1, 2020

fregante mentioned this issue Nov 17, 2023

Reintroduce "length" rules to catch excess xojs/xo#738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule proposal: `try-maximum-statements` #651

Rule proposal: `try-maximum-statements` #651

yvele commented Mar 31, 2020 •

edited

Loading

yvele commented Mar 31, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

yvele commented Apr 3, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

sindresorhus commented Apr 3, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

sindresorhus commented Apr 3, 2020

yvele commented Apr 3, 2020 •

edited

Loading

sindresorhus commented Apr 4, 2020

sindresorhus commented Apr 4, 2020

yvele commented Apr 4, 2020 •

edited

Loading

yvele commented Apr 4, 2020 •

edited

Loading

sindresorhus commented Apr 25, 2020

sindresorhus commented Apr 25, 2020

yvele commented Apr 30, 2020

sindresorhus commented May 1, 2020

sindresorhus commented May 1, 2020

sindresorhus commented May 1, 2020

fisker commented May 1, 2020

fisker commented May 2, 2020

yvele commented May 2, 2020

futpib commented Jun 3, 2020 •

edited

Loading

yvele commented Jun 3, 2020 •

edited

Loading

papb commented Jun 4, 2020

yvele commented Jun 4, 2020

fisker commented Jun 3, 2021 •

edited

Loading

sindresorhus commented Jun 3, 2021

papb commented Jun 6, 2021

fisker commented Jun 6, 2021

Rule proposal: try-maximum-statements #651

Rule proposal: try-maximum-statements #651

Comments

yvele commented Mar 31, 2020 • edited Loading

Enforce a maximum number of lines statements to try

Fail

Pass

Options

maximumNumberOfStatement

yvele commented Mar 31, 2020 • edited Loading

fregante commented Apr 3, 2020 • edited Loading

yvele commented Apr 3, 2020 • edited Loading

fregante commented Apr 3, 2020 • edited Loading

sindresorhus commented Apr 3, 2020 • edited Loading

fregante commented Apr 3, 2020 • edited Loading

sindresorhus commented Apr 3, 2020

yvele commented Apr 3, 2020 • edited Loading

sindresorhus commented Apr 4, 2020

sindresorhus commented Apr 4, 2020

yvele commented Apr 4, 2020 • edited Loading

yvele commented Apr 4, 2020 • edited Loading

sindresorhus commented Apr 25, 2020

sindresorhus commented Apr 25, 2020

yvele commented Apr 30, 2020

sindresorhus commented May 1, 2020

sindresorhus commented May 1, 2020

sindresorhus commented May 1, 2020

fisker commented May 1, 2020

fisker commented May 2, 2020

yvele commented May 2, 2020

futpib commented Jun 3, 2020 • edited Loading

yvele commented Jun 3, 2020 • edited Loading

papb commented Jun 4, 2020

yvele commented Jun 4, 2020

fisker commented Jun 3, 2021 • edited Loading

sindresorhus commented Jun 3, 2021

papb commented Jun 6, 2021

fisker commented Jun 6, 2021

Rule proposal: `try-maximum-statements` #651

Rule proposal: `try-maximum-statements` #651

yvele commented Mar 31, 2020 •

edited

Loading

`maximumNumberOfStatement`

yvele commented Mar 31, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

yvele commented Apr 3, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

sindresorhus commented Apr 3, 2020 •

edited

Loading

fregante commented Apr 3, 2020 •

edited

Loading

yvele commented Apr 3, 2020 •

edited

Loading

yvele commented Apr 4, 2020 •

edited

Loading

yvele commented Apr 4, 2020 •

edited

Loading

futpib commented Jun 3, 2020 •

edited

Loading

yvele commented Jun 3, 2020 •

edited

Loading

fisker commented Jun 3, 2021 •

edited

Loading