SonarQube Refactor this method to reduce its Cognitive Complexity

Question

I have the below utility method and I am using multiple if statements and getting cognitive complexity issue. I went through some links, but I am not able to understand how should I change my code without affecting users of this method.

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

    String key=null;
    boolean isValidWrapper = false;

    if (wrapper != null && wrapper.length() > 7
        && wrapper.substring(0, 6).equalsIgnoreCase("XYZ"))
    {
        wrapper= wrapper.substring(7, wrapper.lastIndexOf('.')+1);
    }
    if(wrapper != null && wrapper.equalsIgnoreCase("TFR")) {
        isValidWrapper=Boolean.TRUE;
    }
    try {
         key = wrapper.getKey();
    }
    catch (Exception exception) {
        return isValidWrapper;
    }

    if(key!=null) {

        Date tokenExpiryTime = key.getExpiresAt();

        if(tokenExpiryTime!=null) {
            return isValidWrapper;
        }

        String algorithm=key.getAlgorithm();
        if(!DESIRED_ALGO.equals(algorithm)) {
            return isValidWrapper;
        }

        String value6=key.getType();
        if(!DESIRED_TYPE.equals(value6)) {
            return isValidWrapper;
        }


        if(key.getValue1()!=null && key.getValue2().size()>0 && key.getValue3()!=null && key.getValue4()!=null && key.getValue5()!=null) {
            isValidWrapper=Boolean.TRUE;
        }
    }

    return isValidWrapper;
}

Please share your suggestions to refactor this code.

Answer 1

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).

Answer 2

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.