Charles Clayton
Charles Clayton

Reputation: 17956

Why is the cylcomatic complexity of this function 12?

I have a (C#) function that checks four sets of conditions and returns a bool. If any of them are true, it returns true. I'm sure I could simplify the logic but I want it to be fairly readable.

The CodeMaid extension in Visual Studios and tells me the cylomatic complexity of the function is 12. I looked it up and the cylomatic complexity is

the number of independent paths through the source code

I don't understand why it's 12. I can think of it two ways, either the cyclomatic complexity should be 2, because it always goes through the same path but could return either a true or a false. Or could understand if it was 16, because the four booleans or'd together at the end could each be true or false, 2*2*2*2 = 16.

Can someone tell me why its 12? Maybe even show a diagram so I can visualize the different paths?

public bool FitsCheckBoxCriteria(TaskClass tasks)
{
    // note: bool == true/false comparisons mean you don't have to cast 'bool?' as bool


    // if neither checkboxes are checked, show everything
    bool showEverything = NoShutDownRequiredCheckBox.IsChecked == false &&
                          ActiveRequiredCheckBox.IsChecked == false; 

    // if both are checked, only show active non-shutdown tasks
    bool showActiveNonShutdown = ActiveRequiredCheckBox.IsChecked == true &&
                                 tasks.Active == "YES" &&
                                 NoShutDownRequiredCheckBox.IsChecked == true &&
                                 tasks.ShutdownRequired == "NO";

    // if active is checked but shudown isn't, display all active
    bool showActive = ActiveRequiredCheckBox.IsChecked == true &&
                      tasks.Active == "YES" &&
                      NoShutDownRequiredCheckBox.IsChecked == false;

    // if non-shutdown is checked but active isn't, display all non-shutdown tasks
    bool showNonShutdown = NoShutDownRequiredCheckBox.IsChecked == true &&
                           tasks.ShutdownRequired == "NO" &&
                           ActiveRequiredCheckBox.IsChecked == false;

    return showEverything || showActiveNonShutdown || showActive || showNonShutdown;
}

Thanks in advance.

Edit:

I changed it to this. assigning local variables for the checkbox conditions didn't have any effect, but creating booleans out of the "YES"/"NO" cranked up the complexity to 14, which I think I understand.

public bool FitsCheckBoxCriteria(LubeTask tasks)
{
    bool noShutdownReqChecked = (bool)NoShutDownRequiredCheckBox.IsChecked;
    bool activeChecked = (bool)ActiveRequiredCheckBox.IsChecked;

    bool active = tasks.Active == "YES" ? true : false;
    bool shutdownReq = tasks.ShutdownRequired == "YES" ? true : false;

    // if neither checkboxes are checked, show everything
    bool showEverything = !noShutdownReqChecked && !activeChecked;

    // if both are checked, only show activeChecked non-shutdown tasks
    bool showActiveNonShutdown = activeChecked && noShutdownReqChecked && active && !shutdownReq;

    // if activeChecked is checked but shudown isn't, display all activeChecked
    bool showActive = activeChecked && !noShutdownReqChecked && active;

    // if non-shutdown is chceked but activeChecked isn't, display all non-shutdown tasks
    bool showNonShutdown = noShutdownReqChecked && !activeChecked && !shutdownReq;

    return showEverything || showActiveNonShutdown || showActive || showNonShutdown;
}

Upvotes: 5

Views: 631

Answers (3)

perfectionist
perfectionist

Reputation: 4346

The key is in "independent paths".

I'm going to rewrite your code to shorten it so we can discuss it.

public bool FitsCheckBoxCriteria(TaskClass tasks)
{
    bool E1 = A1 && A2; 

    bool E2 = B1 && B2 && B3 && B4;

    bool E3 = C1 && C2 && C3;

    bool E4 = D1 && D2 && D3;

    return E1 || E2 || E3 || E4;
}

The Cyclomatic complexity is the number of independent paths. This is not the total possible number of return values (2).

The && operator and the || operator are short circuit operations; if A1 is false, A2 is not evaluated. Similarly, if E1 is true, E2 is not evaluated.

If you replace all the &&s with & and all the ||s with | in the code above, the cyclomatic complexity is 1, because there is only one path through the code. (This wouldn't make it better code though).

As it is, there are 72 possible paths...

  1. A1, B1, C1, D1, E1 are evaluated; the others are not.
  2. A1, A2, B1, C1, D1, E1 are evaluated; the others are not.
  3. A1, B1, B2, C1, D1, E1 are evaluated; the others are not.
  4. A1, A2, B1, B2, C1, D1, E1 are evaluated; the others are not. etc...

But path 4 doesn't contain any new code that isn't already on the previous paths. And this is the definition of "independent paths" - each path must include new code.

So in this example, you can count the code by hand as follows:

1 + the number of short circuit operators in this code (11) = 12.

Wikipedia has an excellent in depth explanation.

Upvotes: 3

user3517045
user3517045

Reputation:

C# uses short-circuit evaluation which means that if there is an x && y expression y is evaluated only if it is necessary, more precisely, if x is true. This means that result = x && y; has two independent execution paths: (1) if x is false then only x is evaluated and result gets false value (without evaluating y) but (2) if x is true y is evaluated as well and result gets the evaluation result of y. This means that every && and || operator increases the cyclomatic complexity and in the first example there are 8 && and 3 || operators so the cyclomatic complexity of the method is 12.

Upvotes: 2

doogle
doogle

Reputation: 3406

This is just a guess, but I think the assignments are +2 each (if =true/else =false) and then +1 for each possible exit condition in the return statement. So it might unwind into something like:

bool showEverything = false;
if (...) { showEverything = true; } +1
else { showEverything = false; } +1

bool showActiveNonShutdown = ... +2 if/else
bool showActive = ... +2 if/else
bool showNonShutdown = ... +2 if/else

if (showEverything) {...} +1
else if (showActiveNonShutdown) {...} +1
else if (showActive) {...} +1
else if (showNonShutdown) {...} +1
else {false}

Upvotes: 3

Related Questions