The Cyclomatic Complexity of this method is greater than authorized

Question

I am using below method for checking null or empty field:

public boolean isVoNotNull(){
        return null != this.cardNo &&  StringUtils.isNotBlank(this.cardNo) 
                && null != this.otp && StringUtils.isNotBlank(this.otp) 
                && null != this.password && StringUtils.isNotBlank(this.password)
                && null != this.userid && StringUtils.isNotBlank(this.userid) 
                && null != this.type && StringUtils.isNotBlank(this.type) 
                && null != this.walletMobileNo && StringUtils.isNotBlank(this.walletMobileNo);
    }

But getting below exception while this code is validating with SonarLint

EXCEPTION : The Cyclomatic Complexity of this method "isVoNotNull" is 12 which is greater than 10 authorized.

How can I solve this exception or how can I remove the complexity from the code?

If you're using `StringUtils` from Apache, then why do you check for `null` there? — Tom, May 05 '16 at 09:23

score 18 · Accepted Answer · edited May 23 '17 at 12:18

You need to analyze duplicated code and refactor it into a reusable method.

Given your original snippet,

public boolean isVoNotNull() {
    return null != this.cardNo &&  StringUtils.isNotBlank(this.cardNo) 
        && null != this.otp && StringUtils.isNotBlank(this.otp) 
        && null != this.password && StringUtils.isNotBlank(this.password)
        && null != this.userid && StringUtils.isNotBlank(this.userid) 
        && null != this.type && StringUtils.isNotBlank(this.type) 
        && null != this.walletMobileNo && StringUtils.isNotBlank(this.walletMobileNo);
}

we can identify the following repetitive part:

null != this.xxx && StringUtils.isNotBlank(this.xxx)

Given that StringUtils#isNotBlank() already checks for null, we can further simplify it.

StringUtils.isNotBlank(this.xxx)

Given that you need to invoke this a variable number of times, best would be to refactor it to a method taking a variable number of arguments which checks them all in a loop.

public static boolean isNoneBlank(String... strings) {
    for (String string : strings) {
        if (!StringUtils.isNotBlank(string)) {
            return false;
        }
    }
    return true;
}

Or if you're already on Java 8 with Streams and Lambda support:

public static boolean isNoneBlank(String... strings) {
    return Arrays.stream(strings).allMatch(StringUtils::isNotBlank);
}

Now you can make use of it as below:

public boolean isVoNotNull() {
    return isNoneBlank(this.cardNo, this.otp, this.password, this.userid, this.type, this.walletMobileNo);
}

You could further reduce the boilerplate by removing the unnecessary this.

public boolean isVoNotNull() {
    return isNoneBlank(cardNo, otp, password, userid, type, walletMobileNo);
}

This all was an appliance of the Don't Repeat Yourself (DRY) software engineering principle.

That said, as msandiford pointed out, it turns out that Apache Commons Lang StringUtils has since version 3.2 already exactly this method. So if you don't have it yet, consider upgrading Apache Commons Lang to at least 3.2.

public boolean isVoNotNull() {
    return StringUtils.isNoneBlank(cardNo, otp, password, userid, type, walletMobileNo);
}

Best answer for me, it shows the refactoring process wich is much more valuable than the final correct code snippet — CoffeDeveloper, May 05 '16 at 10:21

clstrfsck · Answer 2 · 2016-05-05T10:16:35.887

As others have pointed out the null check is redundant as isNotBlank checks this too.

So, if you are using Apache StringUtils 3.2 or later, you can just use StringUtils.isNoneBlank(...)

public boolean isVoNotNull() {
    return StringUtils.isNoneBlank(cardNo, otp, password, userid, type, walletMobileNo);
}

If you are using an earlier version, you could easily write one:

public static boolean isNoneBlank(CharSequence... seqs) {
  for (CharSequence seq : seqs) {
    if (StringUtils.isBlank(seq))
      return false;
  }
  return true;
}

Or in Java 8:

public static boolean isNoneBlank(CharSequence... seqs) {
  return Stream.of(seqs).allMatch(StringUtils::isNotBlank);
}

CoffeDeveloper · Answer 3 · 2016-05-05T10:14:07.113

Add the strings to an array and check with a loop for nullity and not blank-ness. Worst performance of course but should break that non-sensed limit.

Example code:

public boolean isVoNotNull(){
    string [] stringsToCheck = new string[]{
    this.cardNo,
    this.otp,
    // ...
    }

    //return CheckStringArrayNotNull(stringsToCheck);        
    return StringUtils.isNoneBlank(stringsToCheck);
}

// high chance this method already exists inside StringUtils
// INFACT FOUND! :)
private boolean CheckStringArrayNotNull( string [] stringsToCheck){
     for(int i=0; i<stringsToCheck.length(); i++)
        if( StringUtils.isNotBlank( stringsToCheck[i]) == false)
            return false;

    return true; 
}

or even better, (EDIT: seems @msandiford beat me on time for this solution)

public boolean isVoNotNull(){      
    return StringUtils.isNoneBlank(cardNo, otp, ...);
}

You could of course break into more methods to reduce cyclomatic complexity but that makes the class even harder to read and understand. Code metrics should be used only to investigate problems, not as a development mantra, for example I can take a very stupid class with 100 methods each with Cyclomatic complexity of 100 and refactor it so that it has perfect code metrics but is impossible to understand. Never look at code metrics, think to good solutions without using coding metrics.

Troncador · Answer 4 · 2016-05-05T10:11:24.463

StringUtils.isNotBlank

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isNotBlank%28java.lang.CharSequence%29

"Checks if a CharSequence is not empty (""), not null and not whitespace only."

The function already check if the object is null, then you can simplify

public boolean isVoNotNull() { 
  return StringUtils.isNotBlank(this.cardNo) 
      && StringUtils.isNotBlank(this.otp) 
      && StringUtils.isNotBlank(this.password)
      && StringUtils.isNotBlank(this.userid) 
      && StringUtils.isNotBlank(this.type) 
      && StringUtils.isNotBlank(this.walletMobileNo);
}

To reduce the number of conditional operators, you can do something like this:

public boolean isVoNotNull(){
  Boolean[] condition = new Boolean[] {
      StringUtils.isNotBlank(this.cardNo), 
      StringUtils.isNotBlank(this.otp),
      StringUtils.isNotBlank(this.password),
      StringUtils.isNotBlank(this.userid), 
      StringUtils.isNotBlank(this.type),
      StringUtils.isNotBlank(this.walletMobileNo)
  };
  return BooleanUtils.and(condition);
}

Maybe is a little tricky, but it work

The Cyclomatic Complexity of this method is greater than authorized

4 Answers4