Evite el método demasiado complejo - Complejidad ciclomática

No estoy seguro de cómo usar este método para reducir la Complejidad Ciclomática. Sonar informa 13 mientras que se espera 10. Estoy seguro de que no hay nada malo en dejar este método, ya que es, sin embargo, solo me desafía a obedecer la regla de Sonar. Cualquier pensamiento sería muy apreciado.

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

Todos los métodos ExtractXXX se definen como staticclases internas. Por ejemplo, como uno a continuación:

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

ACTUALIZACIÓN 1

Voy a establecerme con una mezcla de sugerencias aquí para satisfacer al tipo Sonar. Definitivamente espacio para mejoras y simplificación.

La guayaba Functiones solo una ceremonia no deseada aquí. Quería actualizar la pregunta sobre el estado actual. Nada es definitivo aquí. Vierte tus pensamientos por favor ...

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

}

ACTUALIZACIÓN 2

Aunque agregué mi actualización, vale la pena el tiempo. Voy a seguir adelante ya que Sonar ahora no se queja. No se preocupe mucho y estoy aceptando la respuesta de Mattnz, ya que es el camino a seguir y no quiero dar un mal ejemplo para aquellos que se topan con esta pregunta. En pocas palabras: no se exceda en ingeniería por Sonar (o Half Baked Project Manager) se queja de CC. Simplemente haga lo que vale un centavo por el proyecto. Gracias a todos.

Respuesta de ingeniería de software:

Este es solo uno de los muchos casos en los que simplemente contar frijoles que son fáciles de contar te hará hacer lo incorrecto. No es una función compleja, no la cambie. La Complejidad Ciclomática es simplemente una guía para la complejidad, y la está usando mal si cambia esta función en función de ella. Es simple, es legible, es mantenible (por ahora), si crece en el futuro, el CC se disparará exponencialmente y obtendrá la atención que necesita cuando lo necesita, no antes.

Minion trabajando para una gran corporación multinacional Respuesta:

Las organizaciones están llenas de equipos de contadores de frijoles excesivamente pagados y poco productivos. Mantener contentos los contadores de frijoles es más fácil, y ciertamente más sabio, que hacer lo correcto. Necesitas cambiar la rutina para que el CC baje a 10, pero sé honesto sobre por qué lo estás haciendo: para mantener los contadores de frijoles alejados de tu espalda. Como se sugiere en los comentarios, los "analizadores monádicos" podrían ayudar

¡Gracias a @JimmyHoffa, @MichaelT y @ GlenH7 por su ayuda!

Pitón

Lo primero es lo primero, realmente solo debes aceptar prefijos conocidos, es decir, 'H' o 'h'. Si tiene que aceptar ambos, debe realizar alguna operación para que sea coherente ahorrar espacio en su mapa.

En python podrías crear un diccionario.

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

Entonces queremos el método para usar esto:

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

Debería tener una mejor complejidad ciclomática.

Java

Solo necesitamos 1 (uno) de cada multiplicador. Vamos a ponerlos en un mapa como sugieren algunas otras respuestas.

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

Entonces podemos usar el mapa para tomar el convertidor correcto

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

Ya que return millisal final de ese horrible ifelseifelse de todos modos, lo primero que viene a la mente es devolver el valor inmediatamente desde dentro de los bloques if. Este enfoque sigue uno que figura en el catálogo de patrones de refactorización como Reemplazar condicional anidado por cláusulas de protección .

Un método tiene un comportamiento condicional que no deja en claro cuál es la ruta normal de ejecución

Use cláusulas de guardia para todos los casos especiales

Te ayudará a deshacerte de los demás, aplanar el código y hacer feliz a 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

Otra cosa que vale la pena considerar es soltar el bloque try-catch. Esto también reducirá la complejidad ciclomática, pero la razón principal por la que lo recomiendo es que con este bloque, no hay forma de que el código de la persona que llama distinga el 0 legalmente analizado de la excepción de formato de número.

A menos que esté 200% seguro de que devolver el 0 por errores de análisis es lo que necesita el código de la persona que llama, es mejor que propague esa excepción y permita que el código de la persona que llama decida cómo tratarla. Por lo general, es más conveniente decidir en la persona que llama si anular la ejecución o volver a intentar la entrada, o recurrir a algún valor predeterminado como 0 o -1 o lo que sea.

Su fragmento de código, por ejemplo, ExtractHour me hace sentir que la funcionalidad ExtractXXX está diseñada de una manera lejos de ser óptima. Apuesto a que todas las clases restantes repiten sin pensar lo mismo parseDoubley substringmultiplican cosas como 60 y 1000 una y otra vez.

Esto se debe a que se perdió la esencia de lo que se debe hacer sValue, es decir, define cuántos caracteres se deben cortar desde el final de la cadena y cuál sería el valor del multiplicador. Si diseña su objeto "núcleo" en torno a estas características esenciales, se vería de la siguiente manera:

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

Después de eso, necesitaría un código que configure los objetos anteriores por cada condición particular si se cumple, o de alguna manera "omite" lo contrario. Esto podría hacerse de la siguiente manera:

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

Según los bloques de construcción anteriores , el código de su método podría tener el siguiente aspecto:

public long parseTimeValue(String sValue) {

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

Verá, no queda complejidad, no hay llaves dentro del método (ni retornos múltiples como en mi sugerencia de fuerza bruta original en el código de aplanamiento). Simplemente verifica secuencialmente la entrada y ajusta el procesamiento según sea necesario.

Si realmente quieres refactorizarlo, puedes hacer algo como esto:

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

La idea es que tenga un mapa de teclas (lo que está usando en "endsWith" todo el tiempo) que se asignan a objetos específicos que realizan el procesamiento que desea.

Aquí es un poco difícil, pero espero que sea lo suficientemente claro. No completé los detalles extractKeyFromSValue()porque simplemente no sé lo suficiente de qué son estas cadenas para hacerlo correctamente. Parece que son los últimos 1 o 2 caracteres no numéricos (una expresión regular probablemente podría extraerlo con la suficiente facilidad, tal vez .*([a-zA-Z]{1,2})$funcionaría), pero no estoy 100% seguro ...

Respuesta original:

Podrías cambiar

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

a

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

Eso podría ahorrarte un poco, pero, sinceramente, no me preocuparía demasiado. Estoy de acuerdo con usted en que no creo que haya mucho daño en dejar el método como está. En lugar de tratar de "obedecer las reglas de Sonar", trate de "mantenerse cerca de las pautas de Sonar, tanto como sea razonablemente posible".

Puede volverse loco tratando de seguir todas las reglas que estas herramientas de análisis tendrán en ellas, pero también debe decidir si las reglas tienen sentido para su proyecto y para casos específicos en los que el tiempo dedicado a la refactorización podría no valer la pena. .

Puede considerar usar una enumeración para almacenar todos sus casos y predicados disponibles para valores coincidentes. Como se mencionó anteriormente, su función es lo suficientemente legible como para dejarla sin cambios. Esas métricas están ahí para ayudarlo, no al revés.

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

Relacionado con tu comentario de:

En pocas palabras: no se exceda en ingeniería por Sonar (o Half Baked Project Manager) se queja de CC. Simplemente haga lo que vale un centavo por el proyecto.

Otra opción a considerar es cambiar los estándares de codificación de su equipo para situaciones como esta. Quizás pueda agregar algún tipo de voto de equipo para proporcionar una medida de gobernanza y evitar situaciones de atajo.

Pero cambiar los estándares del equipo en respuesta a situaciones que no tienen sentido es una señal de un buen equipo con la actitud correcta sobre los estándares. Los estándares están ahí para ayudar al equipo, no para obstaculizar la escritura del código.

Para ser sincero, todas las respuestas técnicas anteriores parecen terriblemente complicadas para la tarea en cuestión. Como ya se escribió, el código en sí es limpio y bueno, por lo que optaría por el cambio más pequeño posible para satisfacer el contador de complejidad. ¿Qué tal el siguiente 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);
    }
}

Si estoy contando correctamente, la función extraída debe tener una complejidad de 9, que aún supera los requisitos. Y es básicamente el mismo código que antes , lo cual es algo bueno, ya que el código era bueno para empezar.

Además, los lectores de Clean Code pueden disfrutar el hecho de que el método de nivel superior ahora es simple y corto, mientras que el extraído se ocupa de los detalles.