How to reduce cyclomatic complexity on this c# method?

user_12827576 0
Viewed 272

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;
    }
cyclomatic-complexity c#
2 Answers

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;
    }
user_47271320

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

user_29385260
Related