Allow the user to choose between cyclomatic and cognitive complexity in ComplexMethod rule

[arturbosch]

Expected Behavior

The cyclomatic complexity calculates how many paths there are in a function.
It is a measurement for testability as the number of paths tells us how many test cases we need to write.

For example a class with 10x mcabe=1 functions would accumulate to complexity 10 and a class with just one function which is nested a lot could still accumulate to complexity 10.
Most users will say that both classes are not equal complex but the deeper nested one is.

Here comes the cognitive complexity into play. It takes nesting into count and would calculate a much higher value for the one function with deep nesting.

detekt/detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt

Lines 28 to 36 in 31520da

	/**
	* Kotlin implementation of the cognitive complexity metric.
	*
	* Please see the "Cognitive Complexity: A new way of measuring understandability" white paper
	* by G. Ann Campbell of SonarSource for further detail.
	*
	* https://www.sonarsource.com/docs/CognitiveComplexity.pdf
	*/
	class CognitiveComplexity private constructor() : DetektVisitor() {

To not break users baselines and CI's, we could let the user choose the complexity metric to calculate:

  ComplexMethod:
    active: true
    threshold: 15
    metric: "cognitive-complexity" # cyclomatic-complexity

Current Behavior

Cyclomatic complexity is used to calculate function complexity.

Context

Cyclomatic complexity does not really measure the complexity but the testability of a function.
Cognitive complexity was described in https://www.sonarsource.com/docs/CognitiveComplexity.pdf to take function nesting into count.

[cortinico]

Allow the user to choose between cyclomatic and cognitive complexity in ComplexMethod rule

Shouldn't this be another rule instead if the user whish to have both reported?

[sanggggg]

If no one is working on this issue, can I work on this issue?

I'm going to rename ComplexMethod rule to CyclomaticComplexMethod and add rule CognitiveComplexMethod.
If you think that changing the existing rule name is not good for detekt, I'll just add CognitiveComplexMethod without renaming ComplexMethod

[cortinico]

If you think that changing the existing rule name is not good for detekt, I'll just add CognitiveComplexMethod without renaming ComplexMethod

I believe that in this case is fine to rename the rule as long as:

• We provide an alias for it
• We deprecate the properties and the config keys

I've assigned this issue to you @sanggggg Let me know if you need further support 👍

[sanggggg]

I've made draft PR #5442, but there's a point I want to ask for help.

IMHO, It is awkward that implementation details such as Cyclomatic and Cognitive are exposed to the rule's name.

• I thought exposing the complex implementation of the two rules to the rule name seemed to inform the user of little unnecessary details.
• The reason for making the two rules is for the person who wishes to use the two complexities together, but I'm not sure if there are many people.
  • I thought Cognitive Complexity and Cyclomatic Complexity had almost the same goal as "testability and maintenance".
• Considering the above, is it still okay to divide to two rules?

And there are also minor questions encountered during the implementation.

1. If the two rules (CognitiveComplexMethod, CyclomaticComplexMethod) are maintained together, should I consider the default detekt config (or detekt repo's config) as both active?

2. I think a lot of parts have already been tested through the test case of CognitiveComplexitySpec.kt, so is it okay to write a simple test for CognitiveComplexMethodSpec, which is just a wrapper?

Sorry for the passive questions, I haven't written many good test codes or opensource contributions yet, so I'd like to get some help.

[BraisGabin]

I think that Cognitive complexity and Cyclomatic complexity are not implementation details. They are two different concepts and it have sense to have them upfront. So I'm ok having those names for rules.

1. If the two rules (CognitiveComplexMethod, CyclomaticComplexMethod) are maintained together, should I consider the default detekt config (or `detekt repo`'s config) as both active?

Maybe I'm wrong here but we just provide CyclomaticComplexMethod. So I would keep that enable and disable CognitiveComplexMethod because it wasn't ever enabled. But I'm open to discurssion here because, in general, detekt focus is on the cognitive issues of the code. I wouldn't enable both by default.

2. I think a lot of parts have already been tested through the test case of `CognitiveComplexitySpec.kt`, so is it okay to write a simple test for `CognitiveComplexMethodSpec`, which is just a wrapper?

Sure, there is no need to test what CognitiveComplexity does. But I would recommend to add a positive and a negative test. I mean, a test that finds an issue and a test with a method that doesn't raises any issue.

Sorry for the passive questions, I haven't written many good test codes or opensource contributions yet, so I'd like to get some help.

Don't be sorry. All these questions have a lot of sense and we are more than happy to help.

[sanggggg]

thank you for your kind response, @BraisGabin

But I'm open to discurssion here because, in general, detekt focus is on the cognitive issues of the code.

I also think it's a good idea to disable CognitiveComplexMethod to maintain its existing functionality. (Although it is a little more cognitive)