SonarQube Umgestalten, diese Methode zur Reduzierung seiner Kognitiven Komplexität

Habe ich die unten utility-Methode und ich bin mit mehreren if-Anweisungen und erste kognitive Komplexität Frage. Ich ging durch einige links, aber ich bin nicht in der Lage zu verstehen, wie sollte ich ändern mein code ohne Auswirkungen auf die Anwender dieser Methode.

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

Bitte teilen Sie Ihre Vorschläge zur Umgestaltung dieser code.

(wrapper != null && wrapper.length() > 7 && wrapper.substring(0, 6).equalsIgnoreCase("XYZ")) == false. Es sei denn, "XYZ -" nicht eigentlich "XYZ".
Das wird nie wahr sein wrapper.substring(0, 6).equalsIgnoreCase("XYZ"). Weil Sie erstellen eine Teilfolge, die sechs Zeichen lang sein (wrapper.substring(0, 6)). Daher kann es nie gleich sein XYZ.
das ist genau das, was ich dachte, wenn ich schrieb den Kommentar oben. Noch, es könnte möglich sein, da wir nicht genau wissen, was WrapperClass ist. Es könnte sein, dass WrapperClass.substring() tun nicht, was wir denken, was es tut!
Guter Punkt. Hab nicht dran gedacht.
ich hatte gehalten, so nicht, um zu zeigen, ursprünglichen Werte.. plz Ignoriere solche Dinge

InformationsquelleAutor smruti ranjan | 2018-04-10

java sonarqube

Ich glaube nicht, dass die Zusammenlegung viele if Bedingungen, um ein oder machen Sie einfach eine code-Bereinigung, zum Beispiel durch änderung der Reihenfolge einiger Anweisungen, kann Ihr problem lösen.

Ihr code nicht mit dem Prinzip der einzigen Verantwortung. Sollten Sie umgestalten dieser großen Methode, um kleinere Teile. Durch diese wird es testbar, einfacher zu warten und zu Lesen. Ich verbrachte einige Zeit und habe dieses:

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

Ich weiß nicht, die domain-Logik, so dass einige meiner Methoden haben blöde Namen etc. Wie Sie sehen können, haben Sie jetzt eine viel kleinere Methoden mit nicht zu viele äste (if Bedingungen) - einfacher zu testen (statische code ist nicht schön, aber man kann mock es durch zum Beispiel PowerMock).

Guter Punkt, um das Prinzip der einzigen Verantwortung. Aber beachten Sie: es gilt nicht nur für Methoden, sondern um Klassen zu. Es scheint, wie WrapperClass ist eine Art von monströsen Kreuzung zwischen einem String und ein Map. Aber dann vielleicht die OP hat keinen Zugriff zu dieser Klasse und können es nicht ändern.
Ja, ich bin einverstanden. Mein code ist nur ein Beispiel für "how to start". Es ist schwer, etwas umgestalten, wenn Sie nicht wissen, die domain und auch der Beispiel-code nicht kompilieren (es wurde zensiert 😛 - String Klasse nicht getExpiresAt - Methode).
Guter Fang! Vielleicht sind die OP ' s String ist nicht java.lang.String aber seine eigene Klasse mit einem import my.package.String; - Anweisung.
Vielen Dank.. Das war ein wirklich schöner Ansatz.. Sobald ich Gelegenheit habe, läuft sonar qube auf diesen code, und überprüfen Sie, ob das Problem Weg ist

InformationsquelleAutor agabrys

0

Zunächst Sonar sollten Sie weitere flags: Wiederverwendung der wrapper parameter ist in der Regel eine schlechte Praxis, NPE, wo die Anrufung wrapper.getKey weil wrapper können null sein, aber egal, nicht der Punkt...

Verringern Sie die Anzahl der if Aussagen durch die Schaffung von lokalen boolean-Variablen (oder möglicherweise 1 große if - Anweisung, wenn Sie weniger als 5 oder 6 tests, oft aber weniger lesbar). Sobald es fertig ist, sollten Sie nur 1-block-Tests diese booleschen Variablen, und haben eine return-Anweisung, wie im Beispiel oben (nicht unbedingt korrekt!):
```
boolean expired = tokenExpiryTime != null;
boolean desiredAlgo = DESIRED_ALGO.equals(key.getAlgorithm());
boolean desiredType = DESIRED_TYPE.equals(value6);
if (expired || !desiredAlgo || !desiredType) {
    return isValidWrapper;
}
```
Jedoch Ihre Kognitive Komplexität scheint ziemlich niedrig, wenn diese Art von Algorithmus es auslöst...

Anderen große Weise zu reduzieren, ein Algorithmus der Komplexität ist wiederum sub-code-Blöcke (Schleifen, if-und try-catch) in private Methoden. In deinem Beispiel könnte es so etwas wie eine checkWrapperValidity Methode, verantwortlich für jeden test, der Rückkehr isValidWrapper

InformationsquelleAutor HBo

Etwas umschreiben geliefert, eine Vereinfachung, die noch verbessert werden könnte.

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

Nach Kommentar: hier Fang ich eine Ausnahme für alle Anrufe.

Es ist nicht ganz entspricht (die aber wohl ok wie es ist). Das original wird eine Ausnahme werfen, wenn eine der Methoden aufgerufen, nachdem key = wrapper.getKey(); eine Ausnahme werfen. Ihr code wird die Rückkehr in der Regel.
du hast Recht, sollte Hinzugefügt haben, eine Bemerkung. Es wird fast nur Getter und die seltsame isValidWrapper Nutzung führte meine Aufmerksamkeit in die Irre.

InformationsquelleAutor Joop Eggen

Schreibe einen Kommentar

Du musst angemeldet sein, um einen Kommentar abzugeben.