Initial MCC change

[schalkms]

reference https://www.ndepend.com/docs/code-metrics#CC

[arturbosch]

should we move forEach to the scope functions?

[schalkms]

I'm not sure. I think it's also okay to have it in the loops section.

[schalkms]

In my opinion the safe navigation operator ?. and the null coalescing operator ?: also needs to be added. What do you think?

[arturbosch]

I would make this configurable. Most users wont like this change, however per definition this makes sense and they add a bit of clutter.
If you inspect the bytecode there are new paths, but they do not add to mental complexity...

[schalkms]

If you inspect the bytecode there are new paths, but they do not add to mental complexity

I mean it's all about the code paths, right?

[schalkms]

I disagree here. The cyclomatic complexity also determines the number of test cases required for a function. A programmer could trick the analyzer in rewriting the following code. The first example reports 2 the second example 1. Anyways, the programmer needs to write two test cases covering both branches. I know that some users may not like it, but other handle this similarly. We might want to increase the allowed complexity from 10 to 15 for instance.

fun f() {
  if (foo != null) 
    foo.bar()
}

fun f() {
  foo?.bar()
}

[arturbosch]

Ok, what do you think about chained safe navigation foo?.bar?.toString() ?: "bla"?
Would you count all ? or just one because obviously only the non-null and the ?: case are interesting.

[schalkms]

I think I need to do further research in that regard.
I will change the obvious things in this PR (if I have time).

• try/catch count
• if logic (with || and &&) because every other tool does it that way

We mention this in the changelog and we will be fine.
The rest is subject for research and perhaps for another PR.

[amitd291]

IMO a strict check should consider all the branches in chained safe navigation, and not only the first one. For eg: Jacoco considers all branches of chained safe navigation for code coverage.

[schalkms]

@amitdash291 Jacoco works on the lower-level bytecode, whereas detekt uses the PSI.

[schalkms]

I'll proceed with this PR and update the obvious elements (for instance the try/catch blocks).
I'll update the "more controversial" elements in another PR.

[schalkms]

These test cases need to be moved to multiple unit tests.
Furthermore, we should test the McCabeVisitor.kt class intensively and not every usage of it.

[schalkms]

I'd still merge this for the next version, since it fixes the wrong calculation.

[arturbosch]

Sorry, I expect multiple issues to be raised for different releases when we break/change this rule.
I would join forces with you after 1.1 is out to get this in 1.2.

[schalkms]

@arturbosch I see. However, I'd then recommend having more than 1 PR.
The change set would get too large and hard to review.
I like that we bring this into version 1.2.