Generic/CyclomaticComplexity: potential inconsistencies when counting cyclomatic complexity

[rodrigoprimo]

Is your feature request related to a problem?

While working on #684, I noticed some divergences between how PHPCS and PHPMD/PHP Depend count cyclomatic complexity. For example, PHPCS increases the cyclomatic complexity by one whenever it finds a default statement, while PHPMD/PHP Depend doesn't.

Maybe there needs to be a review of the sniff to ensure that it is correctly counting cyclomatic complexity? This issue is somewhat related to #726, which presents another case that the sniff considers but that potentially it should not.

I'm not sure if PHPCS or PHPMD is correct, and I was not able to find what is the definition of cyclomatic complexity that is used by the sniff to check if it is following it or not.

• I have read the Contribution Guidelines and this is not a support question.
• I intend to create a pull request to implement this feature.

[jrfnl]

@rodrigoprimo Thanks for opening this issue.

For reference - issues regarding previous changes made to the sniff (added tokens), including one which is still open which contains an interesting discussion about the concept: squizlabs/PHP_CodeSniffer#3501

For example, PHPCS increases the cyclomatic complexity by one whenever it finds a default statement

This will need a backtrace to see when the T_DEFAULT token was added and why.

If I look at the explanation regarding this in the PDepends documentation, I agree with you that it should not be included, just like T_ELSE and T_INLINE_ELSE are not included.

If we want to apply this consistently though, it also means that T_MATCH_DEFAULT will need to be excluded, which will add some complexity to the sniff as match structure complexity is counted based on the occurance of T_MATCH_ARROW.

Looking even more closely at the $find list, I also wonder if T_FINALLY may need to be added or if finally blocks should be regarded as the "default" path. I think the latter (= default path, so do not search for it), but would like a second opinion on this.
Digging even deeper into this, it may even depend on whether there is a return statement within the finally block ?
Ref: https://www.php.net/manual/en/language.exceptions.php#language.exceptions.finally

Along similar lines, all T_CASE and T_DEFAULT tokens are counted for a switch(), but I wonder whether they should be or if it would be better to have some logic checking whether there is a body ? Or possibly even to check for continue/break/return in switch statements ?

Consider these three code samples, which are functionally the same, but have quite different cyclomatic complexity:

// Cyclomatic complexity: 12.
function switching() {
    switch ($foo) {
        case 0:
        case 1:
        case 2:
            do_something();
            break;
        case 3:
        case 4:
        case 5:
            do_something();
            break;
        case 6:
        case 7:
        case 8:
            do_something();
            break;
        case 9:
        default:
            do_something();
            break;
    }
}

// Cyclomatic complexity: 6.
function switchTrue() {
    switch (true) {
        case $foo == 0 || $foo == 1 || $foo == 2:
            do_something();
            break;
        case $foo == 3 || $foo == 4 || $foo == 5:
            do_something();
            break;
        case $foo == 6 || $foo == 7 || $foo == 8:
            do_something();
            break;
        case 9:
        default:
            do_something();
            break;
    }
}

// Cyclomatic complexity: 4.
function usingIfs() {
    if ($foo == 0 || $foo == 1 || $foo == 2) {
        do_something();
    } elseif ($foo == 3 || $foo == 4 || $foo == 5) {
        do_something();
    } elseif ($foo == 6 || $foo == 7 || $foo == 8) {
        do_something();
    } else {
        do_something();
    }
}

Maybe there needs to be a review of the sniff to ensure that it is correctly counting cyclomatic complexity?

I fully agree.

[jrfnl]

Maybe @MarkBaker would be interested in having another look at this, seeing as he made most of the more recent changes ?

[rodrigoprimo]

For reference - issues regarding previous changes made to the sniff (added tokens), including one which is still open which contains an interesting discussion about the concept: squizlabs/PHP_CodeSniffer#350

@jrfnl, I believe you meant to link to squizlabs/PHP_CodeSniffer#3501 instead of squizlabs/PHP_CodeSniffer#350

[MarkBaker]

default should not be included in the CYC calculation: like the else path, it's already factored in by the initialisation of the complexity value as 1, but every instance of case should be counted, including those with dropthrough.
A match statement is more complex because every possible case must be covered, so the CYC is the number of expressions less one; but a match expression with multiple criteria like $a, $b, $c => 5 counts as three expressions because the comma acts as a logical or.

Similarly, in the case of compound if/elseif criteria, the number of ored criteria should be counted, so if ($foo == 0 || $foo == 1 || $foo == 2) { should add 3 to the CYC calculation.

I can probably take another look and see how these issues might be resolved.

[MarkBaker]

And with some more thought: a switch with a case that drops straight through to default (as in the examples that jrf has shown) without any intermediate code shouldn't be counted, it's covered by the default path. All three of those examples should have a CYC of 9.