Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule Request: Cognitive Complexity #3335

Open
2 tasks done
Nelyus opened this issue Sep 9, 2020 · 5 comments · May be fixed by #5838
Open
2 tasks done

Rule Request: Cognitive Complexity #3335

Nelyus opened this issue Sep 9, 2020 · 5 comments · May be fixed by #5838
Labels
rule-request Requests for a new rules.

Comments

@Nelyus
Copy link

Nelyus commented Sep 9, 2020

New Issue Checklist

New rule request

  1. Why should this rule be added? Share links to existing discussion about what
    the community thinks about this.

I find myself regularly disabling the "Cyclomatic complexity" rule for method that only contain a switch, and are really easy to understand.

While discussing this with fellow developers, I was told and given a link about Cognitive Complexity, which aims exactly that : replace cyclomatic complexity to provide a metric best suited to evaluate the difficulty to understand a method. It has the added benefit of evaluating types as well as methods.

I don't have a lot of experience with it by lack of current tooling to mesure it, but for my methods that currently trigger cyclomatic complexity, it makes sense.

White paper: Cognitive Complexity

  1. Provide several examples of what would and wouldn't trigger violations.

Only methods having a score higher than the given threshold would trigger a violation.

The first exemple of the paper is the following 2 methods. They have the same cyclomatic complexity (4), but different cognitive complexity.

int sumOfPrimes(int max) {
  int total = 0;
  OUT: for (int i = 1; i <= max; ++i) { // +1
    for (int j = 2; j < i; ++j) {       // +2 (for: +1, nesting: +1)
      if (i % j == 0) {                 // +3 (if: +1, nesting: +2)
        continue OUT;                   // +1 (break in the flow: +1)
      }
    }
    total += i;
  }
  return total;
} // Cognitive complexity 7
String getWords(int number) {
  switch (number) {         // +1
    case 1:
      return "one";
    case 2:
      return "a couple";
    case 3:
      return “a few”;
    default:
      return "lots";
  }
} // Cognitive complexity 1

It tends to favour code structures easier to read. For exemple switch are easier to read than there if {} else if {} counterpart, it has less noise, fewer repetition of the same variables.

  1. Should the rule be configurable, if so what parameters should be configurable?

The rule would have 2 thresholds: the first for warnings, the second for errors.

They should have a default value, and be configurable.

  1. Should the rule be opt-in or enabled by default? Why?

In my opinion it should be enabled by default at the end of the road. I use a linter to keep my code understandable, and cognitive complexity precisely try to answer that. And at first look, it does a decent job at that.

But since it is not widely known and use, it is probably wise to have it opt-in for a while, to evaluate its relevance, and find proper default thresholds.

@jpsim jpsim added the rule-request Requests for a new rules. label Nov 7, 2020
@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

Yes this is great, and I'd go even further to say that a rule like this, if it were reliable, should be enabled by default. SwiftLint hasn't shied away from having opinionated defaults.

@lordzsolt
Copy link
Contributor

@Nelyus FYI cyclomatic_complexity has a configuration ignores_case_statements (which is false by default). If you set that to true, cases will not increase the cyclomatic complexity counter

@jpsim
Copy link
Collaborator

jpsim commented Nov 9, 2020

Ah right, I forgot about that configuration parameter. Thanks @lordzsolt!

@Nelyus
Copy link
Author

Nelyus commented Nov 13, 2020

@Nelyus FYI cyclomatic_complexity has a configuration ignores_case_statements (which is false by default). If you set that to true, cases will not increase the cyclomatic complexity counter

I didn't know that. Thanks for the tip !

@Sophrinix
Copy link

Is anyone working on a PR for this rule? It doesn't appear to have ever been added to SwiftLint.

@lorwe lorwe linked a pull request Oct 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule-request Requests for a new rules.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants