Comment puis-je simplifier cet ensemble d'instructions if? (Ou, qu'est-ce qui le rend si inconfortable?)

Question

Mon collègue m'a montré ce code et nous nous sommes demandés pourquoi nous ne semblions pas pouvoir supprimer le code dupliqué.

 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();
    }
    // ...
    // ...
    // ...
    doSomeLogic();
    // ...
    // ...
    // ...
    return someOtherList;
}
 

Voici une représentation alternative, pour le rendre un peu moins verbeux:

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

Existe-t-il un moyen plus simple d’écrire ceci sans dupliquer les !P ? Sinon, y a-t-il une propriété unique dans la situation ou les conditions qui empêche de factoriser les !P ?

Answer 1

Une raison pour laquelle il ressemble à beaucoup de code, c'est qu'il est très répétitif. Utiliser des variables pour stocker les pièces qui sont répétés, et qui va aider à la lisibilité:

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

Edit: Stewart points dans les commentaires ci-dessous, si il est possible de comparer response.status() et Status.OK directement, vous pouvez éliminer le superflu des appels d' .code() et l'utilisation import static pour accéder aux statuts directement:

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

Quant à la question de savoir quoi faire avec la duplication de l' payloadAbsent logique, Zachary a fourni quelques bonnes idées qui sont compatibles avec ce que j'ai suggéré. Une autre option est de garder la structure de base, mais la raison pour la vérification de la charge utile plus explicite. Ce serait la logique facile à suivre et vous éviter d'avoir à utiliser || dans le centre - if. Otoh, que, je ne suis pas très friand de cette approche de moi:

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 2

Si vous voulais clarifier la condition des contrôles et de maintenir la même logique, les éléments suivants peuvent être appropriées.

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

Ou (Ce qui était OP de préférence)

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

Ou (personnellement, je préfère cela, si vous n'avez pas l'esprit de l'interrupteur)

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

Vous pourrait condenser le if en supprimant les accolades et le déplacement de l'unique corps doublé sur la même ligne que le if. Une seule ligne si-états, cependant, peut être source de confusion et de droits de mauvaise pratique. De ce que je comprends de ces observations, votre préférence irait à l'encontre de cette utilisation. Tout en une seule ligne si-états peut se condenser logique et donner l'apparence de nettoyeur de code, la clarté et le code d'intention doivent être évalués de "économiques" de code. Pour être clair: personnellement, je crois qu'il y a des situations où une seule ligne si les relevés sont appropriées, toutefois, que les conditions sont très longues, je conseille vivement contre cette dans ce cas.

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

Comme un côté d'un nœud: Si l' Log(); déclaration a été le seul accessible branche dans le imbriqués si-états, vous pouvez utiliser la suite logique de l'identité afin de condenser la logique (Distributive).

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

EDIT Significative de la modifier pour réorganiser le contenu et de résoudre les problèmes mentionnés dans les commentaires.

Answer 3

N'oubliez pas que la concision n'est pas toujours la meilleure solution et que, dans la plupart des cas, les variables nommées locales seront optimisées par le compilateur.

J'utiliserais probablement:

     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 4

L' odeur de code pour moi, c'est que vous êtes en demandant de l'objet de la Réponse au lieu de dire ça. Demandez-vous pourquoi la méthode d'analyse est extérieur à l'objet de la Réponse, au lieu d'être une méthode de celui-ci (ou, plus probablement, d'une super-classe de celle-ci). Pouvez la méthode Log() peut-être être appelé dans la Réponse du constructeur d'objet au lieu de sa méthode d'analyse? Au moment où les propriétés status().code() et payload().isPresent() sont calculés dans le constructeur serait-il possible d'affecter une valeur par défaut analysé objet d'une propriété privée telle que seule une simple (et unique) if ... else ... reste dans Parse()?

Quand on est béni avec la possibilité d'écrire dans un langage orienté objet avec la mise en œuvre de l'héritage, chaque instruction conditionnelle (et de l'expression!) doit être interrogé, à voir si c'est un candidat pour être introduits soit le constructeur ou les méthode(s) qui invoque le constructeur. La simplification qui peut suivre pour tous vos conceptions de classe est véritablement immense.

Answer 5

La principale chose qui est en fait redondante est le !P (!la charge utile est présent). Si vous l'avez écrit comme une expression booléenne que vous avez:

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

Comme vous l'avez observé, l' !P semble être répété, et c'est inutilement. Dans l'algèbre booléenne que vous pouvez traiter ET le type de comme la multiplication, OU en quelque sorte comme l'addition. De sorte que vous pouvez étendre ces parenthèses, tout comme avec les simples de l'algèbre dans:

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

Vous pouvez regrouper tous les ANDed expressions qui ont !P ensemble:

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

Depuis tous ces termes ont !P en eux, vous pouvez le prendre comme la multiplication. Lorsque vous le faites, vous le remplacez par de vrai (comme vous le feriez de 1, parce que 1 fois tout est en lui-même, vrai ET tout est en lui-même):

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

Notez que le "true && true" est l'un des l'un OU logique des expressions, de sorte que l'ensemble du groupement est toujours vrai, et vous pouvez simplifier à:

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

Je suis rouillé sur la bonne notation et la terminologie ici, peut-être. Mais c'est l'essentiel.

De retour pour le code, si vous avez quelques expressions complexes dans une instruction if, comme d'autres l'ont noté, vous devriez conserver dans une variable significative, même si vous êtes juste de l'utiliser une fois et de le jeter au loin.

Donc, en mettant ceux qui, ensemble, vous obtenez:

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

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

Avis de la variable est nommée "statusIsGood" même si vous aurez le nier. Parce que les variables avec nié les noms sont vraiment déroutant.

Gardez à l'esprit, vous pouvez le faire ci-dessus forme de simplification pour vraiment logique complexe, mais vous ne devriez pas toujours le faire. Vous vous retrouverez avec des expressions qui sont techniquement correct, mais personne ne peut dire pourquoi en la regardant.

Dans ce cas, je pense que la simplification précise l'intention.