Unikaj zbyt skomplikowanej metody - złożoności cyklicznej

Nie wiem, jak postępować zgodnie z tą metodą, aby zmniejszyć złożoność cyklomatyczną. Sonar donosi 13, podczas gdy spodziewane jest 10. Jestem pewien, że nie ma nic złego w pozostawieniu tej metody, ponieważ stanowi ona jedynie wyzwanie dla mnie, jak postępować zgodnie z zasadą Sonaru. Jakiekolwiek propozycje będą mile widziane.

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

Wszystkie metody ExtractXXX są zdefiniowane jako staticklasy wewnętrzne. Na przykład jak poniżej -

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

AKTUALIZACJA 1

Zamierzam zadowolić się mieszanką sugestii, aby zadowolić faceta Sonara. Zdecydowanie miejsce na ulepszenia i uproszczenia.

Guawa Functionto po prostu niechciana ceremonia. Chciał zaktualizować pytanie o bieżący stan. Nic tu nie jest ostateczne. Wlej swoje myśli, proszę ...

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

}

AKTUALIZACJA 2

Chociaż dodałem moją aktualizację, jest to tylko tyle warte czasu. Zamierzam przejść dalej, ponieważ Sonar nie narzeka. Nie przejmuj się zbytnio i akceptuję odpowiedź Mattnz, ponieważ jest to dobra droga i nie chcę dawać złego przykładu dla tych, którzy wpadają na to pytanie. Konkluzja - Nie przesadzaj z inżynierem ze względu na Sonara (lub Half Baked Project Managera) narzeka na CC. Po prostu zrób to, co jest warte ani grosza za projekt. Dziękuje za wszystko.

Odpowiedź inżynierii oprogramowania:

To tylko jeden z wielu przypadków, w których zwykłe liczenie fasoli, które można łatwo policzyć, spowoduje, że zrobisz coś złego. To nie jest złożona funkcja, nie zmieniaj jej. Cyklomatyczna złożoność jest jedynie przewodnikiem po złożoności i używasz jej słabo, jeśli zmienisz tę funkcję na jej podstawie. Jest prosty, czytelny, łatwy w utrzymaniu (na razie), jeśli w przyszłości powiększy się, CC gwałtownie wzrośnie wykładniczo i zyska uwagę, której potrzebuje, kiedy tego potrzebuje, a nie wcześniej.

Minion pracujący dla dużej międzynarodowej korporacji Odpowiedź:

Organizacje są pełne przepłaconych, nieproduktywnych zespołów liczników fasoli. Utrzymywanie liczników fasoli w stanie szczęśliwym jest łatwiejsze i na pewno mądrzejsze niż robienie właściwych rzeczy. Musisz zmienić rutynę, aby obniżyć CC do 10, ale bądź szczery, dlaczego to robisz - aby nie dopuścić do tego, aby ziarna fasoli się odwróciły. Jak sugerowano w komentarzach - pomocne mogą być „parady monadyczne”

Dzięki @JimmyHoffa, @MichaelT i @ GlenH7 za pomoc!

Pyton

Po pierwsze, naprawdę powinieneś akceptować tylko znane przedrostki, czyli „H” lub „h”. Jeśli masz do zaakceptowania zarówno, należy wykonać kilka operacji, aby je dostosować, aby zaoszczędzić miejsce w swojej mapie.

W Pythonie możesz stworzyć słownik.

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

Następnie chcemy, aby metoda wykorzystała to:

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

Powinien mieć większą złożoność cykliczną.

Jawa

Potrzebujemy tylko 1 (jednego) każdego mnożnika. Umieśćmy je na mapie, jak sugerują inne odpowiedzi.

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

Następnie możemy po prostu użyć mapy, aby pobrać odpowiedni konwerter

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

Ponieważ i tak jesteś return millisna końcu tego okropnego ifelseifelse , pierwszą rzeczą, która przychodzi na myśl, jest natychmiastowe zwrócenie wartości z wnętrza if-bloków. Podejście to jest zgodne z podejściem wymienionym w katalogu wzorców refaktoryzacji jako Zamień zagnieżdżone warunkowe na klauzule zabezpieczające .

Metoda ma zachowanie warunkowe, które nie wyjaśnia, jaka jest normalna ścieżka wykonania

Użyj klauzul ochronnych we wszystkich szczególnych przypadkach

Pomoże ci to pozbyć się innych, spłaszczy kod i sprawi, że Sonar będzie szczęśliwy:

    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

Kolejną rzeczą wartą rozważenia jest porzucenie bloku try-catch. Spowoduje to również zmniejszenie złożoności cyklicznej, ale głównym powodem, dla którego zalecam, jest to, że w tym bloku nie ma sposobu, aby kod wywołujący mógł odróżnić poprawnie przeanalizowane 0 od wyjątku formatu liczb.

Jeśli nie masz 200% pewności, że zwracanie 0 za błędy analizy jest tym, czego potrzebuje kod dzwoniącego, lepiej propaguj ten wyjątek i pozwól, aby kod dzwoniącego zdecydował, jak sobie z tym poradzić. Zazwyczaj wygodniej jest zdecydować u dzwoniącego, czy przerwać wykonywanie, czy ponowić próbę uzyskania danych wejściowych, czy też powrócić do wartości domyślnej, takiej jak 0 lub -1 lub cokolwiek innego.

Twój fragment kodu na przykład ExtractHour sprawia, że ​​czuję, że funkcjonalność ExtractXXX została zaprojektowana w sposób daleki od optymalnego. Założę się, każdy z pozostałych klas bezmyślnie powtarza to samo parseDoublei substring, i mnożąc rzeczy jak 60 i 1000 w kółko i kółko.

Wynika to z tego, że nie zauważyłeś istoty tego, co należy zrobić w zależności od sValue- a mianowicie określa, ile znaków należy wyciąć z końca łańcucha i jaka byłaby wartość mnożnika. Jeśli projektujesz swój „rdzeń” wokół tych podstawowych funkcji, wyglądałby on następująco:

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

Następnie potrzebujesz kodu, który konfiguruje powyższe obiekty dla określonych warunków, jeśli jest spełniony, lub w jakiś sposób „omija” inaczej. Można to zrobić w następujący sposób:

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

W oparciu o powyższe elementy składowe kod Twojej metody może wyglądać następująco:

public long parseTimeValue(String sValue) {

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

Widzisz, nie ma już złożoności, żadnych nawiasów klamrowych w metodzie (ani wielu zwrotów, jak w mojej oryginalnej sugestii brutalnej siły na spłaszczonym kodzie). Wystarczy sekwencyjnie sprawdzić dane wejściowe i odpowiednio dostosować przetwarzanie.

Jeśli naprawdę chcesz to zmienić, możesz zrobić coś takiego:

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

Chodzi o to, że masz mapę kluczy (co zawsze używasz w „kończy się”), które mapują do określonych obiektów, które wykonują pożądane przetwarzanie.

Jest tu trochę szorstko, ale mam nadzieję, że jest wystarczająco jasny. Nie podałem szczegółów, extractKeyFromSValue()ponieważ po prostu nie wiem wystarczająco dużo o tych ciągach, aby zrobić to poprawnie. Wygląda na to, że są to ostatnie 1 lub 2 znaki nienumeryczne (regex prawdopodobnie mógłby go łatwo wyodrębnić, może .*([a-zA-Z]{1,2})$zadziałałby), ale nie jestem w 100% pewien ...

Oryginalna odpowiedź:

Mógłbyś się zmienić

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

do

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

To może trochę cię uratować, ale szczerze mówiąc, nie martwiłbym się tym zbytnio. Zgadzam się z tobą, że nie sądzę, aby pozostawienie tej metody było w jakikolwiek sposób szkodliwe. Zamiast próbować „przestrzegać zasad Sonaru”, staraj się „trzymać się wytycznych Sonaru tak dalece, jak to możliwe”.

Możesz doprowadzać się do szaleństwa, próbując przestrzegać każdej reguły, którą będą w sobie te narzędzia analityczne, ale musisz także zdecydować, czy reguły mają sens dla twojego projektu, a także w szczególnych przypadkach, w których czas poświęcony na refaktoryzację może nie być tego wart .

Możesz rozważyć użycie wyliczenia do przechowywania wszystkich dostępnych obserwacji i predykatów w celu dopasowania wartości. Jak wspomniano wcześniej, twoja funkcja jest wystarczająco czytelna, aby pozostawić ją bez zmian. Te dane są po to, by ci pomóc, a nie na odwrót.

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

Związane z Twoim komentarzem do:

Konkluzja - Nie przesadzaj z inżynierem ze względu na Sonara (lub Half Baked Project Managera) narzeka na CC. Po prostu zrób to, co jest warte ani grosza za projekt.

Inną opcją do rozważenia jest zmiana standardów kodowania zespołu w takich sytuacjach. Być może możesz dodać głosowanie zespołowe, aby zapewnić miarę rządzenia i uniknąć sytuacji skrótowych.

Ale zmiana standardów zespołu w odpowiedzi na sytuacje, które nie mają sensu, jest oznaką dobrego zespołu z właściwym podejściem do standardów. Standardy mają pomóc zespołowi, a nie przeszkadzać w pisaniu kodu.

Szczerze mówiąc, wszystkie powyższe odpowiedzi techniczne wydają się okropnie skomplikowane dla danego zadania. Jak już napisano, sam kod jest czysty i dobry, dlatego wybrałbym najmniejszą możliwą zmianę, aby zaspokoić licznik złożoności. Co powiesz na następujący refaktor:

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

Jeśli liczę poprawnie, wyodrębniona funkcja powinna mieć złożoność 9, co nadal spełnia wymagania. Jest to w zasadzie ten sam kod, co wcześniej , co jest dobrą rzeczą, ponieważ kod był dobry na początek.

Ponadto czytelnicy Clean Code mogą cieszyć się z faktu, że metoda najwyższego poziomu jest teraz prosta i krótka, podczas gdy wyodrębniona zajmuje się szczegółami.