Cette utilisation des conditionnels est-elle un anti-modèle?

J'ai beaucoup vu cela dans notre système hérité au travail - des fonctions qui ressemblent à ceci:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

En d'autres termes, la fonction comporte deux parties. La première partie fait une sorte de traitement (contenant potentiellement des boucles, des effets secondaires, etc.), et en cours de route, elle pourrait définir le drapeau "todo". La deuxième partie n'est exécutée que si le drapeau "todo" a été défini.

Cela semble être une façon assez moche de faire les choses, et je pense que la plupart des cas que j'ai pris le temps de comprendre pourraient être refactorisés pour éviter d'utiliser le drapeau. Mais est-ce un véritable anti-modèle, une mauvaise idée ou parfaitement acceptable?

La première refactorisation évidente serait de la découper en deux méthodes. Cependant, ma question est plus de savoir s'il y a jamais un besoin (dans un langage OO moderne) de créer une variable de drapeau local, en la définissant potentiellement à plusieurs endroits, puis en l'utilisant plus tard pour décider d'exécuter ou non le prochain bloc de code.

Je ne connais pas l'anti-pattern, mais j'en extraire trois méthodes.

Le premier effectuerait un travail et retournerait une valeur booléenne.

Le second effectuerait tout travail effectué par "un autre code"

Le troisième effectuerait le travail auxiliaire si le booléen retourné était vrai.

Les méthodes extraites seraient probablement privées s'il était important que la seconde seulement (et toujours) soit appelée si la première méthode renvoyait true.

En nommant bien les méthodes, j'espère que cela rendra le code plus clair.

Quelque chose comme ça:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

De toute évidence, un débat doit avoir lieu pour savoir si les retours anticipés sont acceptables, mais il s'agit d'un détail de mise en œuvre (tout comme la norme de formatage du code).

Le fait est que l'intention du code devient plus claire, ce qui est bien ...

Un des commentaires sur la question suggère que ce modèle représente une odeur , et je suis d'accord avec cela. Il vaut la peine de l'examiner pour voir si vous pouvez rendre l'intention plus claire.

Je pense que la laideur est due au fait qu'il y a beaucoup de code dans une seule méthode, et / ou les variables sont mal nommées (qui sont toutes deux des odeurs de code à part entière - les antipatterns sont des choses plus abstraites et complexes IMO).

Donc, si vous extrayez la plupart du code dans des méthodes de niveau inférieur comme le suggère @Bill, le reste devient propre (du moins pour moi). Par exemple

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Ou vous pouvez même vous débarrasser complètement du drapeau local en masquant le contrôle du drapeau dans la deuxième méthode et en utilisant un formulaire comme

calculateTaxRefund(isTaxRefundable(...), ...)

Dans l'ensemble, je ne considère pas qu'une variable de drapeau local soit nécessairement mauvaise en soi. Laquelle des options ci-dessus est plus lisible (= préférable pour moi) dépend du nombre de paramètres de méthode, des noms choisis et de la forme la plus cohérente avec la logique interne du code.

todo est un très mauvais nom pour la variable, mais je pense que c'est peut-être tout ce qui ne va pas. Il est difficile d'être entièrement sûr sans le contexte.

Disons que la deuxième partie de la fonction trie une liste, construite par la première partie. Cela devrait être beaucoup plus lisible:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Cependant, la suggestion de Bill est également correcte. C'est encore plus lisible:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Le modèle de machine d'état me semble bien. Les anti-patterns sont "todo" (mauvais nom) et "lots de code".

Ça dépend vraiment. Si le code protégé par todo(j'espère que vous n'utilisez pas ce nom pour de vrai car il est totalement non mnémonique!) Est un code de nettoyage conceptuel, alors vous avez un anti-modèle et devriez utiliser quelque chose comme RAII ou C # de C ++ usingconstruire pour gérer les choses à la place.

D'un autre côté, s'il ne s'agit pas conceptuellement d' une étape de nettoyage, mais plutôt d'un traitement supplémentaire qui est parfois nécessaire et où la décision de le faire doit être prise plus tôt, ce qui est écrit est bien. Demandez-vous si des morceaux de code individuels seraient mieux refactorisés dans leurs propres fonctions, et si vous avez capturé la signification de la variable indicateur dans son nom, mais ce modèle de code de base est OK. En particulier, essayer d'en mettre trop dans d'autres fonctions pourrait rendre ce qui se passe moins clair, et ce serait certainement un anti-modèle.

Beaucoup de réponses ici auraient du mal à passer une vérification de complexité, quelques-unes semblaient> 10.

Je pense que c'est la partie "anti-modèle" de ce que vous regardez. Trouvez un outil qui mesure la complexité cyclomatique de votre code - il existe des plugins pour éclipse. Il s'agit essentiellement d'une mesure de la difficulté de votre code à tester et implique le nombre et les niveaux de branches de code.

En guise de supposition totale sur une solution possible, la disposition de votre code me fait penser dans "Tâches", si cela se produit dans beaucoup d'endroits, peut-être que ce que vous voulez vraiment est une architecture orientée tâche - chaque tâche étant la sienne objet et au milieu de la tâche, vous avez la possibilité de mettre en file d'attente la tâche suivante en instanciant un autre objet de tâche et en le jetant dans la file d'attente. Ceux-ci peuvent être incroyablement simples à configurer et ils réduisent considérablement la complexité de certains types de code - mais comme je l'ai dit, c'est un coup de couteau dans le noir.

En utilisant l'exemple de pdr ci-dessus, car c'est un bel exemple, je vais aller plus loin.

Il avait:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

J'ai donc réalisé que ce qui suit fonctionnerait:

if(BuildList(list)) 
    SortList(list)

Mais ce n'est pas aussi clair.

Donc à la question d'origine, pourquoi ne pas avoir:

BuildList(list)
SortList(list)

Et laisser SortList décider s'il nécessite un tri?

Vous voyez que votre méthode BuildList semble connaître le tri, car elle renvoie un bool indiquant en tant que tel, mais cela n'a aucun sens pour une méthode conçue pour créer une liste.

Oui, cela semble être un problème car vous devez continuer à suivre tous les endroits où vous marquez le drapeau ON / OFF. Il est préférable d'inclure la logique juste à l'intérieur comme une condition imbriquée au lieu de retirer la logique.

De plus, les modèles de domaine riches, dans ce cas, un seul liner feront de grandes choses à l'intérieur de l'objet.

Si le drapeau n'est défini qu'une seule fois, déplacez le code
...
directement après le
... // un autre code ici,
puis supprimez le drapeau.

Sinon, divisez le
... // beaucoup de code ici
... // un autre code ici
...
codez-le si possible en fonctions séparées, il est donc clair que cette fonction a une responsabilité qui est la logique de branchement.

Dans la mesure du possible, séparez le code dans
... // beaucoup de code ici
en deux ou plusieurs fonctions, certaines qui font du travail (qui est une commande) et d'autres qui retournent la valeur todo (qui est une requête) ou la font très explicite, ils le modifient (une requête utilisant des effets secondaires)

Le code lui-même n'est pas l'anti-modèle en cours ici ... Je soupçonne que le fait de mélanger la logique de branchement avec le fait de faire des choses (commandes) est l'anti-modèle que vous recherchez.

Parfois, je trouve que j'ai besoin de mettre en œuvre ce modèle. Parfois, vous souhaitez effectuer plusieurs vérifications avant de procéder à une opération. Pour des raisons d'efficacité, les calculs impliquant certains contrôles ne sont pas effectués à moins qu'il ne semble absolument nécessaire de vérifier. Donc, vous voyez généralement du code comme:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Cela pourrait être simplifié en séparant la validation du processus réel d'exécution de l'opération, de sorte que vous verriez plus comme:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Évidemment, cela varie en fonction de ce que vous essayez d'atteindre, bien qu'écrit comme ceci, à la fois votre code de «réussite» et votre code d '«échec» sont écrits une seule fois, ce qui simplifie la logique et maintient le même niveau de performances. À partir de là, ce serait une bonne idée d'intégrer des niveaux entiers de validation à l'intérieur de méthodes internes qui renvoient des indications booléennes de succès ou d'échec qui simplifient encore les choses, bien que certains programmeurs aiment les méthodes extrêmement longues pour une raison étrange.

Ce n'est pas un modèle . L'interprétation la plus générale est que vous définissez une variable booléenne et que vous ramifiez sa valeur ultérieurement. C'est une programmation procédurale normale, rien de plus.

Maintenant, votre exemple spécifique peut être réécrit comme suit:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Cela peut être plus facile à lire. Ou peut être pas. Cela dépend du reste du code que vous avez omis. Concentrez-vous sur la simplification de ce code.

Les drapeaux locaux utilisés pour le flux de contrôle doivent être reconnus comme une forme de gotodéguisement. Si un indicateur n'est utilisé que dans une fonction, il pourrait être éliminé en écrivant deux copies de la fonction, en étiquetant l'un comme "indicateur est vrai" et l'autre comme "indicateur est faux", et en remplaçant chaque opération qui définit l'indicateur quand c'est clair, ou efface quand c'est réglé, avec un saut entre les deux versions de la fonction.

Dans de nombreux cas, le code qui utilise l'aide d'un indicateur sera plus propre que toute alternative possible qui utilise à la gotoplace, mais ce n'est pas toujours vrai. Dans certains cas, utiliser gotopour sauter un morceau de code peut être plus propre que d'utiliser des drapeaux pour le faire [bien que certaines personnes puissent insérer un certain dessin animé de raptor ici].

Je pense que le principe directeur principal devrait être que le flux logique du programme devrait ressembler à la description de la logique métier dans la mesure du possible. Si les exigences de la logique métier sont décrites en termes d'états qui se divisent et fusionnent de manière étrange, la logique de programme peut également être plus propre que d'essayer d'utiliser des indicateurs pour masquer une telle logique. D'un autre côté, si la façon la plus naturelle de décrire les règles métier est de dire qu'une action doit être effectuée si certaines autres actions ont été effectuées, la manière la plus naturelle d'exprimer cela peut être d'utiliser un indicateur qui est défini lors de l'exécution ces dernières actions et est par ailleurs clair.