SonarQube Refactor this method to reduce its Cognitive Complexity

We Are Going To Discuss About SonarQube Refactor this method to reduce its Cognitive Complexity. So lets Start this Java Article.

SonarQube Refactor this method to reduce its Cognitive Complexity

Advertisements
  1. SonarQube Refactor this method to reduce its Cognitive Complexity

    I don't think that merging many if conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem.

  2. SonarQube Refactor this method to reduce its Cognitive Complexity

    I don't think that merging many if conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem.

Solution 1

Advertisements

I don’t think that merging many if conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem.

Your code does not match the single responsibility principle. You should refactor this big method to smaller parts. Due to this it will testable, easier to maintain and read. I spent some time and did this:

public static boolean isWrapperValid(WrapperClass wrapper, boolean isTechnicalToken) {

    final WrapperClass unpackedWrapper = unpackWrapper(wrapper);
    boolean wrapperValid = isUnpackedWrapperValid(unpackedWrapper);

    Key key = null;
    try {
        key = unpackedWrapper.getKey();
    } catch (final Exception exception) {
        return wrapperValid;
    }

    if (key != null) {   
        if (doesKeyMeetsBasicConditions(key)) {
            return wrapperValid;
        }
        if (doesKeyMeetsValueConditions(key)) {
            return true;
        }
    }
    return wrapperValid;
}

protected static WrapperClass unpackWrapper(final WrapperClass wrapper) {      
    if (wrapper != null && wrapper.length() > 7 && wrapper.substring(0, 6).equalsIgnoreCase("XYZ")) {
        return wrapper.substring(7, wrapper.lastIndexOf('.') + 1);
    }
    return wrapper;
}

protected static boolean isUnpackedWrapperValid(final WrapperClass wrapper) {
   return wrapper != null && wrapper.equalsIgnoreCase("TFR");
}

protected static boolean doesKeyMeetsBasicConditions(final Key key) {
    Date tokenExpiryTime = key.getExpiresAt();
    if (tokenExpiryTime != null) {
        return true;
    }
    
    String algorithm = key.getAlgorithm();
    if (!DESIRED_ALGO.equals(algorithm)) {
        return true;
    }
    
    String value6 = key.getType();
    return !DESIRED_TYPE.equals(value6);
}

protected static boolean doesKeyMeetsValueConditions(final Key key) {
    return key.getValue1() != null && key.getValue2().size() > 0
           && key.getValue3() != null && key.getValue4() != null
           && key.getValue5() != null;
}

I don’t know the domain logic, so some of my methods have stupid names etc. As you can see, now you have a lot of smaller methods with not many branches (if conditions) – easier to test (a static code is not nice, but you can mock it by using for example PowerMock).

Original Author agabrys Of This Content

Solution 2

Advertisements

A bit of rewriting delivered a simplification, that still could be improved upon.

public static boolean isWrapperValid(WrapperClass wrapper, boolean isTechnicalToken){
    if (wrapper != null && wrapper.length() > 7
        && wrapper.substring(0, 6).equalsIgnoreCase("XYZ"))
    {
        wrapper = wrapper.substring(7, wrapper.lastIndexOf('.')+1);
    }
    boolean isValidWrapper = wrapper != null && wrapper.equalsIgnoreCase("TFR");

    try {
        String key = wrapper.getKey();
        if (key != null && key.getExpiresAt() == null
                && DESIRED_ALGO.equals(key.getAlgorithm())
                && DESIRED_TYPE.equals(key.getType())
                && key.getValue1() != null && !key.getValue2().isEmpty()
                && key.getValue3() != null && key.getValue4() != null
                && key.getValue5() != null) {
            isValidWrapper = true;
        }
    }
    catch (Exception exception) {
        // DO NOTHING
    }
    return isValidWrapper;
}

After comment: here I catch any exception for all calls.

Original Author Joop Eggen Of This Content

Solution 3

Advertisements

First of all, Sonar should give you more flags: reusing the wrapper parameter is usually a bad practice, NPE where invoking wrapper.getKey because wrapper can be null, but anyway, not the point…

Try reducing the number of if statements by creating local boolean variables (or possibly 1 big if statement if you have less than 5 or 6 tests, but often less readable). Once it’s done, you should only have 1 block testing these boolean variables, and have one return statement, like the example above (not necessarily accurate!):

boolean expired = tokenExpiryTime != null;
boolean desiredAlgo = DESIRED_ALGO.equals(key.getAlgorithm());
boolean desiredType = DESIRED_TYPE.equals(value6);
if (expired || !desiredAlgo || !desiredType) {
    return isValidWrapper;
}

However, your Cognitive complexity level seems pretty low if this kind of algorithm triggers it…

Another big way to reduce an algorithm complexity is to turn sub-blocks of code (loops, if and try-catch) into private methods. In your example, it could be something like a checkWrapperValidity method, responsible for every test returning isValidWrapper

Original Author HBo Of This Content

Conclusion

Advertisements

So This is all About This Tutorial. Hope This Tutorial Helped You. Thank You.

Also Read,

Siddharth

I am an Information Technology Engineer. I have Completed my MCA And I have 4 Year Plus Experience, I am a web developer with knowledge of multiple back-end platforms Like PHP, Node.js, Python and frontend JavaScript frameworks Like Angular, React, and Vue.

Leave a Comment