Evita il metodo troppo complesso - Complessità ciclomatica

23

Non sei sicuro su come procedere con questo metodo per ridurre la complessità ciclomatica. Sonar riporta 13 mentre è previsto 10. Sono sicuro che non c'è niente di male nel lasciare questo metodo così com'è, ma solo sfidandomi su come obbedire alla regola di Sonar. Qualsiasi pensiero sarebbe molto apprezzato.

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

Tutti i metodi ExtractXXX sono definiti come static inner classes. Ad esempio, come uno di seguito -

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

AGGIORNAMENTO 1

Mi accontento di un mix di suggerimenti per soddisfare Sonar. Sicuramente spazio per miglioramenti e semplificazione.

Guava Function è solo una cerimonia indesiderata qui. Volevo aggiornare la domanda sullo stato attuale. Niente è definitivo qui. Versa i tuoi pensieri per favore ..

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

}

AGGIORNAMENTO 2

Anche se ho aggiunto il mio aggiornamento, ne valeva solo la pena. Ho intenzione di andare avanti dal momento che Sonar ora non si lamenta. Non ti preoccupare molto e sto accettando la risposta di mattnz in quanto è la strada da percorrere e non voglio dare un cattivo esempio a chi si imbatte in questa domanda. Bottom line - Non over engineer per il bene di Sonar (o Half Baked Project Manager) si lamenta di CC. Fai ciò che vale un centesimo per il progetto. Grazie a tutti.

    
posta asyncwait 25.11.2013 - 21:41
fonte

7 risposte

46

Risposta all'ingegnerizzazione del software:

Questo è solo uno dei tanti casi in cui il semplice conteggio dei fagioli che sono semplici da contare ti farà fare la cosa sbagliata. Non è una funzione complessa, non cambiarlo. La complessità ciclomatica è semplicemente una guida alla complessità, e la stai usando male se cambi questa funzione in base ad essa. È semplice, leggibile, è mantenibile (per ora), se diventerà più grande in futuro il CC salirà alle stelle in modo esponenziale e otterrà l'attenzione di cui ha bisogno quando ne avrà bisogno, non prima.

Minion che lavora per una grande multinazionale Risposta:

Le organizzazioni sono piene di squadre di contatori di bean strapagati e non produttivi. Mantenere felici i banchi di fagioli è più facile, e certamente più saggio, che fare la cosa giusta. Devi cambiare la routine per portarlo a 10, ma sinceramente sul perché lo stai facendo - per tenere lontani i banchi di fagioli. Come suggerito nei commenti - "parser monadici" potrebbe aiutare

    
risposta data 25.11.2013 - 22:26
fonte
15

Grazie a @JimmyHoffa, @MichaelT e @ GlenH7 per il loro aiuto!

Python

Per prima cosa, dovresti accettare solo i prefissi noti, ovvero "H" o "h". Se hai per accettare entrambi, devi eseguire alcune operazioni per renderlo coerente per salvare spazio nella tua mappa.

In python puoi creare un dizionario.

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

Quindi vogliamo che il metodo usi questo:

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

Dovrebbe avere una migliore complessità ciclomatica.

Java

Abbiamo solo bisogno di 1 (uno) di ciascun moltiplicatore. Inserirli in una mappa come suggerito da altre risposte.

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

Quindi possiamo semplicemente usare la mappa per prendere il convertitore giusto

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);
}
    
risposta data 25.11.2013 - 22:26
fonte
3

Dato che return millis alla fine di quel terribile ifelseifelse in ogni caso, la prima cosa che viene in mente è di restituire immediatamente il valore dall'interno di if-blocks. Questo approccio ne segue uno che è elencato nel catalogo dei pattern di refactoring come Sostituisci nidificati condizionali con clausole di guardia .

A method has conditional behavior that does not make clear what the normal path of execution is

Use Guard Clauses for all the special cases

Ti aiuterà a sbarazzarti di altri, appiattisci il codice e rende felice Sonar:

    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

Un'altra cosa che vale la pena considerare è quella di eliminare il blocco try-catch. Ciò ridurrà anche la complessità ciclomatica, ma il motivo principale per cui lo raccomando è che con questo blocco, il codice del chiamante non è in grado di distinguere lo 0 dall'eccezione di formato numero legalmente analizzata.

A meno che tu non sia sicuro al 200% che restituire 0 per gli errori di analisi è ciò di cui ha bisogno il codice del chiamante, è meglio propagarlo e lasciare che il codice del chiamante decida come gestirlo. In genere è più conveniente decidere al chiamante se interrompere l'esecuzione o riprovare a ricevere l'input, oppure tornare a un valore predefinito come 0 o -1 o qualsiasi altra cosa.

Il tuo frammento di codice per un esempio ExtractHour mi fa pensare che la funzionalità ExtractXXX sia progettata in un modo tutt'altro che ottimale. Scommetto su tutte le restanti classi spensieratamente ripete stesso parseDouble e substring , e moltiplicando roba come 60 e 1000 ancora e ancora e ancora e ancora.

Questo perché hai perso l'essenza di ciò che deve essere fatto a seconda di sValue - cioè, definisce quanti caratteri tagliare dalla fine della stringa e quale sarebbe il moltiplicatore valore. Se progetti il tuo oggetto "core" attorno a queste caratteristiche essenziali, sarebbe come segue:

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

Dopodiché, avresti bisogno di un codice che configura gli oggetti al di sopra di ogni particolare condizione se è soddisfatto, o in qualche modo "ignora" altrimenti. Questo potrebbe essere fatto come segue:

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

In base ai precedenti blocchi predefiniti , il codice del tuo metodo potrebbe essere il seguente:

public long parseTimeValue(String sValue) {

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

Vedete, non c'è alcuna complessità rimasta, nessuna parentesi graffa all'interno del metodo (né più resi come nel mio originale suggerimento di forza bruta sul codice di appiattimento). Basta controllare sequenzialmente l'input e regolare l'elaborazione secondo necessità.

    
risposta data 26.11.2013 - 12:55
fonte
1

Se vuoi veramente refactarlo, puoi fare qualcosa di simile:

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

L'idea è che tu abbia una mappa di chiavi (ciò che usi in "endsWith" per tutto il tempo) che mappa su oggetti specifici che eseguono l'elaborazione che desideri.

È un po 'difficile qui ma spero sia abbastanza chiaro. Non ho inserito i dettagli di extractKeyFromSValue() perché non so abbastanza di cosa siano queste stringhe per farlo correttamente. Sembra che siano gli ultimi 1 o 2 caratteri non numerici (una regex potrebbe probabilmente estrarla abbastanza facilmente, forse .*([a-zA-Z]{1,2})$ funzionerebbe), ma non sono sicuro al 100% ...

Risposta originale:

Potresti cambiare

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

a

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

Questo potrebbe farti risparmiare un po ', ma onestamente, non mi preoccuperei troppo di ciò. Sono d'accordo con te sul fatto che non penso che ci sia molto danno nel lasciare il metodo così com'è. Invece di cercare di "obbedire alle regole di Sonar", cerca di "stare vicino alle linee guida di Sonar, per quanto ragionevolmente possibile".

Puoi impazzire cercando di seguire ogni singola regola che questi strumenti di analisi avranno in loro, ma devi anche decidere se le regole hanno un senso per il tuo progetto, e per casi specifici in cui il tempo impiegato per il refactoring potrebbe non vale la pena.

    
risposta data 25.11.2013 - 21:55
fonte
0

Potresti prendere in considerazione l'utilizzo di un enum per archiviare tutti i casi e i predicati disponibili per i valori corrispondenti. Come accennato prima, la tua funzione è sufficientemente leggibile solo per lasciarla invariata. Quelle metriche sono lì per aiutarti non viceversa.

//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;
    }
}
    
risposta data 02.12.2013 - 23:17
fonte
0

In relazione al tuo commento su:

Bottom line -- Don't over engineer for the sake of Sonar (or Half Baked Project Manager) complains about CC. Just do what's worth a penny for the project.

Un'altra opzione da considerare è quella di cambiare gli standard di codifica della tua squadra in situazioni come questa. Forse puoi aggiungere una sorta di voto di squadra per fornire una misura di governance ed evitare situazioni di scorciatoia.

Ma cambiare gli standard del team in risposta a situazioni che non hanno senso è un segno di una buona squadra con la giusta attitudine agli standard. Gli standard sono lì per aiutare il team, non ostacolare la scrittura del codice.

    
risposta data 22.12.2013 - 18:08
fonte
0

Per essere onesti, tutte le risposte tecniche di cui sopra sembrano terribilmente complicate per il compito da svolgere. Come è stato già scritto, il codice stesso è pulito e buono, quindi opterei per il minimo cambiamento possibile per soddisfare il contatore della complessità. Che ne dici del seguente refactoring:

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

Se sto contando correttamente, la funzione estratta dovrebbe avere una complessità di 9, che comunque supera i requisiti. Ed è fondamentalmente lo stesso codice di prima , che è una buona cosa, dal momento che il codice era buono per cominciare.

Inoltre, i lettori di Clean Code potrebbero apprezzare il fatto che il metodo di livello superiore è ora semplice e breve, mentre quello estratto tratta i dettagli.

    
risposta data 20.05.2016 - 12:07
fonte