How to reduce cyclomatic complexity on this c# method?

Question

I'm currently doing a project for my c# classes. Our teacher gave us some code metrics limits that we have to abide to and one of them is cyclomatic complexity. Right now he complexity of the method below is 5, but it needs to be 4. Is there any way to improve that?

MethodI was talking about:

private bool MethodName()
    {
        int counter = 0;

        for (int k = 0; k < 8; k++)
        {
            for (int j = 0; j < 3; j++)
            {
                if (class1.GetBoard()[array1[k, j, 0], array1[k, j, 1]] == player.WhichPlayer()) counter++;
            }

            if (counter == 3) return true;
            else counter = 0;
        }
        return false;
    }

You can use the int counter = 0; inside the first for cycle. Then, you can remove the `else counter = 0;` — Emanuele, Jun 07 '21 at 14:25
Write some unintelligible linq! (That *actually* may be the thing to do here.) — Peter - Reinstate Monica, Jun 07 '21 at 14:31
More seriously, the standard way to solve this (unless you can simplify the code as such) is to turf some blocks (like the inner for loop) to separate functions. — Peter - Reinstate Monica, Jun 07 '21 at 14:33
No idea about that metric, but it looks like `class1.GetBoard()` does not depend on both `j` and `k`, so definitely I would call it just once(!) **outside** the for loops, instead of calling it `8*3` times **inside** the for loops. Same goes for `player.WhichPlayer()`. — Peter B, Jun 07 '21 at 14:36
One micro-optimization could be based on the fact that `counter` is actually only used locally in the outer loop; move the definition and initialization to its top, thus obviating the else branch. — Peter - Reinstate Monica, Jun 07 '21 at 14:36
Can you please explain what does 'to turf some blocks mean'? — mandarancza, Jun 07 '21 at 14:39

Boris Sokolov · Accepted Answer · 2021-06-07T15:33:01.087

2

I can wrap the conditions to reduce it. For example

private bool MethodName()
    {
        for (int k = 0; k < 8; k++)
        {
            bool b = true;
            for (int j = 0; j < 3; j++)
            {
                b &= class1.GetBoard()[array1[k, j, 0], array1[k, j, 1]] == player.WhichPlayer();
            }

            if (b) return true;
        }
        return false;
    }

edited Jun 07 '21 at 15:33

answered Jun 07 '21 at 14:26

Boris Sokolov

1,723
4
23
38

1

Converting a bool to int to reduce cyclomatic complexity... It might do, but makes the code much. less clear. I wouldn't do that unless I had a very strong reason. – Rodrigo Rodrigues Jun 07 '21 at 14:29
I mean, technically, You also could remove the `if (counter == 3) return true` condition and put a `goto` statement... would you? – Rodrigo Rodrigues Jun 07 '21 at 14:32
Thank you very much. That seems to do the trick, wouldn't think about this method. Also as the above comment suggests to use goto, we cannot really do that on these classes. – mandarancza Jun 07 '21 at 14:38
1

@RodrigoRodrigues, didn't have time for clean code. There is no need to Convert. The cyclomatic complexity is rarely used in real life projects anyway – Boris Sokolov Jun 07 '21 at 15:35
@mandarancza, please check a bit better version with bool instead of counter. – Boris Sokolov Jun 07 '21 at 15:38
@BorisSokolov can you please explain what does &= do in these case? – mandarancza Jun 07 '21 at 15:55
1

@mandarancza It is logical AND. It means that "b" is going to be true only in case all the conditions are true. Any false option will bring you false as a result. – Boris Sokolov Jun 07 '21 at 16:02
1

@BorisSokolov thanks for explenation, it seems better alternative, thanks – mandarancza Jun 07 '21 at 16:14

Rodrigo Rodrigues · Answer 2 · 2021-06-12T18:22:17.540

For the OP (that seems to be just starting on programming):

It is nice that you got an assignment to reduce the cyclomatic complexity of a method, and it is good both to know what is it and how to keep it low as a general practice.

However, try to not get too zealous with these kind of metrics. There is more value on having the code as straightforward as possible, easy to reason about and understand quickly, and only worry about metrics after profiling the app and knowing the places it matters the most.

For more experienced coders:

This simple problem reminded me of a very famous discussion from 1968 between Dijkstra and other fellas on the ACM periodic. Although I tend to align with him on this matter, that was one answer from Frank Rubin that is very reasonable.

Frank basically advocates that "elegance" can many times arise from code clarity, instead of any other metrics of practices. Back then, the discussion was the over-use of the goto statement on the popular languages of that time. Today, discussion goes around cyclomatic complexity, terseness, oop or whatever.

The bottom line is, in my opinion:

know your tools
code with clarity in mind
try to write efficient code on the first pass, but don't overthink
profile your code and decide where it's work to spend more time

Back to the question

The implementation presented in the question got the following scores in my Visual Studio Analyzer:

Cycl. Compl: 5; Maintainability: 67

The snippet presented by @Boris got this:

Cycl. Compl: 4; Maintainability: 68

Even though the cyclomatic complexity improved, the maintainability index keeps basically the same. Personally, I consider the latter metric more valuable most of the time.

Just for fun, let's see how a solution akin the one presented by Frank Rubin that uses the dreaded goto statement would look like:

private bool MethodName() {
  for (int k = 0; k < 8; k++) {
    for (int j = 0; j < 3; j++) {
      if (watheverTestCondition(k, j) is false) goto reject;
    }
    // condition is true for all items in this row
    return true; 
    // if condition is false for any item, go straight to this line
    reject:;
  }
  return false;
}

Honestly, I think this is the most clear, simple and performatic implementation for this. Do I recommend goto in general as a code feature? NO. Does it fit perfectly and smoothly in this specific case? YES. And what about the metrics?

Cycl. Compl: 4; Maintainability: 70

Bonus

Just because I wouldn't be able to sleep if I didn't tell that, this is how you would implement this in real life:

obj.Any(row => row.All(watheverTestCondition));

Cycl. Compl: 1; Maintainability: 80

How to reduce cyclomatic complexity on this c# method?

2 Answers2

For the OP (that seems to be just starting on programming):

For more experienced coders:

Back to the question

Bonus