cyclomatic-complexity: Don't count empty switch cases #2743
Conversation
There was a problem hiding this comment.
I don't really like that rule enabled. It encourages splitting functions just to satisfy the linter.
I'm ok with separation of concerns but there are cases where it just doesn't make sense to split functions.
Take the functions that were disabled in this PR as an example. Most of them just switch on node.kind with many cases. You can't easily extract functionality to another function.
...just my 2c
| @@ -105,6 +105,7 @@ function walk(ctx: Lint.WalkContext<{ threshold: number }>): void { | |||
| function increasesComplexity(node: ts.Node): boolean { | |||
| switch (node.kind) { | |||
| case ts.SyntaxKind.CaseClause: | |||
| return (node as ts.CaseClause).statements.length > 0; | |||
There was a problem hiding this comment.
that's not entirely right. consider the following code which does not increase complexity with this change:
switch (foo) {
case 0:
default:
return bar();
}There was a problem hiding this comment.
There's only one possible path through that code, so I don't think it should increase complexity.
| @@ -105,6 +105,7 @@ function walk(ctx: Lint.WalkContext<{ threshold: number }>): void { | |||
| function increasesComplexity(node: ts.Node): boolean { | |||
| switch (node.kind) { | |||
| case ts.SyntaxKind.CaseClause: | |||
| return (node as ts.CaseClause).statements.length > 0; | |||
There was a problem hiding this comment.
if (foo === 0 || foo === 1) {bar()} increases complexity twice. The will increase complexity only once with this change:
switch (foo) {
case 0:
case 1:
bar();
}There was a problem hiding this comment.
btw. if revert your change here to make them equal again, you probably need to disable the rule in almost all ts.forEachChild callbacks in this repo
There was a problem hiding this comment.
foo === 0 || foo === 1 is semantically equivalent to the switch statement, but syntactically it's treated the same as any other || expression, which can contain arbitrarily complex behavior.
There's some debate about this issue here: http://stackoverflow.com/questions/30240236/cyclomatic-complexity-of-switch-case-statement
In my opinion, treating multiple identical switch cases as each contributing to more complexity just makes this rule unusable, as you just pointed out.
| @@ -105,6 +105,7 @@ function walk(ctx: Lint.WalkContext<{ threshold: number }>): void { | |||
| function increasesComplexity(node: ts.Node): boolean { | |||
| switch (node.kind) { | |||
| case ts.SyntaxKind.CaseClause: | |||
| return (node as ts.CaseClause).statements.length > 0; | |||
There was a problem hiding this comment.
related: why do else branches or default clauses not increase complexity?
There was a problem hiding this comment.
Well, these two code snippets seem equally complex to me:
if (foo) {
return 0;
}
return 1;if (foo) {
return 0;
} else {
return 1;
}
|
I'm not a big fan of cyclomatic complexity either, so I'd be happy to switch this pull request to just the change to the rule, and move it in our |
|
I don't want to enable this rule either. Please move it from "TODO" to "Don't want" instead. |
andy-hanson
changed the title
|
Are there docs on how switch statement cases affect the cyclomatic complexity score? |
PR checklist
Overview of change:
switch { case 0: case 1: case 2: return "a"; default: return "b"; }now has a cyclomatic complexity of 2, not 4.Also enabled the rule. Each of the disables (not marked "fixed in 5.3") should be looked into later.
CHANGELOG.md entry:
[enhancement]
cyclomatic-complexity: Don't count empty switch case