SonarQube重构此方法以降低其认知复杂性

Question

我有下面的实用程序方法，并且正在使用多个if语句并遇到认知复杂性问题。我浏览了一些链接，但是我不明白如何更改代码而不影响此方法的用户。

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

请分享您的建议以重构此代码。

Answer 1

我不认为将许多if条件合并到一个条件或简单地进行代码清理（例如通过更改某些指令的顺序）都可以解决您的问题。

您的代码不符合单一责任原则。您应该将此大方法重构为较小的部分。因此，它是可测试的，易于维护和阅读。我花了一些时间，这样做：

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();
    if (!DESIRED_TYPE.equals(value6)) {
        return true;
    }
    return false;
}

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

我不知道域逻辑，所以我的一些方法具有愚蠢的名称等。如您所见，现在您有许多较小的方法，分支（if条件）不多-测试起来更容易（静态代码不好） ，但您可以使用例如PowerMock来模拟它。

Answer 2

首先，Sonar 应该给你更多的标志：重用参数wrapper通常是一种不好的做法，NPE 调用的wrapper.getKey原因wrapper可能为 null，但无论如何，这不是重点......

if尝试通过创建局部布尔变量来减少语句数量（if如果您的测试少于 5 或​​ 6 个，则可能是 1 个大语句，但通常可读性较差）。完成后，您应该只有 1 个块来测试这些布尔变量，并有一个 return 语句，如上面的示例（不一定准确！）：

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

然而，如果这种算法触发它，你的认知复杂性水平似乎很低......

降低算法复杂性的另一个重要方法是将子代码块（循环、if 和 try-catch）转换为私有方法。在您的示例中，它可能类似于一种checkWrapperValidity方法，负责返回的每个测试isValidWrapper