How to improve Cyclomatic Complexity?

问题:

Cyclomatic Complexity will be high for methods with a high number of decision statements including if/while/for statements. So how do we improve on it?

I am handling a big project where I am supposed to reduced the CC for methods that have CC > 10. And there are many methods with this problem. Below I will list down some eg of code patterns (not the actual code) with the problems I have encountered. Is it possible that they can be simplified?

Example of cases resulting in many decision statements:

Case 1)

if(objectA != null) //objectA is a pass in as a parameter
{
 objectB = doThisMethod();
 if(objectB != null)
 {
  objectC = doThatMethod();
  if(objectC != null)
  {
   doXXX();
  }
  else{
   doYYY();
  }
 }
 else
 {
  doZZZ();
 }
}

Case 2)

if(a < min)
 min = a;

if(a < max)
 max = a;

if(b > 0)
 doXXX();

if(c > 0)
{
 doYYY();
}
else
{
 doZZZ();
 if(c > d)
  isTrue = false;

 for(int i=0; i<d; i++)
  s[i] = i*d;

 if(isTrue)
 {
  if(e > 1)
  {
   doALotOfStuff();
  }
 }
}

Case 3)

// note that these String Constants are used elsewhere as diff combination,
// so you can't combine them as one
if(e.PropertyName.Equals(StringConstants.AAA) ||
e.PropertyName.Equals(StringConstants.BBB) ||
e.PropertyName.Equals(StringConstants.CCC) ||
e.PropertyName.Equals(StringConstants.DDD) ||
e.PropertyName.Equals(StringConstants.EEE) ||
e.PropertyName.Equals(StringConstants.FFF) ||
e.PropertyName.Equals(StringConstants.GGG) ||
e.PropertyName.Equals(StringConstants.HHH) ||
e.PropertyName.Equals(StringConstants.III) ||
e.PropertyName.Equals(StringConstants.JJJ) ||
e.PropertyName.Equals(StringConstants.KKK)) 
{
 doStuff();
}

回答1:

Case 1 - deal with this simply by refactoring into smaller functions. E.g. the following snippet could be a function:

objectC = doThatMethod();
if(objectC != null)
{
 doXXX();
}
else{
 doYYY();
}

Case 2 - exactly the same approach. Take the contents of the else clause out into a smaller helper function

Case 3 - make a list of the strings you want to check against, and make a small helper function that compares a string against many options (could be simplified further with linq)

var stringConstants = new string[] { StringConstants.AAA, StringConstants.BBB etc };
if(stringConstants.Any((s) => e.PropertyName.Equals(s))
{
    ...
}

回答2:

You should use the refactoring Replace Conditional with Polymorphism to reduce CC.

The difference between conditional an polymorphic code is that the in polymorphic code the decision is made at run time. This gives you more flexibility to add\change\remove conditions without modifying the code. You can test the behaviors separately using unit tests which improves testability. Also since there will be less conditional code means that the code is easy to read and CC is less.

For more look into behavioral design patterns esp. Strategy.

I would do the first case like this to remove the conditionals and consequently the CC. Moreover the code is more Object Oriented, readable and testable as well.

void Main() {
    var objectA = GetObjectA();
    objectA.DoMyTask();
}

GetObjectA(){
    return If_All_Is_Well ? new ObjectA() : new EmptyObjectA();
}

class ObjectA() {
    DoMyTask() {
        var objectB = GetObjectB();
        var objectC = GetObjectC();
        objectC.DoAnotherTask();     // I am assuming that you would call the doXXX or doYYY methods on objectB or C because otherwise there is no need to create them
    }

    void GetObjectC() {
        return If_All_Is_Well_Again ? new ObjectC() : new EmptyObjectC();
    }
}

class EmptyObjectA() { // http://en.wikipedia.org/wiki/Null_Object_pattern
    DoMyTask() {
        doZZZZ();
    }
}

class ObjectC() {
    DoAnotherTask() {
        doXXX();
    }
}

class EmptyObjectB() { 
    DoAnotherTask() {
        doYYY();
    }
}

In second case do it the same was as first.

In the third case -

var myCriteria = GetCriteria();
if(myCriteria.Contains(curretnCase))
    doStuff();

IEnumerable<Names> GetCriteria() {
   // return new list of criteria.
}

回答3:

I'm not a C# programmer, but I will take a stab at it.

In the first case I would say that the objects should not be null in the first place. If this is unavoidable (it is usually avoidable) then I would use the early return pattern:

if ( objectA == NULL ) {
    return;
}
// rest of code here

The second case is obviously not realistic code, but I would at least rather say:

if ( isTrue && e > 1 ) {
    DoStuff();
}

rather than use two separate ifs.

And in the last case, I would store the strings to be tested in an array/vector/map and use that containers methods to do the search.

And finally, although using cyclomatic complexity is "a good thing" (tm) and I use it myself, there are some functions which naturally have to be a bit complicated - validating user input is an example. I often wish that the CC tool I use (Source Monitor at http://www.campwoodsw.com - free and very good) supported a white-list of functions that I know must be complex and which I don't want it to flag.

回答4:

The last if in case 2 can be simplified:

 if(isTrue)
 {
  if(e > 1)
  {

can be replaced by

if(isTrue && (e>1))

case 3 can be rewritten as:

new string[]{StringConstants.AAA,...}
    .Contains(e.PropertyName)

you can even make the string array into a HashSet<String> to get O(1) performance.