Comment puis-je éditer une chaîne d'instructions if-else if pour adhérer aux principes de code propre d'Oncle Bob?

45

J'essaie de suivre les suggestions de code propre d'Oncle Bob et en particulier de garder les méthodes courtes.

Je me trouve cependant incapable de raccourcir cette logique:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

Je ne peux pas enlever les elses et donc séparer le tout en petits morceaux, provoquer le "else" dans le "else si" aide à la performance - évaluer ces conditions coûte cher et si je peux éviter d'évaluer les conditions ci-dessous, provoquer l'un des premiers est vrai, je veux les éviter.

Même sémantiquement parlant, évaluer la prochaine condition si la précédente était remplie n’a pas de sens du point de vue commercial.


edit: Cette question a été identifiée comme un doublon possible de façons élégantes de traiter si (sinon) autre chose .

Je pense que la question est différente (vous pouvez le constater également en comparant les réponses de ces questions).

Ev0oD
la source
46
Qu'est-ce qui est réellement faux ou incertain à propos de ce code dans son contexte? Je ne vois pas comment cela pourrait éventuellement être raccourci ou simplifié! Le code qui évalue les conditions apparaît déjà bien factorisé, tout comme la méthode appelée à la suite de la décision. Il suffit de regarder certaines des réponses ci-dessous, qui ne font que compliquer le code!
Steve
38
Il n'y a rien de mal avec ce code. C'est très lisible et facile à suivre. Tout ce que vous ferez pour le réduire davantage ajoutera une indirection et le rendra plus difficile à comprendre.
17 du 26
20
Votre code est bon. Mettez votre énergie restante dans quelque chose de plus productif que d'essayer de la raccourcir davantage.
Robert Harvey
5
Si c'est vraiment juste 4 conditions, c'est bon. Si c'est vraiment quelque chose comme 12 ou 50 alors vous voudrez probablement refactoriser à un niveau supérieur à celui de cette méthode.
JimmyJames
9
Laissez votre code exactement tel qu'il est. Écoutez ce que vos parents vous ont toujours dit: ne faites pas confiance aux oncles qui offrent des friandises aux enfants de la rue. @Harvey Assez drôle, les différentes tentatives "d'amélioration" du code l'ont rendu beaucoup plus volumineux, plus compliqué et moins lisible.
gnasher729

Réponses:

81

Idéalement, je pense que vous devriez extraire votre logique pour obtenir le code / numéro d'alerte dans sa propre méthode. Donc, votre code existant est réduit jusqu’à

{
    addAlert(GetConditionCode());
}

et GetConditionCode () encapsule la logique pour vérifier les conditions. Peut-être aussi mieux d'utiliser un Enum qu'un nombre magique.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}
Steven Eccles
la source
2
Si possible d'encapsuler comme vous le décrivez (je suppose que ce n'est peut-être pas le cas, je pense que OP omet des variables pour des raisons de simplicité), cela ne change pas le code, ce qui en soi va bien, mais ajoute de l'ergonomie au code et un peu de lisibilité + 1
quand le
17
Avec ces codes d’alerte, je remercie un seul code qui peut être renvoyé à la fois
Josh Part
12
Cela semble également être une correspondance parfaite pour l’utilisation d’une instruction switch, si celle-ci est disponible dans la langue de OP.
Frank Hopkins
4
Ce n'est probablement qu'une bonne idée d'extraire le code d'erreur d'une nouvelle méthode si cela peut être écrit de manière à être utile dans de multiples situations sans avoir à fournir une foule d'informations sur la situation particulière. En fait, il y a un compromis et un seuil de rentabilité quand cela en vaut la peine. Mais vous constaterez assez souvent que la séquence de validations est spécifique au travail en cours, et il est préférable de les garder ensemble avec ce travail. Dans de tels cas, inventer un nouveau type pour indiquer à une autre partie du code ce qu’il faut faire est un lest indésirable.
PJTraill
6
L'un des problèmes de cette réimplémentation est qu'elle oblige la fonction addAlertà vérifier la condition d'alerte fictive AlertCode.None.
David Hammen
69

La mesure importante est la complexité du code, pas la taille absolue. En supposant que les différentes conditions ne sont en réalité que des appels à une seule fonction, tout comme les actions ne sont pas plus complexes que ce que vous avez montré, je dirais que le code ne présente aucun défaut. C'est déjà aussi simple que possible.

Toute tentative de simplification supplémentaire compliquera les choses.

Bien sûr, vous pouvez remplacer le elsemot - clé par un mot returncomme d'autres l'ont suggéré, mais il ne s'agit que d'une question de style, pas de changement de complexité.


De côté:

Mon conseil général serait de ne jamais devenir religieux avec une règle de code propre: la plupart des conseils de codage que vous voyez sur Internet sont bons s'ils sont appliqués dans un contexte approprié, mais l'application radicale de ces mêmes conseils partout peut vous faire gagner une entrée l' IOCCC . L'astuce consiste toujours à trouver un équilibre qui permette aux êtres humains de raisonner facilement sur votre code.

Utilisez trop de méthodes, et vous êtes foutus. Utilisez des fonctions trop petites et vous êtes foutus. Évitez les expressions ternaires, et vous êtes foutu. Utilisez des expressions ternaires partout et vous êtes foutus. Réalisez qu'il existe des endroits qui appellent des fonctions sur une ligne et des endroits qui appellent des fonctions à 50 lignes (oui, ils existent!). Sachez qu'il y a des endroits qui appellent des if()déclarations et des endroits qui appellent des ?:opérateurs. Utilisez tout l'arsenal à votre disposition et essayez de toujours utiliser l'outil le plus adapté que vous puissiez trouver. Et rappelez-vous, ne devenez pas religieux même à propos de ce conseil.

cmaster
la source
2
Je dirais que le remplacement else ifpar un caractère interne returnsuivi d'un simple if(suppression du else) pourrait rendre le code plus difficile à lire . Lorsque le code dit else if, je sais immédiatement que le code du bloc suivant ne sera exécuté que si le code précédent ne l’a pas été. Pas de muss, pas de chichi. S'il est une plaine ifalors il pourrait exécuter ou il ne pourrait pas, indépendamment du fait que la précédente exécution. Maintenant, je vais devoir faire un effort mental pour analyser le bloc précédent et constater qu'il se termine par un return. Je préférerais consacrer cet effort mental à analyser la logique métier.
un CVn
1
Je sais, c'est une petite chose, mais au moins pour moi, else ifforme une unité sémantique. (Ce n'est pas nécessairement une seule unité au compilateur, mais c'est correct.) ...; return; } if (...N'a pas; encore moins si elle est répartie sur plusieurs lignes. C'est quelque chose que je devrai regarder pour voir ce qu'il fait, au lieu de pouvoir le prendre directement en regardant simplement la paire de mots-clés else if.
un CVn
@ MichaelKjörling Full Ack.Je préférerais moi-même cette else ifconstruction, d’autant plus que sa forme chaînée est un modèle bien connu. Cependant, le code de la forme if(...) return ...;est également un modèle bien connu, je ne le condamnerais donc pas complètement. Je considère toutefois qu’il s’agit là d’un problème mineur: la logique du flux de contrôle est la même dans les deux cas, et un simple coup d’œil plus minutieux sur une if(...) { ...; return; }échelle me dira que c’est effectivement équivalent à une else iféchelle. Je vois la structure d'un seul terme, j'en déduis le sens, je me rends compte qu'il se répète partout et je sais ce qui se passe.
cmaster
En venant de JavaScript / Node.js, certains utiliser le code « ceinture et bretelles » de l' utilisation à la fois else if et return . par exempleelse if(...) { return alert();}
utilisateur949300
1
"Et souviens-toi, ne deviens pas religieux même à propos de ce conseil." +1
mots comme Jared
22

Il est controversé de savoir si cela est «meilleur» que la plaine, si ..else pour un cas donné Mais si vous voulez essayer autre chose, c'est une façon courante de le faire.

Mettez vos conditions dans des objets et mettez ces objets dans une liste

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

Si plusieurs actions sont requises à condition que vous puissiez faire une récursion folle

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

Évidemment oui. Cela ne fonctionne que si vous avez un modèle dans votre logique. Si vous essayez d'effectuer une action conditionnelle récursive super générique, la configuration de l'objet sera aussi compliquée que l'instruction if d'origine. Vous allez inventer votre propre nouveau langage / framework.

Mais votre exemple ne un modèle

Un cas d'utilisation courant pour ce modèle serait la validation. Au lieu de :

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

Devient

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}
Ewan
la source
27
Cela ne fait que déplacer l’ if...elseéchelle dans la construction de la Conditionsliste. Le gain net est négatif, car la construction de Conditionsprendra autant de code que le code OP, mais le caractère indirectionnel ajouté a un coût en lisibilité. Je préférerais définitivement une échelle propre.
cmaster
3
@ cmaster oui, je pense avoir dit exactement cela ", alors la configuration de l'objet sera aussi compliquée que l'originale si statement £
Ewan
7
C'est moins lisible que l'original. Afin de déterminer quelle condition est actuellement vérifiée, vous devez aller chercher dans un autre domaine du code. Cela ajoute un niveau inutile d'indirection qui rend le code plus difficile à comprendre.
17 du 26
8
La conversion d'une chaîne if .. else if .. else .. en une table de prédicats et d'actions est logique, mais uniquement pour des exemples beaucoup plus vastes. Le tableau ajoute un peu de complexité et d'indirection, vous avez donc besoin de suffisamment d'entrées pour amortir cette surcharge de conception. Donc, pour 4 paires prédicats / actions, conservez le code original simple, mais si vous aviez 100, allez définitivement avec la table. Le point de croisement est quelque part entre les deux. @cmaster, la table peut être initialisée de manière statique, de sorte que la surcharge supplémentaire liée à l'ajout d'une paire prédicat / action correspond à une ligne qui les nomme simplement: difficile de faire mieux.
Stephen C. Steel
2
La lisibilité n'est pas personnelle. C'est un devoir envers le public de la programmation. C'est subjectif. C'est précisément pourquoi il est important de venir dans des endroits comme celui-ci et d'écouter ce que le public en dit. Personnellement, je trouve cet exemple incomplet. Montrez-moi comment conditionsest construit ... ARG! Pas d'annotation-attributs! Pourquoi Dieu? Ow mes yeux!
candied_orange
7

Pensez à utiliser return;après une condition a réussi, il vous sauve tous les elses. Vous pourriez même pouvoir return addAlert(1)directement si cette méthode a une valeur de retour.

Kilian Foth
la source
3
Bien sûr, cela suppose que rien d’autre ne se passe après la chaîne de ifs ... C’est peut-être une hypothèse raisonnable, mais ce n’est peut-être pas le cas.
un CVn
5

J'ai parfois vu des constructions comme celle-ci considérée comme plus propre:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

Ternary avec un bon espacement peut également être une alternative intéressante:

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

J'imagine que vous pourriez également essayer de créer un tableau avec une paire contenant une condition et une fonction et l'itérer jusqu'à ce que la première condition soit remplie - ce qui, à mon avis, équivaudrait à la première réponse d'Ewan.

Zworek
la source
1
ternaire est soignée
Ewan
6
@Ewan déboguer un «ternaire profondément récursif» cassé peut être une douleur inutile.
dfri
5
il a l' air bien sur l'écran cependant.
Ewan
Uhm, quel langage permet d'utiliser des fonctions avec des caseétiquettes?
Undercat soutient Monica le
1
@undercat c'est un code ECMAScript / JavaScript valide après l'achat de
zworek
1

Comme variante de la réponse de @ Ewan, vous pouvez créer une chaîne (au lieu d'une "liste simple") de conditions telles que:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

De cette façon, vous pouvez appliquer vos conditions dans un ordre défini et l'infrastructure (la classe abstraite affichée) ignore les vérifications restantes une fois la première rencontrée.

C’est là où il est supérieur à l’approche "liste plate" dans laquelle vous devez implémenter le "saut" dans la boucle qui applique les conditions.

Vous configurez simplement la chaîne de conditions:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

Et commencez l'évaluation avec un simple appel:

c1.alertOrPropagate(display);
Timothy Truckle
la source
Oui, c'est ce qu'on appelle le modèle de chaîne de responsabilité
Max
4
Je ne ferai pas semblant de parler au nom de quelqu'un d'autre, mais si le code de la question est immédiatement lisible et manifeste dans son comportement, je ne considérerais pas que cela soit immédiatement évident pour ce qu'il fait.
un CVn
0

Tout d'abord, le code d'origine n'est pas terrible OMI. C'est assez compréhensible et il n'y a rien de mauvais en soi.

Ensuite, si vous n’aimez pas cela, développez l’idée de @ Ewan d’utiliser une liste en supprimant son foreach breakmotif quelque peu anormal :

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

Adaptez maintenant ceci dans la langue de votre choix, faites de chaque élément de la liste un objet, un tuple, peu importe, et vous serez doué.

EDIT: il semble que ce ne soit pas aussi clair que je le pensais, alors laissez-moi vous expliquer davantage. conditionsest une liste ordonnée de quelque sorte; headest l’élément en cours d’investigation - au début c’est le premier élément de la liste, et chaque fois next()qu’on l’appelle, il devient le suivant; check()et alert()sont le checkConditionX()et addAlert(X)de l'OP.

Nico
la source
1
(Je n'ai pas voté mais) Je ne peux pas suivre cela. Quelle est la tête ?
Belle-Sophie
@Belle j'ai édité la réponse pour expliquer plus loin. C'est la même idée que celle d'Ewan mais avec un while notau lieu de foreach break.
Nico
Une brillante évolution d'une idée brillante
Ewan
0

La question manque de quelques détails. Si les conditions sont:

  • sujet à changement ou
  • répété dans d'autres parties de l'application ou du système ou
  • modifié dans certains cas (tels que différentes versions, tests, déploiements)

ou si le contenu de addAlertest plus compliqué, alors une meilleure solution, par exemple, de c # serait:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

Les tuples ne sont pas si beaux en c # <8, mais choisis pour leur convenance.

Les avantages de cette méthode, même si aucune des options ci-dessus ne s’appliquent, est que la structure est typée de manière statique. Vous ne pouvez pas accidentellement bousiller par, par exemple, manquer un else.

NiklasJ
la source
0

Le meilleur moyen de réduire la complexité de Cyclomatic dans les cas où vous en avez beaucoup if->then statementsest d'utiliser un dictionnaire ou une liste (dépendant de la langue) pour stocker la valeur de clé (si valeur d'instruction ou une valeur de), puis un résultat valeur / fonction.

Par exemple, au lieu de (C #):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

Je peux simplement

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

Si vous utilisez des langues plus modernes, vous pouvez stocker plus de logique que simplement des valeurs (c #). C’est vraiment juste des fonctions en ligne, mais vous pouvez aussi simplement indiquer d’autres fonctions si la logique est de les mettre en ligne.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}
Erik Philips
la source
0

J'essaie de suivre les suggestions de code propre d'Oncle Bob et en particulier de garder les méthodes courtes.

Je me trouve cependant incapable de raccourcir cette logique:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

Votre code est déjà trop court, mais la logique elle-même ne devrait pas être modifiée. À première vue, il semble que vous vous répétiez avec quatre appels à checkCondition(), et il est évident que chacun est différent après avoir relu attentivement le code. Vous devez ajouter un formatage approprié et des noms de fonction, par exemple:

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

Votre code doit être lisible avant tout. Ayant lu plusieurs livres de mon oncle Bob, je crois que c’est le message qu’il cherche constamment à faire passer.

CJ Dennis
la source
0

En supposant que toutes les fonctions soient implémentées dans le même composant, vous pouvez les conserver dans un état afin de vous débarrasser des multiples branches du flux.

EG: checkCondition1()deviendrait evaluateCondition1(), sur lequel il vérifierait si la condition précédente était remplie; si tel est le cas, il met en cache une valeur à récupérer getConditionNumber().

checkCondition2()deviendrait evaluateCondition2(), sur lequel il vérifierait si les conditions précédentes étaient remplies. Si la condition précédente n'était pas remplie, il vérifie le scénario de condition 2, mettant en cache une valeur à récupérer getConditionNumber(). Etc.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

MODIFIER:

Voici comment la vérification des conditions coûteuses devrait être mise en œuvre pour que cette approche fonctionne.

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

Par conséquent, si vous avez trop de contrôles coûteux à effectuer et que les éléments de ce code restent confidentiels, cette approche permet de le maintenir, ce qui permet de modifier l'ordre des contrôles si nécessaire.

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

Cette réponse fournit simplement quelques suggestions alternatives des autres réponses, et ne sera probablement pas meilleure que le code d'origine si nous considérons seulement 4 lignes de code. Bien que ce ne soit pas une approche terrible (et ne rend pas la maintenance plus difficile, comme d’autres l'ont dit) compte tenu du scénario que j'ai mentionné (trop de contrôles, seule fonction principale exposée en tant que public, toutes les fonctions sont des détails de mise en œuvre de la même classe).

Emerson Cardoso
la source
Je n’aime pas cette suggestion car elle cache la logique de test dans plusieurs fonctions. Cela peut rendre le code difficile à maintenir si, par exemple, vous deviez modifier l'ordre et faire le n ° 3 avant le n ° 2.
Lawrence
Non. Vous pouvez vérifier si une condition antérieure a été évaluée si anyCondition() != false.
Emerson Cardoso
1
Ok, je vois où tu veux en venir. Toutefois, si (par exemple) les conditions 2 et 3 sont évaluées à true, le PO ne veut pas que la condition 3 soit évaluée.
Lawrence
Ce que je voulais dire, c'est que vous pouvez vérifier anyCondition() != falsedans les fonctions evaluateConditionXX(). Ceci est possible à mettre en œuvre. Si l’approche consistant à utiliser l’état interne n’est pas souhaitée, je comprends, mais l’argument selon lequel cela ne fonctionne pas n’est pas valable.
Emerson Cardoso
1
Oui, mon objection est que cela cache inutilement la logique de test, pas que cela ne fonctionne pas. Dans votre réponse (paragraphe 3), le contrôle de la condition de réunion 1 est placé dans eval ... 2 (). Mais s'il change les conditions 1 et 2 au niveau supérieur (en raison de changements dans les exigences du client, etc.), vous devrez passer à eval ... 2 () pour supprimer la vérification de la condition 1, puis accéder à eval. ..1 () pour ajouter une vérification pour la condition 2. Cela peut fonctionner, mais cela peut facilement entraîner des problèmes de maintenance.
Lawrence
0

Pas plus de deux clauses "else" obligent le lecteur du code à parcourir l'intégralité de la chaîne pour trouver celle qui présente un intérêt. Utilisez une méthode telle que: void AlertUponCondition (Condition condition) {switch (condition) {case Condition.Con1: ... break; case Condition.Con2: ... pause; etc ...} Où "Condition" est une énumération appropriée. Si nécessaire, renvoyez une valeur booléenne ou une valeur. Appelez comme ça: AlertOnCondition (GetCondition ());

Cela ne peut vraiment pas être plus simple ET c'est plus rapide que la chaîne if-else une fois que vous avez dépassé quelques cas.

Jimmy T
la source
0

Je ne peux pas parler de votre situation particulière car le code n'est pas spécifique, mais ...

Un code comme celui-ci est souvent une odeur de modèle OO manquant. Vous avez vraiment quatre types de choses, chacune associée à son propre type d’avertisseur, mais au lieu de reconnaître ces entités et de créer une instance de classe pour chacune d’elles, vous les traitez comme une seule chose et essayez de les rattraper plus tard, à un moment vraiment propice. besoin de savoir à quoi vous avez affaire pour pouvoir procéder.

Le polymorphisme pourrait vous convenir mieux.

Méfiez-vous du code avec les méthodes longues contenant des constructions if ou then longues ou complexes. Vous voulez souvent un arbre de classes avec des méthodes virtuelles.

Martin Maat
la source