Vermeiden Sie zu komplexe Methoden - Zyklomatische Komplexität

Ich bin nicht sicher, wie ich mit dieser Methode vorgehen soll, um die zyklomatische Komplexität zu verringern. Sonar meldet 13, während 10 erwartet werden. Ich bin mir sicher, dass es nicht schadet, diese Methode zu verlassen, da sie mich jedoch nur herausfordert, wie ich Sonars Regel befolgen soll. Alle mögliche Gedanken würden sehr geschätzt.

 public static long parseTimeValue(String sValue) {

    if (sValue == null) {
        return 0;
    }

    try {
        long millis;
        if (sValue.endsWith("S")) {
            millis = new ExtractSecond(sValue).invoke();
        } else if (sValue.endsWith("ms")) {
            millis = new ExtractMillisecond(sValue).invoke();
        } else if (sValue.endsWith("s")) {
            millis = new ExtractInSecond(sValue).invoke();
        } else if (sValue.endsWith("m")) {
            millis = new ExtractInMinute(sValue).invoke();
        } else if (sValue.endsWith("H") || sValue.endsWith("h")) {
            millis = new ExtractHour(sValue).invoke();
        } else if (sValue.endsWith("d")) {
            millis = new ExtractDay(sValue).invoke();
        } else if (sValue.endsWith("w")) {
            millis = new ExtractWeek(sValue).invoke();
        } else {
            millis = Long.parseLong(sValue);
        }

        return millis;

    } catch (NumberFormatException e) {
        LOGGER.warn("Number format exception", e);
    }

    return 0;
}

Alle ExtractXXX-Methoden sind als staticinnere Klassen definiert . Zum Beispiel wie eine unten -

    private static class ExtractHour {
      private String sValue;

      public ExtractHour(String sValue) {
         this.sValue = sValue;
      }

      public long invoke() {
         long millis;
         millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
         return millis;
     }
 }

UPDATE 1

Ich werde mich hier mit einer Mischung von Vorschlägen abfinden, um Sonar-Typen zufriedenzustellen. Auf jeden Fall Raum für Verbesserungen und Vereinfachungen.

Guave Functionist hier nur eine unerwünschte Zeremonie. Wollte die Frage zum aktuellen Status aktualisieren. Hier ist nichts endgültig. Gießen Sie Ihre Gedanken bitte ..

public class DurationParse {

private static final Logger LOGGER = LoggerFactory.getLogger(DurationParse.class);
private static final Map<String, Function<String, Long>> MULTIPLIERS;
private static final Pattern STRING_REGEX = Pattern.compile("^(\\d+)\\s*(\\w+)");

static {

    MULTIPLIERS = new HashMap<>(7);

    MULTIPLIERS.put("S", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractSecond(input).invoke();
        }
    });

    MULTIPLIERS.put("s", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractInSecond(input).invoke();
        }
    });

    MULTIPLIERS.put("ms", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractMillisecond(input).invoke();
        }
    });

    MULTIPLIERS.put("m", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractInMinute(input).invoke();
        }
    });

    MULTIPLIERS.put("H", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractHour(input).invoke();
        }
    });

    MULTIPLIERS.put("d", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractDay(input).invoke();
        }
    });

    MULTIPLIERS.put("w", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractWeek(input).invoke();
        }
    });

}

public static long parseTimeValue(String sValue) {

    if (isNullOrEmpty(sValue)) {
        return 0;
    }

    Matcher matcher = STRING_REGEX.matcher(sValue.trim());

    if (!matcher.matches()) {
        LOGGER.warn(String.format("%s is invalid duration, assuming 0ms", sValue));
        return 0;
    }

    if (MULTIPLIERS.get(matcher.group(2)) == null) {
        LOGGER.warn(String.format("%s is invalid configuration, assuming 0ms", sValue));
        return 0;
    }

    return MULTIPLIERS.get(matcher.group(2)).apply(matcher.group(1));
}

private static class ExtractSecond {
    private String sValue;

    public ExtractSecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = Long.parseLong(sValue);
        return millis;
    }
}

private static class ExtractMillisecond {
    private String sValue;

    public ExtractMillisecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue));
        return millis;
    }
}

private static class ExtractInSecond {
    private String sValue;

    public ExtractInSecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 1000);
        return millis;
    }
}

private static class ExtractInMinute {
    private String sValue;

    public ExtractInMinute(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 60 * 1000);
        return millis;
    }
}

private static class ExtractHour {
    private String sValue;

    public ExtractHour(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 60 * 60 * 1000);
        return millis;
    }
}

private static class ExtractDay {
    private String sValue;

    public ExtractDay(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 24 * 60 * 60 * 1000);
        return millis;
    }
}

private static class ExtractWeek {
    private String sValue;

    public ExtractWeek(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 7 * 24 * 60 * 60 * 1000);
        return millis;
    }
}

}

UPDATE 2

Obwohl ich mein Update hinzugefügt habe, ist es nur so viel die Zeit wert. Ich werde weitermachen, da Sonar sich jetzt nicht beschwert. Machen Sie sich keine Sorgen, und ich akzeptiere die Antwort von Mattnz, da dies der richtige Weg ist, und möchte nicht denjenigen ein schlechtes Beispiel geben, die auf diese Frage stoßen. Fazit - Don't over Engineer für Sonar (oder Half Baked Project Manager) beschwert sich über CC. Tun Sie einfach, was für das Projekt einen Cent wert ist. Dank an alle.

Software Engineering Antwort:

Dies ist nur einer der vielen Fälle, in denen Sie durch einfaches Zählen von Bohnen, die einfach zu zählen sind, das Falsche tun. Es ist keine komplexe Funktion, ändern Sie sie nicht. Die zyklomatische Komplexität ist nur ein Hinweis auf die Komplexität, und Sie verwenden sie nur schlecht, wenn Sie diese Funktion darauf basierend ändern. Sein einfaches, sein lesbares, sein wartbares (für jetzt), wenn es in der Zukunft größer wird, wird das CC exponentiell in die Höhe schnellen und es wird die Aufmerksamkeit bekommen, die es braucht, wenn es es braucht, nicht vorher.

Minion arbeitet für ein großes multinationales Unternehmen Antwort:

Organisationen sind voll von überbezahlten, unproduktiven Teams von Bohnenkostenzählern. Die Bohnenzähler glücklich zu halten ist einfacher und sicherlich weiser als das Richtige zu tun. Sie müssen die Routine ändern, um die CC auf 10 zu senken, aber seien Sie ehrlich, warum Sie das tun - um die Bohnenzähler von Ihrem Rücken abzuhalten. Wie in den Kommentaren vorgeschlagen, könnten "monadische Parser" helfen

Vielen Dank an @JimmyHoffa, @MichaelT und @ GlenH7 für ihre Hilfe!

Python

Als erstes sollten Sie wirklich nur bekannte Präfixe akzeptieren, das heißt entweder 'H' oder 'h'. Wenn Sie haben beide zu akzeptieren, sollten Sie eine Operation durchführen , um es konsequent um Platz in der Karte zu speichern.

In Python können Sie ein Wörterbuch erstellen.

EXTRACTION_MAP = {
    'S': ExtractSecond,
    'ms': ExtractMillisecond,
    'm': ExtractMinute,
    'H': ExtractHour,
    'd': ExtractDay,
    'w': ExtractWeek
}

Dann wollen wir, dass die Methode dies verwendet:

def parseTimeValue(sValue)
    ending = ''.join([i for i in sValue if not i.isdigit()])
    return EXTRACTION_MAP[ending](sValue).invoke()

Sollte eine bessere zyklomatische Komplexität haben.

Java

Wir brauchen nur 1 (einen) von jedem Multiplikator. Fügen wir sie in eine Karte ein, wie andere Antworten vermuten lassen.

Map<String, Float> multipliers = new HashMap<String, Float>();
    map.put("S", 60 * 60);
    map.put("ms", 60 * 60 * 1000);
    map.put("m", 60);
    map.put("H", 1);
    map.put("d", 1.0 / 24);
    map.put("w", 1.0 / (24 * 7));

Dann können wir einfach die Karte benutzen, um den richtigen Konverter zu finden

Pattern foo = Pattern.compile(".*(\\d+)\\s*(\\w+)");
Matcher bar = foo.matcher(sValue);
if(bar.matches()) {
    return (long) (Double.parseDouble(bar.group(1)) * multipliers.get(bar.group(2);
}

Da Sie ohnehin return millisam Ende dieser schrecklichen ifelseifelse stehen , fällt Ihnen als Erstes ein, den Wert sofort aus den if-Blöcken zurückzugeben. Dieser Ansatz folgt einem, der im Katalog der Refactoring-Muster als " Verschachtelte Bedingung durch Schutzklauseln ersetzen" aufgeführt ist .

Eine Methode hat ein bedingtes Verhalten, das nicht klar macht, wie der normale Ausführungspfad lautet

Verwenden Sie für alle Sonderfälle Schutzklauseln

Es wird dir helfen, die anderen loszuwerden, den Code zu reduzieren und Sonar glücklich zu machen:

    if (sValue.endsWith("S")) {
        return new ExtractSecond(sValue).invoke();
    } // no need in else after return, code flattened

    if (sValue.endsWith("ms")) {
        return new ExtractMillisecond(sValue).invoke();
    }

    // and so on...
    return Long.parseLong(sValue); // forget millis, these aren't needed anymore

Eine andere erwägenswerte Sache ist es, den Try-Catch-Block fallen zu lassen. Dies wird auch die zyklomatische Komplexität verringern, aber der Hauptgrund, warum ich es empfehle, ist, dass es mit diesem Block keine Möglichkeit für den Anrufercode gibt, legal analysierte 0 von der Ausnahme des Zahlenformats zu unterscheiden.

Wenn Sie nicht zu 200% sicher sind, dass das Zurückgeben von 0 für Analysefehler den Anforderungen des Aufrufercodes entspricht, sollten Sie diese Ausnahme weitergeben und den Aufrufercode entscheiden lassen, wie er damit umgeht. Es ist in der Regel bequemer, beim Aufrufer zu entscheiden, ob die Ausführung abgebrochen oder die Eingabe wiederholt werden soll, oder auf einen Standardwert wie 0 oder -1 oder einen anderen Wert zurückzugreifen.

Ihr Code-Snippet für ein Beispiel ExtractHour gibt mir das Gefühl, dass die ExtractXXX-Funktionalität keineswegs optimal gestaltet ist. Ich wette, jede der verbleibenden Klassen wiederholt nachdenklich dasselbe parseDoubleund substringmultipliziert Dinge wie 60 und 1000 immer und immer wieder.

Dies liegt daran, dass Sie die Essenz dessen übersehen haben, wovon abhängig ist sValue- es definiert nämlich, wie viel Zeichen vom Ende der Zeichenkette abgeschnitten werden müssen und was der Multiplikatorwert wäre. Wenn Sie Ihr "Kern" -Objekt um diese wesentlichen Features herum entwerfen, sieht es folgendermaßen aus:

private static class ParseHelper {
    // three things you need to know to parse:
    final String source;
    final int charsToCutAtEnd;
    final long multiplier;

    ParseHelper(String source, int charsToCutAtEnd, long multiplier) {
        this.source = source == null ? "0" : source; // let's handle null here
        this.charsToCutAtEnd = charsToCutAtEnd;
        this.multiplier = multiplier;
    }

    long invoke() {
        // NOTE consider Long.parseLong instead of Double.parseDouble here
        return (long) (Double.parseDouble(cutAtEnd()) * multiplier);
    }

    private String cutAtEnd() {
        if (charsToCutAtEnd == 0) {
            return source;
        }
        // write code that cuts 'charsToCutAtEnd' from the end of the 'source'
        throw new UnsupportedOperationException();
    }
}

Danach benötigen Sie einen Code, der die obigen Objekte für bestimmte Bedingungen konfiguriert, wenn diese erfüllt sind, oder ansonsten "umgeht". Dies könnte folgendermaßen geschehen:

private ParseHelper setupIfInSecond(ParseHelper original) {
    final String sValue = original.source;
    return sValue.endsWith("s") && !sValue.endsWith("ms")
            ? new ParseHelper(sValue, 1, 1000)
            :  original; // bypass
}

private ParseHelper setupIfMillisecond(ParseHelper original) {
    final String sValue = original.source;
    return sValue.endsWith("ms")
            ? new ParseHelper(sValue, 2, 1)
            : original; // bypass
}

// and so on...

Basierend auf den obigen Bausteinen könnte der Code Ihrer Methode wie folgt aussehen:

public long parseTimeValue(String sValue) {

   return setupIfSecond(
           setupIfMillisecond(
           setupIfInSecond(
           setupIfInMinute(
           setupIfHour(
           setupIfDay(
           setupIfWeek(
           new ParseHelper(sValue, 0, 1))))))))
           .invoke();
}

Sie sehen, es gibt keine Komplexität mehr, keine geschweiften Klammern innerhalb der Methode (und auch keine Mehrfachrückgaben wie in meinem ursprünglichen Brute-Force-Vorschlag zum Reduzieren von Code). Sie überprüfen einfach nacheinander die Eingabe und passen die Verarbeitung nach Bedarf an.

Wenn Sie es wirklich überarbeiten möchten, können Sie Folgendes tun:

// All of your Extract... classes will have to implement this interface!
public Interface TimeExtractor
{
    public long invoke();
}

private static class ExtractHour implements TimeExtractor
{
  private String sValue;


  /*Not sure what this was for - might not be necessary now
  public ExtractHour(String sValue)
  {
     this.sValue = sValue;
  }*/

  public long invoke(String s)
  {
     this.sValue = s;
     long millis;
     millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
     return millis;
 }
}

private static HashMap<String, TimeExtractor> extractorMap= new HashMap<String, TimeExtractor>();

private void someInitMethod()
{
   ExtractHour eh = new ExtractorHour;
   extractorMap.add("H",eh);
   /*repeat for all extractors */
}

public static long parseTimeValue(String sValue)
{
    if (sValue == null)
    {
        return 0;
    }
    String key = extractKeyFromSValue(sValue);
    long millis;
    TimeExtractor extractor = extractorMap.get(key);
    if (extractor!=null)
    {
      try
      {
         millis= extractor.invoke(sValue);
      }
        catch (NumberFormatException e)
      {
          LOGGER.warn("Number format exception", e);
      }
    }
    else
       LOGGER.error("NO EXTRACTOR FOUND FOR "+key+", with sValue: "+sValue);

    return millis;
}

Die Idee ist, dass Sie eine Karte von Schlüsseln haben (was Sie in "endsWith" die ganze Zeit verwenden), die bestimmten Objekten zugeordnet sind, die die gewünschte Verarbeitung ausführen.

Es ist hier etwas rau, aber ich hoffe, es ist klar genug. Ich habe die Details von nicht ausgefüllt, extractKeyFromSValue()weil ich einfach nicht genug weiß, was diese Saiten sind, um es richtig zu machen. Es sieht so aus, als wären es die letzten 1 oder 2 nicht numerischen Zeichen (ein Regex könnte es wahrscheinlich leicht genug extrahieren, vielleicht.*([a-zA-Z]{1,2})$ wären würde funktionieren), aber ich bin nicht 100% sicher ...

Ursprüngliche Antwort:

Sie könnten sich ändern

else if (sValue.endsWith("H") || sValue.endsWith("h")) {

zu

else if (sValue.toUpper().endsWith("H")) {

Das könnte dich ein bisschen retten, aber ehrlich gesagt würde ich mir darüber keine allzu großen Sorgen machen. Ich stimme Ihnen zu, dass es meiner Meinung nach nicht sehr schlimm ist, die Methode so zu belassen, wie sie ist. Anstatt zu versuchen, "Sonars Regeln zu befolgen", versuchen Sie, "Sonars Richtlinien so weit wie möglich zu befolgen".

Sie können sich verrückt machen, wenn Sie versuchen, jede einzelne Regel zu befolgen, die diese Analysetools enthalten, aber Sie müssen auch entscheiden, ob die Regeln für Ihr Projekt sinnvoll sind und in bestimmten Fällen, in denen sich der Zeitaufwand für das Refactoring möglicherweise nicht lohnt .

Sie können eine Aufzählung verwenden, um alle verfügbaren Fälle und Prädikate für übereinstimmende Werte zu speichern. Wie bereits erwähnt, ist Ihre Funktion lesbar genug, um sie unverändert zu lassen. Diese Metriken sollen Ihnen helfen, nicht umgekehrt.

//utility class for matching values
private static class ValueMatchingPredicate implements Predicate<String>{
    private final String[] suffixes;

    public ValueMatchingPredicate(String[] suffixes) {      
        this.suffixes = suffixes;
    }

    public boolean apply(String sValue) {
        if(sValue == null) return false;

        for (String suffix : suffixes) {
            if(sValue.endsWith(suffix)) return true;
        }

        return false;
    }

    public static Predicate<String> withSuffix(String... suffixes){         
        return new ValueMatchingPredicate(suffixes);
    }       
}

//enum containing all possible options
private static enum TimeValueExtractor {                
    SECOND(
        ValueMatchingPredicate.withSuffix("S"), 
        new Function<String, Long>(){ 
            public Long apply(String sValue) {  return new ExtractSecond(sValue).invoke(); }
        }),

    MILISECOND(
        ValueMatchingPredicate.withSuffix("ms"), 
        new Function<String, Long>(){
            public Long apply(String sValue) { return new ExtractMillisecond(sValue).invoke(); }
        }),

    IN_SECOND(
        ValueMatchingPredicate.withSuffix("s"),
        new Function<String, Long>(){
            public Long apply(String sValue) { return new ExtractInSecond(sValue).invoke(); }
        }),

    IN_MINUTE(
        ValueMatchingPredicate.withSuffix("m"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractInMinute(sValue).invoke(); }
        }),

    HOUR(
        ValueMatchingPredicate.withSuffix("H", "h"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractHour(sValue).invoke(); }
        }),

    DAY(
        ValueMatchingPredicate.withSuffix("d"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractDay(sValue).invoke(); }
        }),

    WEEK(
        ValueMatchingPredicate.withSuffix("w"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractWeek(sValue).invoke(); }
        });

    private final Predicate<String>      valueMatchingRule;
    private final Function<String, Long> extractorFunction;

    public static Long DEFAULT_VALUE = 0L;

    private TimeValueExtractor(Predicate<String> valueMatchingRule, Function<String, Long> extractorFunction) {
        this.valueMatchingRule = valueMatchingRule;
        this.extractorFunction = extractorFunction;
    }

    public boolean matchesValueSuffix(String sValue){
        return this.valueMatchingRule.apply(sValue);
    }

    public Long extractTimeValue(String sValue){
        return this.extractorFunction.apply(sValue);
    }

    public static Long extract(String sValue) throws NumberFormatException{
        TimeValueExtractor[] extractors = TimeValueExtractor.values();

        for (TimeValueExtractor timeValueExtractor : extractors) {
            if(timeValueExtractor.matchesValueSuffix(sValue)){
                return timeValueExtractor.extractTimeValue(sValue);
            }
        }

        return DEFAULT_VALUE;
    }
}

//your function
public static long parseTimeValue(String sValue){
    try{
        return TimeValueExtractor.extract(sValue);
    } catch (NumberFormatException e) {
        //LOGGER.warn("Number format exception", e);
        return TimeValueExtractor.DEFAULT_VALUE;
    }
}

Bezogen auf Ihren Kommentar von:

Fazit - Don't over Engineer für Sonar (oder Half Baked Project Manager) beschwert sich über CC. Tun Sie einfach, was für das Projekt einen Cent wert ist.

Eine weitere Möglichkeit besteht darin, die Kodierungsstandards Ihres Teams für Situationen wie diese zu ändern. Vielleicht können Sie eine Art Team-Voting hinzufügen, um ein Maß an Governance zu bieten und Kurzschlusssituationen zu vermeiden.

Das Ändern der Teamstandards als Reaktion auf Situationen, die keinen Sinn ergeben, ist jedoch ein Zeichen für ein gutes Team mit der richtigen Einstellung zu Standards. Die Standards sollen dem Team dabei helfen, Code nicht in die Quere zu kommen.

Um ehrlich zu sein, erscheinen alle oben genannten technischen Antworten für die anstehende Aufgabe schrecklich kompliziert. Wie bereits geschrieben, ist der Code selbst sauber und gut, so dass ich mich für die kleinstmögliche Änderung entscheiden würde, um dem Komplexitätszähler gerecht zu werden. Wie wäre es mit dem folgenden Refactor:

public static long parseTimeValue(String sValue) {

    if (sValue == null) {
        return 0;
    }

    try {
        return getMillis(sValue);
    } catch (NumberFormatException e) {
        LOGGER.warn("Number format exception", e);
    }

    return 0;
}

private static long getMillis(String sValue) {
    if (sValue.endsWith("S")) {
        return new ExtractSecond(sValue).invoke();
    } else if (sValue.endsWith("ms")) {
        return new ExtractMillisecond(sValue).invoke();
    } else if (sValue.endsWith("s")) {
        return new ExtractInSecond(sValue).invoke();
    } else if (sValue.endsWith("m")) {
        return new ExtractInMinute(sValue).invoke();
    } else if (sValue.endsWith("H") || sValue.endsWith("h")) {
        return new ExtractHour(sValue).invoke();
    } else if (sValue.endsWith("d")) {
        return new ExtractDay(sValue).invoke();
    } else if (sValue.endsWith("w")) {
        return new ExtractWeek(sValue).invoke();
    } else {
        return Long.parseLong(sValue);
    }
}

Wenn ich richtig zähle, sollte die extrahierte Funktion eine Komplexität von 9 haben, was immer noch den Anforderungen entspricht. Und es ist im Grunde derselbe Code wie zuvor , was gut ist, da der Code von Anfang an gut war.

Außerdem mögen Leser von Clean Code die Tatsache genießen, dass die Methode der obersten Ebene jetzt einfach und kurz ist, während die extrahierte Methode sich mit Details befasst.