java - significado - simplificar fracciones ejercicios

¿Cómo puedo simplificar este conjunto de sentencias if?(O, ¿qué lo hace sentir tan incómodo?)

Question

Mi colega me mostró este código, y ambos nos preguntamos por qué no podíamos eliminar el código duplicado.

private List<Foo> parseResponse(Response<ByteString> response) {
    if (response.status().code() != Status.OK.code() || !response.payload().isPresent()) {
      if (response.status().code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

Aquí hay una representación alternativa, para que sea un poco menos prolijo:

private void f() {
    if (S != 200 || !P) {
        if (S != 404 || !P) {
            Log();
        }
        return;
    }
    // ...
    // ...
    // ...
    return;
}

¿Hay alguna manera más simple de escribir esto, sin duplicar el !P ? Si no, ¿hay alguna propiedad única sobre la situación o las condiciones que hace que sea imposible factorizar el !P ?

Answer 1

Con este conjunto de condiciones, no creo que haya forma de evitar la duplicación. Sin embargo, prefiero mantener mis condiciones separadas tanto como sea razonable y duplicar otras áreas cuando sea necesario.

Si estuviera escribiendo esto, manteniendo el estilo actual, sería algo como esto:

    private void f() {
        if(!P) {
            Log();          // duplicating Log() & return but keeping conditions separate
            return;
        } else if (S != 200) {
            if (S != 404) {
                Log();
            }
            return;
        }

        // ...
        // ...
        // ...
        return;
    }

La simplicidad del código tiene algunos elementos subjetivos y la legibilidad es muy subjetiva. Dado que, si fuera a escribir este método desde cero, esto es lo que habría dado mis prejuicios.

private static final String ERR_TAG = "Cannot fetch recently played, got status code {}";

private List<Foo> parseResponse(Response<ByteString> response) {
    List<Foo> list = Lists.newArrayList();

    // similar to @JLRishe keep local variables rather than fetch a duplicate value multiple times
    Status status = response.status();
    int statusCode = status.code();
    boolean hasPayload = response.payload().isPresent();

    if(!hasPayload) {
        // If we have a major error that stomps on the rest of the party no matter
        //      anything else, take care of it 1st.
        LOG.error(ERR_TAG, status);
    } else if (statusCode == Status.OK.code()){
        // Now, let''s celebrate our successes early.
        // Especially in this case where success is narrowly defined (1 status code)
        // ...
        // ...
        // ...
        list = someOtherList;
    } else {
        // Now we''re just left with the normal, everyday failures.
        // Log them if we can
        if(statusCode != Status.NOT_FOUND.code()) {
            LOG.error(ERR_TAG, status);
        }
    }
    return list;        // One of my biases is trying to keep 1 return statement
                        // It was fairly easy here.
                        // I won''t jump through too many hoops to do it though.
}

Si elimino mis comentarios, esto casi duplica las líneas de código. Algunos argumentarían que esto no podría hacer que el código sea más simple. Para mí, lo hace.

Answer 2

Creo que lo siguiente es equivalente. Pero, como otros han señalado, la transparencia del código puede ser más importante que el código "simple".

if (not ({200,404}.contains(S) && P)){
    log();
    return;
}
if (S !=200){
    return;
}
// other stuff

Answer 3

El código que me huela es que está pidiendo el objeto Response en lugar de contarlo . Pregúntese por qué el método Parse es externo al objeto Response en lugar de ser un método (o, más probablemente, una superclase de él). ¿Se puede llamar al método Log () en el constructor del objeto Response en lugar de su método Parse? En el momento en que el status().code() las propiedades status().code() y payload().isPresent() se calculan en el constructor, sería posible asignar un objeto analizado por defecto a una propiedad privada de manera que solo un simple (y único) if ... else ... permanece en Parse ()?

Cuando uno es bendecido con poder escribir en un lenguaje orientado a objetos con herencia de implementación, se debe consultar cada enunciado (¡y expresión!) Condicional, para ver si es un candidato para elevarlo al constructor o al método (s). ) que invocan al constructor. La simplificación que puede seguir para todos los diseños de su clase es realmente inmensa.

Answer 4

En mi humilde opinión, el problema es principalmente la repetición y si anida. Otros han sugerido usar variables claras y funciones de utilidad que recomiendo también, pero también puede intentar separar las preocupaciones de sus validaciones.

Corrígeme si me equivoco, pero parece que tu código está intentando validar antes de procesar realmente la respuesta, así que aquí hay una forma alternativa de escribir tus validaciones:

private List<Foo> parseResponse(Response<ByteString> response)
{
    if (!response.payload.isPresent()) {
        LOG.error("Response payload not present");
        return Lists.newArrayList();
    }
    Status status = response.status();
    if (status != Status.OK || status != Status.NOT_FOUND) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

Answer 5

Es posible factorizar la repetida prueba P. El siguiente (pseudo código) es lógicamente equivalente al código en su pregunta.

private List<Foo> f() {
    List<Foo> list(); /*whatever construction*/
    if (P) {
        if (S==200) {
            // ...
            // ...
            // ...
            list.update(/*whatever*/);
        }
        else if (S!=404) {
           Log();
        }
    }
    else {
       Log();
    }
    return list;
}

En términos de legibilidad, iría con lo siguiente (nuevamente pseudo código):

private bool write_log() {
    return (S!=200 && S!=404) || !P
}
private bool is_good_response() {
    return S==200 && P
}
private List<Foo> f() {
    List<Foo> list(); /*whatever construction*/
    if (write_log()) {
       Log();
    }
    if (is_good_response()) {
        // ...
        // ...
        // ...
        list.update(/*whatever*/);
    }
    return list;
}

con quizás funciones más apropiadamente nombradas.

Answer 6

La parte más incómoda es que los getters como response.status() llamados muchas veces cuando la lógica parece requerir un único valor consistente. Es de suponer que funciona porque se garantiza que los captores siempre devuelven el mismo valor, pero expresa erróneamente la intención del código y lo hace más frágil a las suposiciones actuales.

Para solucionar esto, el código debería obtener response.status() una vez,

var responseStatus = response.status();

, entonces solo usa responseStatus después de eso. Esto debe repetirse para los otros valores getter que se supone que tienen el mismo valor en cada obtención.

Además, todas estas obtenciones idealmente se realizarían en el mismo punto secuencial si este código se pudiera refactorizar posteriormente en una implementación segura para subprocesos en un contexto más dinámico. Lo esencial es que pretenda obtener los valores de la response en un punto particular en el tiempo, por lo que la sección crítica del código debe obtener esos valores en un proceso sincrónico.

En general, especificar correctamente el flujo de datos previsto hace que el código sea más resistente y mantenible. Entonces, si alguien necesita agregar un efecto secundario a un captador o hacer que response un tipo de datos abstracto, será mucho más probable que continúe trabajando como se pretendía.

Answer 7

La ramificación en un entero al compararla con un conjunto finito de valores explícitos se maneja mejor con un switch :

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}

Log();
return A;

Answer 8

Las funciones de ayuda pueden simplificar los condicionales anidados.

private List<Foo> parseResponse(Response<ByteString> response) {
    if (!isGoodResponse(response)) {
        return handleBadResponse(response);
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

Answer 9

Lo principal que en realidad es redundante es la carga de! P (! Está presente). Si lo escribió como una expresión booleana, tiene:

(A || !P) && (B || !P)

Como has observado, el! P parece repetirse, y es innecesario. En el álgebra booleana puedes tratar Y como la multiplicación, y O más o menos como una suma. Entonces puedes ampliar esos paréntesis como con el álgebra simple en:

A && B || A && !P || !P && B || !P && !P

Puede agrupar todas las expresiones ANDed que tienen! P juntas:

A && B || (A && !P || !P && B || !P && !P)

Como todos esos términos tienen! P en ellos, puedes sacarlo como una multiplicación. Cuando lo haces, lo reemplazas con verdadero (como lo harías con 1, porque 1 vez todo es en sí mismo, verdadero Y cualquier cosa es ella misma):

A && B || !P && (A && true || B && true || true && true)

Observe que "true && true" es una de las expresiones OR, por lo que toda la agrupación siempre es verdadera, y puede simplificar a:

A && B || !P && true
-> A && B || !P

Estoy oxidado con la notación y la terminología adecuadas aquí, tal vez. Pero esa es la esencia de eso.

Volviendo al código, si tiene algunas expresiones complejas en una declaración if, como han notado otros, debería pegarlas en una variable significativa incluso si solo va a usarla una vez y descartarla.

Así que ponerlos juntos te da:

boolean statusIsGood = response.status().code() != Status.OK.code() 
  && response.status().code() != Status.NOT_FOUND.code();

if (!statusIsGood || !response.payload().isPresent()) {
  log();
}

Observe que la variable se llama "statusIsGood" aunque la negará. Porque las variables con nombres negados son realmente confusas.

Tenga en cuenta que puede hacer el tipo de simplificación anterior para una lógica realmente compleja, pero no siempre debe hacerlo . Terminará con expresiones que son técnicamente correctas, pero nadie puede decir por qué mirándolo.

En este caso, creo que la simplificación aclara la intención.

Answer 10

No estoy seguro de qué intenta hacer el código: honestamente, registrar solo el estado 404 y devolver una lista vacía cuando el código no es 200 parece que estás tratando de evitar una NPE ...

Creo que es mucho mejor algo así como:

private boolean isResponseValid(Response<ByteString> response){
    if(response == null){
        LOG.error("Invalid reponse!");
        return false;
    }

    if(response.status().code() != Status.OK.code()){
        LOG.error("Invalid status: {}", response.status());
        return false;
    }

    if(!response.payload().isPresent()){
        LOG.error("No payload found for response!");
        return false;
    }
    return true;
}

private List<Foo> parseResponse(Response<ByteString> response) throws InvalidResponseException{
    if(!isResponseValid(response)){
        throw InvalidResponseException("Response is not OK!");
    }

    // logic
}

Si por alguna razón la lógica if no se puede cambiar, de todos modos moveré la validación a una función separada.

Además, intente utilizar las convenciones de nomenclatura de Java:

LOG.error("")    // should be log.error("")
Status.OK.code() // why couldn''t it be a constant like Status.OK, or Status.getOkCode()?

Answer 11

Podría invertir la instrucción if para hacerlo más claro como en:

private void f() {
    if (S == 200 && P) {
        return;
    }

    if (S != 404 || !P) {
        Log();
    }

    return;
}

A continuación, puede refactorizar los condicionales utilizando nombres de métodos significativos como "responseIsValid ()" y "responseIsInvalid ()".

Answer 12

Recuerde que la concisión no siempre es la mejor solución y, en la mayoría de los casos, el compilador optimizará las variables nominales locales.

Probablemente usaría:

    boolean goodPayload = response.status().code() == Status.OK.code() && response.payload().isPresent();
    if (!goodPayload) {
        // Log it if payload not found error or no payload at all.
        boolean logIt = response.status().code() == Status.NOT_FOUND.code()
                || !response.payload().isPresent();
        if (logIt) {
            LOG.error("Cannot fetch recently played, got status code {}", response.status());
        }
        // Use empty.
        return Lists.newArrayList();
    }

Answer 13

Si desea aclarar las verificaciones condicionales y mantener el mismo resultado lógico, lo siguiente puede ser apropiado.

if (!P) {
    Log();
    return A;
}
if (S != 200) {
    if (S != 404) {
        Log();
    }
    return A;
}
return B;

O (Esto fue OP preferido)

if (S == 404 && P) {
    return A;
}
if (S != 200 || !P) {
    Log();
    return A;
}
return B;

O (yo personalmente prefiero esto, si no te importa el cambio)

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}
Log ();
return A;

Podría condensar el enunciado if quitando llaves y mover el cuerpo de líneas simples a la misma línea que el enunciado if. Los enunciados if de una línea, sin embargo, pueden ser una mala práctica confusa y desagradable. Por lo que deduzco de los comentarios, su preferencia sería en contra de este uso. Si bien los enunciados if de una sola línea pueden condensar la lógica y dar la apariencia de un código más limpio, la claridad y la intención del código deben valorarse con un código "económico". Para ser claros: personalmente creo que hay situaciones en las que las declaraciones if de una línea son apropiadas, sin embargo, dado que las condiciones originales son muy largas, recomendaría encarecidamente no hacerlo en este caso.

if (S != 200 || !P) {
    if (S != 404 || !P) Log();
    return A;
}
return B;

Como un nodo lateral: Si el Log(); declaración fue la única rama alcanzable en las declaraciones if anidadas, puede usar la siguiente identidad lógica para condensar la lógica ( Distributive ).

(S != 200 || !P) && (S! = 404 || !P) <=> (S != 200 && S != 404) || !P

EDITAR Edición significativa para reorganizar el contenido y resolver problemas mencionados en los comentarios.

Answer 14

Solo usa una variable como la respuesta de JLRishe. Pero yo diría que la claridad del código es mucho más importante que no duplicar una simple verificación booleana. Puede usar declaraciones de devoluciones anticipadas para que esto sea mucho más claro:

private List<Foo> parseResponse(Response<ByteString> response) {

    if (response.status().code() == Status.NOT_FOUND.code() && !response.payload().isPresent()) // valid state, return empty list
        return Lists.newArrayList();

    if (response.status().code() != Status.OK.code()) // status code says something went wrong
    {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
        return Lists.newArrayList();
    }

    if (!response.payload().isPresent()) // we have an OK status code, but no payload! should this even be possible?
    {
        LOG.error("Cannot fetch recently played, got status code {}, but payload is not present!", response.status());
        return Lists.newArrayList();
    }

    // ... got ok and payload! do stuff!

    return someOtherList;
}

Answer 15

Una razón por la que parece mucho código es porque es muy repetitivo. Use variables para almacenar las partes que se repiten, y eso ayudará con la legibilidad:

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    int code = status.code();
    boolean payloadAbsent = !response.payload().isPresent();

    if (code != Status.OK.code() || payloadAbsent) {
      if (code != Status.NOT_FOUND.code() || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

Editar: como señala Stewart en los comentarios a continuación, si es posible comparar response.status() y Status.OK directamente, entonces puede eliminar las llamadas .code() a .code() y usar la import static para acceder a los estados directamente:

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean payloadAbsent = !response.payload().isPresent();

    if (status != OK || payloadAbsent) {
      if (status!= NOT_FOUND || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

En cuanto a la cuestión de qué hacer con la duplicación de la lógica de payloadAbsent , Zachary me ha proporcionado algunas buenas ideas que son compatibles con lo que he sugerido. Una opción más es mantener la estructura básica, pero hacer que la razón para verificar la carga sea más explícita. Esto haría que la lógica sea más fácil de seguir y le ahorrará tener que usar || en el interior if . OTOH, yo no estoy muy interesado en este enfoque yo mismo:

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean failedRequest = status != OK;
    boolean loggableError = failedRequest && status!= NOT_FOUND ||
        !response.payload().isPresent();

    if (loggableError) {
      LOG.error("Cannot fetch recently played, got status code {}", status);
    }
    if (failedRequest || loggableError) {
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

Answer 16

puede tener un conjunto de código que desea registrar y la condición común de la carga útil no está presente

Set<Code> codes = {200, 404};
if(!codes.contains(S) && !P){
    log();
}
return new ArrayList<>();

Corrección en la condición.

Answer 17

Descargo de responsabilidad: No voy a cuestionar la firma de la función presentada, ni la funcionalidad.

Para mí, se siente incómodo, porque la función va mucho trabajo por sí misma, en lugar de delegarla.

En este caso, sugeriría elevar la parte de validación :

// Returns empty if valid, or a List if invalid.
private Optional<List<Foo>> validateResponse(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.of(Lists.newArrayList());
    }

    if (status.code() != Status.OK.code()) {
        return Optional.of(Lists.newArrayList());
    }

    return Optional.empty();
}

Tenga en cuenta que prefiero repetir la declaración de return lugar de anidar las condiciones. Esto mantiene el código plano , reduciendo la complejidad ciclomática. Además, no hay garantía de que siempre desee devolver el mismo resultado para todos los códigos de error.

Después, parseResponse vuelve fácil:

private List<Foo> parseResponse(Response<ByteString> response) {
    var error = validateResponse(response);
    if (error.isPresent()) {
        return error.get();
    }

    // ...
    // ...
    // ...
    return someOtherList;
}

Puede, en cambio, usar un estilo funcional.

/// Returns an instance of ... if valid.
private Optional<...> isValid(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.empty();
    }

    if (status.code() != Status.OK.code()) {
        return Optional.empty();
    }

    return Optional.of(...);
}

private List<Foo> parseResponse(Response<ByteString> response) {
    return isValid(response)
        .map((...) -> {
            // ...
            // ...
            // ...
            return someOtherList;
        })
        .orElse(Lists.newArrayList());
}

Aunque personalmente encuentro que anidar extra es un poco molesto.