Est-ce une erreur d'utiliser un paramètre booléen pour déterminer le comportement?

J'ai vu de temps en temps une pratique qui "se sent" mal, mais je ne peux pas tout à fait dire ce qui ne va pas. Ou peut-être que c'est juste mes préjugés. Voici:

Un développeur définit une méthode avec un booléen comme l'un de ses paramètres, et cette méthode en appelle une autre, et ainsi de suite, et éventuellement ce booléen est utilisé uniquement pour déterminer si une action doit être prise ou non. Cela peut être utilisé, par exemple, pour autoriser l'action uniquement si l'utilisateur dispose de certains droits, ou si nous sommes (ou ne sommes pas) en mode test, en mode batch ou en mode réel, ou peut-être uniquement lorsque le système est en mode de test. certain état.

Eh bien, il y a toujours une autre façon de le faire, que ce soit en interrogeant le moment opportun (plutôt que de transmettre le paramètre), en ayant plusieurs versions de la méthode, ou plusieurs implémentations de la classe, etc. Ma question est: Pas tellement comment améliorer cela, mais plutôt si c'est vraiment faux (comme je le soupçonne), et si c'est le cas, qu'est-ce qui ne va pas à ce sujet?

Oui, il s'agit probablement d'une odeur de code, ce qui conduirait à un code difficile à comprendre, incompréhensible et qui a moins de chance d'être facilement réutilisé.

Comme d'autres affiches l'ont noté , tout est dans le contexte (ne vous précipitez pas s'il s'agit d'un événement isolé ou si la pratique a été reconnue comme une dette technique contractée délibérément pour être redéfinie ultérieurement), mais d'une manière générale si un paramètre est passé dans une fonction qui sélectionne un comportement spécifique à exécuter, un raffinement par étapes supplémentaire est nécessaire; Briser cette fonction en fonctions plus petites produira des fonctions plus cohérentes.

Alors, quelle est une fonction hautement cohésive?

C'est une fonction qui fait une chose et une seule chose.

Le problème avec un paramètre transmis, comme vous l'avez décrit, est que la fonction fait plus de deux choses. En fonction de l'état du paramètre booléen, il peut vérifier ou non les droits d'accès des utilisateurs. En fonction de cet arbre de décision, il exécute une fonctionnalité.

Il serait préférable de séparer les préoccupations du contrôle d'accès de celles de la tâche, de l'action ou du commandement.

Comme vous l'avez déjà noté, ces préoccupations semblent inextricablement liées.

Ainsi, la notion de cohésion nous aide à identifier que la fonction en question n’est pas très cohérente et que nous pourrions refactoriser le code pour produire un ensemble de fonctions plus cohérentes.

Donc, la question pourrait être reformulée; Étant donné que nous sommes tous d’accord pour dire qu’il est préférable d’éviter de passer des paramètres de sélection comportementale, comment améliorer la situation?

Je me débarrasserais complètement du paramètre. Le fait de pouvoir désactiver le contrôle d'accès même à des fins de test constitue un risque potentiel pour la sécurité. À des fins de test, remplacez ou simulez le contrôle d'accès pour tester les scénarios d'accès autorisé et d'accès refusé.

Réf: Cohésion (informatique)

J'ai arrêté d'utiliser ce modèle il y a longtemps, pour une raison très simple. coût de maintenance. Plusieurs fois, j’ai trouvé que j’avais une fonction frobnicate(something, forwards_flag)qui dit que celle-ci était appelée plusieurs fois dans mon code et devait localiser tous les emplacements du code où la valeur falseétait passée forwards_flag. Vous ne pouvez pas facilement les rechercher, alors cela devient un casse-tête de maintenance. Et si vous devez créer un correctif sur chacun de ces sites, vous risquez d’avoir un problème si vous en oubliez un.

Mais ce problème spécifique est facilement résolu sans changer fondamentalement l'approche:

enum FrobnicationDirection {
  FrobnicateForwards,
  FrobnicateBackwards;
};

void frobnicate(Object what, FrobnicationDirection direction);

Avec ce code, il suffirait de rechercher des instances de FrobnicateBackwards. Bien qu'il soit possible qu'un code attribue cette variable à une variable, vous devez donc suivre quelques threads de contrôle, mais en pratique, je trouve que c'est assez rare pour que cette alternative fonctionne correctement.

Cependant, le fait de passer le drapeau de cette manière pose un autre problème, du moins en principe. Ceci est dû au fait que certains (seulement certains) systèmes ayant cette conception exposent peut-être trop d'informations sur les détails d'implémentation des parties profondément imbriquées du code (qui utilise l'indicateur) aux couches extérieures (qui ont besoin de savoir quelle valeur passer). dans ce drapeau). Pour utiliser la terminologie de Larry Constantine, cette conception peut avoir un couplage trop fort entre le passeur et l'utilisateur du drapeau booléen. Franky est difficile à dire avec certitude sur cette question sans en savoir plus sur la base de code.

Pour aborder les exemples spécifiques que vous donnez, je voudrais avoir un certain degré de préoccupation dans chacun d’eux, mais principalement pour des raisons de risque / exactitude. Autrement dit, si votre système doit transmettre des indicateurs indiquant l'état du système, il se peut que vous disposiez d'un code qui aurait dû en tenir compte, mais ne vérifie pas le paramètre (car il n'a pas été transmis à cette fonction). Donc vous avez un bug parce que quelqu'un a omis de passer le paramètre.

Il convient également d'admettre qu'un indicateur d'état du système qui doit être transmis à presque toutes les fonctions est en fait essentiellement une variable globale. La plupart des inconvénients d'une variable globale s'appliqueront. Je pense que dans de nombreux cas, il est préférable d'appliquer la connaissance de l'état du système (ou des informations d'identification de l'utilisateur, ou de l'identité du système) au sein d'un objet chargé d'agir correctement sur la base de ces données. Ensuite, vous transmettez une référence à cet objet par opposition aux données brutes. Le concept clé ici est l' encapsulation .

Ce n'est pas nécessairement faux mais cela peut représenter une odeur de code .

Le scénario de base à éviter en ce qui concerne les paramètres booléens est le suivant:

public void foo(boolean flag) {
    doThis();
    if (flag)
        doThat();
}

Ensuite, lorsque vous appelez, vous appelez généralement foo(false)et en foo(true)fonction du comportement exact que vous souhaitez.

C'est vraiment un problème parce que c'est un cas de mauvaise cohésion. Vous créez une dépendance entre les méthodes qui n'est pas vraiment nécessaire.

Ce que vous devriez faire dans ce cas, c'est de partir doThiset en doThattant que méthode séparée et publique:

doThis();
doThat();

ou

doThis();

De cette façon, vous laissez la décision correcte à l'appelant (exactement comme si vous passiez un paramètre booléen) sans créer de couplage.

Bien sûr, tous les paramètres booléens ne sont pas utilisés de manière aussi mauvaise, mais il s'agit certainement d'une odeur de code et vous avez raison de vous méfier si vous voyez cela beaucoup dans le code source.

Ceci est juste un exemple de la façon de résoudre ce problème basé sur les exemples que j'ai écrits. Il existe d'autres cas où une approche différente sera nécessaire.

Martin Fowler a publié un bon article dans lequel il explique en détail la même idée.

PS: si la méthode, fooau lieu d’appeler deux méthodes simples, implémentait une implémentation plus complexe, il vous suffisait d’appliquer une petite méthode d’extraction de refactoring pour que le code résultant ressemble à l’implémentation de foocelle que j’ai écrite.

Tout d'abord: la programmation n'est pas une science, mais un art. Il existe donc très rarement une "mauvaise" et une "bonne" façon de programmer. La plupart des normes de codage ne sont que des "préférences" que certains programmeurs jugent utiles; mais finalement ils sont plutôt arbitraires. Donc, je ne qualifierais jamais un choix de paramètre comme étant "faux" en soi - et certainement pas quelque chose d'aussi générique et utile qu'un paramètre booléen. L'utilisation d'un boolean(ou d' intailleurs) pour encapsuler un état est parfaitement justifiable dans de nombreux cas.

Dans l’ensemble, les décisions de codage devraient reposer principalement sur les performances et la maintenabilité. Si la performance n'est pas en jeu (et je ne peux pas imaginer comment cela pourrait être dans vos exemples), alors votre considération suivante devrait être: à quel point cela sera-t-il facile à maintenir pour moi (ou un futur rédacteur)? Est-ce intuitif et compréhensible? Est-ce isolé? Votre exemple d'appels de fonction chaînés semble en fait potentiellement fragile à cet égard: si vous décidez de changer votre bIsUpen bIsDown, combien d'autres endroits dans le code devront également être changés? De plus, votre liste de paramètres est-elle en train de gonfler? Si votre fonction comporte 17 paramètres, la lisibilité pose problème et vous devez vous demander si vous appréciez les avantages de l'architecture orientée objet.

Je pense que l' article de code propre de Robert C Martins stipule qu'il est préférable d'éliminer les arguments booléens dans la mesure du possible, car ils montrent qu'une méthode fait plus d'une chose. Une méthode devrait faire une chose et une seule chose, à mon avis, est l’une de ses devises.

Je pense que la chose la plus importante ici est d'être pratique.

Lorsque le booléen détermine l'intégralité du comportement, créez une deuxième méthode.

Lorsque le booléen détermine seulement un comportement au milieu, vous pouvez le garder dans un pour réduire la duplication de code. Dans la mesure du possible, vous pourriez même être en mesure de scinder la méthode en trois: deux méthodes d'appel pour l'une ou l'autre des options booléennes et une qui effectue la majeure partie du travail.

Par exemple:

private void FooInternal(bool flag)
{
  //do work
}

public void Foo1()
{
  FooInternal(true);
}

public void Foo2()
{
  FooInternal(false);
}

Bien entendu, en pratique, vous aurez toujours un point entre ces deux extrêmes. Habituellement, je me contente de ce qui me semble juste, mais je préfère pécher par excès de duplication de code.

J'aime l'approche consistant à personnaliser le comportement à l'aide de méthodes de générateur qui renvoient des instances immuables. Voici comment Guava l'Splitter utilise:

private static final Splitter MY_SPLITTER = Splitter.on(',')
       .trimResults()
       .omitEmptyStrings();

MY_SPLITTER.split("one,,,,  two,three");

Les avantages de ceci sont:

• Lisibilité supérieure
• Séparation claire des méthodes de configuration et d'action
• Favorise la cohésion en vous obligeant à réfléchir sur ce qu'est l'objet, ce qu'il devrait et ne devrait pas faire. Dans ce cas c'est un Splitter. Vous ne mettriez jamais someVaguelyStringRelatedOperation(List<Entity> myEntities)dans une classe appelée Splitter, mais vous envisageriez de la définir comme une méthode statique dans une StringUtilsclasse.
• Les instances sont pré-configurées, donc facilement injectables en dépendance. Le client n'a pas besoin de se demander s'il doit passer trueou falseà une méthode pour obtenir le comportement correct.

Certainement une odeur de code . S'il n'enfreint pas le principe de responsabilité unique, il enfreint probablement Tell, Don't Ask . Considérer:

• Pouvez-vous résumer ce que votre méthode fait sans utiliser de conjonctions (et, mais, etc.)?
• Existe-t-il des sous-parties de votre méthode qui pourraient facilement être regroupées dans une autre méthode plus petite ? (Cela inclut la création complexe d'une variable locale .)
• Si votre drapeau détermine la méthode à utiliser pour un paramètre, votre paramètre en fait-il trop ?
• Cette méthode appartient-elle à l'un de vos paramètres? (Vous remarquerez peut-être s'il sélectionne des variables d'instance dans votre paramètre à l'aide de getters / setters .)

S'il s'avère ne pas enfreindre l'un de ces deux principes, vous devriez toujours utiliser une énumération. Les drapeaux booléens sont l'équivalent booléen des nombres magiques . foo(false)fait autant de sens que bar(42). Les énumérations peuvent être utiles pour le modèle de stratégie et vous permettent d’ajouter une autre stratégie. (N'oubliez pas de les nommer correctement .)

Votre exemple particulier me dérange particulièrement. Pourquoi ce drapeau est-il passé par tant de méthodes? Cela ressemble à la nécessité de scinder votre paramètre en sous-classes .

TL; DR: n'utilisez pas d'arguments booléens.

Voir ci-dessous pourquoi ils sont mauvais et comment les remplacer (en caractères gras).

Les arguments booléens sont très difficiles à lire et donc difficiles à maintenir. Le problème principal est que l'objectif est généralement clair lorsque vous lisez la signature de la méthode où l'argument est nommé. Cependant, nommer un paramètre n'est généralement pas requis dans la plupart des langues. Ainsi, vous aurez des anti-modèles comme RSACryptoServiceProvider#encrypt(Byte[], Boolean)où le paramètre booléen détermine quel type de cryptage doit être utilisé dans la fonction.

Donc, vous recevrez un appel comme:

rsaProvider.encrypt(data, true);

où le lecteur doit rechercher la signature de la méthode simplement pour déterminer ce que l'enfer truepourrait réellement vouloir dire. Passer un entier est bien sûr tout aussi mauvais:

rsaProvider.encrypt(data, 1);

vous en dirait autant - ou plutôt: tout aussi peu. Même si vous définissez des constantes à utiliser pour l'entier, les utilisateurs de la fonction peuvent simplement les ignorer et continuer à utiliser des valeurs littérales.

La meilleure façon de résoudre ce problème consiste à utiliser une énumération . Si vous devez passer une énumération RSAPaddingavec deux valeurs: OAEPou PKCS1_V1_5alors vous pourrez immédiatement lire le code:

rsaProvider.encrypt(data, RSAPadding.OAEP);

Les booléens ne peuvent avoir que deux valeurs. Cela signifie que si vous avez une troisième option, vous devrez alors refactoriser votre signature. Généralement, cela ne peut pas être facilement réalisé si la compatibilité avec les versions antérieures pose un problème. Vous devez donc étendre toute classe publique avec une autre méthode publique. C'est ce que Microsoft a finalement fait quand ils ont introduit RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding)où ils utilisaient une énumération (ou au moins une classe imitant une énumération) au lieu d'un booléen.

Il peut même être plus facile d’utiliser un objet complet ou une interface en tant que paramètre, au cas où le paramètre lui-même aurait besoin d’être paramétré. Dans l'exemple ci-dessus, le rembourrage OAEP lui-même pourrait être paramétré avec la valeur de hachage à utiliser en interne. Notez qu’il existe maintenant 6 algorithmes de hachage SHA-2 et 4 algorithmes de hachage SHA-3. Par conséquent, le nombre de valeurs d’énumération peut exploser si vous utilisez une seule énumération plutôt que des paramètres (c’est peut-être la prochaine chose que Microsoft va découvrir. ).

Les paramètres booléens peuvent également indiquer que la méthode ou la classe n'est pas bien conçue. Comme dans l'exemple ci-dessus: aucune bibliothèque cryptographique autre que la librairie .NET n'utilise du tout d'indicateur de remplissage dans la signature de la méthode.

Presque tous les gourous du logiciel que j'aime mettent en garde contre les arguments booléens. Par exemple, Joshua Bloch les met en garde dans le livre très apprécié "Effective Java". En général, ils ne devraient tout simplement pas être utilisés. Vous pourriez faire valoir qu'ils pourraient être utilisés s'il y avait un paramètre facile à comprendre. Mais même dans ce cas: il Bit.set(boolean)est probablement préférable de mettre en œuvre deux méthodes : Bit.set()et Bit.unset().

Si vous ne pouvez pas refactoriser directement le code, vous pouvez définir des constantes pour au moins les rendre plus lisibles:

const boolean ENCRYPT = true;
const boolean DECRYPT = false;

...

cipher.init(key, ENCRYPT);

est beaucoup plus lisible que:

cipher.init(key, true);

même si vous préférez avoir:

cipher.initForEncryption(key);
cipher.initForDecryption(key);

au lieu.

Je suis surpris que personne n'ait mentionné les paramètres nommés .

Le problème que je vois avec les drapeaux booléens est qu'ils nuisent à la lisibilité. Par exemple, que fait-on truedans

myObject.UpdateTimestamps(true);

faire? Je n'ai aucune idée. Mais qu'en est-il:

myObject.UpdateTimestamps(saveChanges: true);

Maintenant, il est clair que le paramètre que nous passons est censé faire: nous disons à la fonction de sauvegarder ses modifications. Dans ce cas, si la classe n'est pas publique, le paramètre booléen est correct.

Bien sûr, vous ne pouvez pas forcer les utilisateurs de votre classe à utiliser des paramètres nommés. Pour cette raison, une enumou deux méthodes distinctes sont généralement préférables, selon la fréquence de votre cas par défaut. C'est exactement ce que fait .Net:

//Enum used
double a = Math.Round(b, MidpointRounding.AwayFromZero);

//Two separate methods used
IEnumerable<Stuff> ascendingList = c.OrderBy(o => o.Key);
IEnumerable<Stuff> descendingList = c.OrderByDescending(o => o.Key); 

Je ne peux pas vraiment expliquer ce qui ne va pas.

Si cela ressemble à une odeur de code, à une odeur de code et - bien - à une odeur de code, il s'agit probablement d'une odeur de code.

Ce que tu veux faire c'est:

1) Évitez les méthodes avec des effets secondaires.

2) Traitez les états nécessaires avec une machine à états formelle centrale (comme celle-ci ).

Je suis d'accord avec toutes les préoccupations liées à l'utilisation de paramètres booléens pour ne pas déterminer les performances afin de: améliorer, lisibilité, fiabilité, réduction de la complexité, réduction des risques d’encapsulation et de cohésion faibles et réduction du coût total de possession avec la maintenabilité.

J'ai commencé à concevoir du matériel au milieu des années 70, que nous appelons maintenant SCADA (contrôle de supervision et acquisition de données). Il s'agissait de matériel finement réglé avec code machine dans EPROM exécutant des télécommandes macro et collectant des données à haute vitesse.

La logique s'appelle machines Mealey & Moore , que nous appelons maintenant machines à états finis . Celles-ci doivent également être effectuées dans les mêmes règles que ci-dessus, à moins qu'il ne s'agisse d'une machine à temps réel avec un temps d'exécution fini et que des raccourcis doivent être utilisés pour atteindre cet objectif.

Les données sont synchrones, mais les commandes sont asynchrones et la logique de commande suit une logique booléenne sans mémoire, mais avec des commandes séquentielles basées sur la mémoire de l'état précédent, présent et souhaité. Pour que cela fonctionne dans le langage machine le plus efficace (seulement 64 Ko), un soin particulier a été apporté à la définition de chaque processus de manière heuristique IBM HIPO. Cela impliquait parfois de passer des variables booléennes et de créer des branches indexées.

Mais maintenant, avec beaucoup de mémoire et la facilité d’utilisation d’OOK, l’encapsulation est un ingrédient essentiel aujourd’hui, mais constitue une pénalité lorsque le code est compté en octets pour le code machine en temps réel et SCADA.

Ce n'est pas nécessairement faux, mais dans votre exemple concret de l'action dépendant de certains attributs de "l'utilisateur", je passerais par une référence à l'utilisateur plutôt que par un drapeau.

Cela clarifie et aide de plusieurs manières.

Toute personne lisant l'instruction appelante se rendra compte que le résultat changera en fonction de l'utilisateur.

Dans la fonction appelée en fin de compte, vous pouvez facilement implémenter des règles métier plus complexes car vous pouvez accéder à n’importe quel attribut utilisateur.

Si une fonction / méthode de la "chaîne" fait quelque chose de différent en fonction d'un attribut d'utilisateur, il est très probable qu'une dépendance similaire à l'égard d'attributs d'utilisateur soit introduite pour certaines des autres méthodes de la "chaîne".

La plupart du temps, je considérerais ce mauvais codage. Je peux cependant penser à deux cas où cela pourrait être une bonne pratique. Comme il y a beaucoup de réponses qui disent déjà pourquoi c'est mauvais, j'offre deux fois quand ça pourrait être bon:

Le premier est si chaque appel de la séquence a un sens en soi. Il serait logique si le code d' appel peut être modifié de vrai à faux ou faux vrai, ou si la méthode appelée peut être modifié pour utiliser le paramètre booléen directement plutôt que de passer sur. La probabilité de dix appels consécutifs est faible, mais cela pourrait arriver. Si c'était le cas, ce serait une bonne pratique en matière de programmation.

Le deuxième cas est un peu exagéré étant donné que nous avons affaire à un booléen. Mais si le programme a plusieurs threads ou événements, la transmission de paramètres est le moyen le plus simple de suivre les données spécifiques à un thread / événement. Par exemple, un programme peut recevoir les entrées de deux sockets ou plus. Le code en cours d'exécution pour un socket peut avoir besoin de générer des messages d'avertissement, et un autre en cours d'exécution peut ne pas l'être. Il est alors logique (en quelque sorte) qu'un ensemble booléen d'un niveau très élevé soit transmis via de nombreux appels de méthode aux emplacements où des messages d'avertissement pourraient être générés. Les données ne peuvent pas être sauvegardées (sauf très difficilement) de manière globale, car plusieurs threads ou événements entrelacés nécessiteraient chacun leur propre valeur.

Certes, dans ce dernier cas , je serais probablement créer une classe / structure dont le seul contenu était un booléen et passer que dans la place. Je serais presque certain d'avoir besoin d'autres champs avant trop longtemps - par exemple, où envoyer les messages d'avertissement.

Le contexte est important. De telles méthodes sont assez courantes dans iOS. UINavigationController fournit la méthode -pushViewController:animated:et le animatedparamètre est un BOOL. La méthode remplit essentiellement la même fonction dans les deux cas, mais elle anime la transition d'un contrôleur de vue à un autre si vous passez à OUI, et non à NO. Cela semble tout à fait raisonnable. il serait stupide de devoir utiliser deux méthodes à la place de celle-ci afin de pouvoir déterminer si vous souhaitez utiliser une animation.

Il peut être plus facile de justifier ce genre de chose dans Objective-C, où la syntaxe de nommage de méthode fournit plus de contexte pour chaque paramètre que dans des langages tels que C et Java. Néanmoins, je penserais qu'une méthode qui prend un seul paramètre pourrait facilement prendre un booléen et avoir encore un sens:

file.saveWithEncryption(false);    // is there any doubt about the meaning of 'false' here?