Évaluation des courts-circuits, est-ce une mauvaise pratique?

88

Ce que je connais depuis longtemps, mais que je n’avais jamais considéré, c’est que dans la plupart des langues, il est possible de donner la priorité aux opérateurs dans une instruction if en fonction de leur ordre. J'utilise souvent cela comme moyen d'empêcher les exceptions de références nulles, par exemple:

if (smartphone != null && smartphone.GetSignal() > 50)
{
   // Do stuff
}

Dans ce cas, le code vérifiera d'abord que l'objet n'est pas nul, puis utilisera cet objet en sachant qu'il existe. Le langage est intelligent car il sait que si la première instruction est fausse, il ne sert à rien d’évaluer la seconde et une exception de référence nulle n’est donc jamais levée. Cela fonctionne de la même manière pour andet les oropérateurs.

Cela peut être utile dans d'autres situations, comme par exemple vérifier qu'un index se trouve dans les limites d'un tableau et qu'une telle technique peut être exécutée dans différents langages, à ma connaissance: Java, C #, C ++, Python et Matlab.

Ma question est la suivante: ce type de code est-il une représentation de la mauvaise pratique? Cette mauvaise pratique découle-t-elle d’un problème technique caché (c’est-à-dire que cela pourrait éventuellement conduire à une erreur) ou cela pose-t-il un problème de lisibilité pour les autres programmeurs? Pourrait-il être déroutant?

Innova
la source
69
Le terme technique est évaluation de court-circuit . Il s'agit d'une technique d'implémentation bien connue, et lorsque la norme de langage le garantit, il est judicieux de s'y fier. (Si ce n'est pas le cas, ce n'est pas vraiment une langue, OMI.)
Kilian Foth
3
La plupart des langues vous permettent de choisir le comportement que vous souhaitez. En C #, l'opérateur ||court-circuite, mais l'opérateur |(conduite simple) ne le fait pas ( &&et &se comporte de la même manière). Comme les opérateurs de court-circuit sont beaucoup plus fréquemment utilisés, je recommande de marquer les utilisations de celles qui ne le sont pas avec un commentaire expliquant pourquoi vous avez besoin de leur comportement (principalement des invocations de méthodes, par exemple).
Linac
14
@linac maintenant, mettre quelque chose du côté droit &ou |pour s'assurer qu'un effet secondaire se produise est quelque chose que je dirais, c'est certainement une mauvaise pratique: cela ne vous coûtera rien de mettre cette méthode à sa place.
Jon Hanna
14
@linac: En C et C ++, le &&court-circuit &n'est pas le cas, mais ce sont des opérateurs très différents. &&est un "et" logique; &est un bitwise "et". Il est possible x && yd'être vrai alors que x & yc'est faux.
Keith Thompson
3
Votre exemple spécifique peut être une mauvaise pratique pour une autre raison: que se passe-t-il s'il est nul? Est-il raisonnable que ce soit nul ici? Pourquoi est-ce nul? Le reste de la fonction en dehors de ce bloc doit-il être exécuté s'il est nul, le tout doit-il ne rien faire ou votre programme doit-il s'arrêter? Taper un "si nul" sur chaque accès (peut-être en raison d'une exception de référence nulle que vous êtes pressé de supprimer) signifie que vous pouvez aller assez loin dans le reste du programme, sans jamais vous rendre compte que l'initialisation de l'objet phone a été vissée up. Et finalement, quand vous
arriverez

Réponses:

125

Non, ce n'est pas une mauvaise pratique. Le recours au court-circuit des conditionnels est une technique largement acceptée et utile - tant que vous utilisez un langage garantissant ce comportement (qui inclut la grande majorité des langues modernes).

Votre exemple de code est assez clair et, en effet, c'est souvent la meilleure façon de l'écrire. Les alternatives (telles que les ifdéclarations imbriquées ) seraient plus compliquées, plus compliquées et donc plus difficiles à comprendre.

Quelques mises en garde:

Faites preuve de bon sens en ce qui concerne la complexité et la lisibilité

Ce qui suit n'est pas une si bonne idée:

if ((text_string != null && replace(text_string,"$")) || (text_string = get_new_string()) != null)

Comme beaucoup de méthodes "intelligentes" de réduction des lignes dans votre code, une confiance excessive dans cette technique peut générer un code difficile à comprendre et source d'erreurs.

Si vous devez gérer les erreurs, il existe souvent un meilleur moyen

Votre exemple est correct tant que l'état d'un programme normal pour un smartphone est nul. Si, toutefois, il s’agit d’une erreur, vous devez intégrer une gestion plus intelligente des erreurs.

Éviter les vérifications répétées du même état

Comme le souligne Neil, de nombreuses vérifications répétées de la même variable dans différentes conditions conditionnelles sont très probablement la preuve d'un défaut de conception. Si vous avez dix déclarations différentes parsemées dans votre code qui ne devrait pas être exécuté si smartphoneest null, se demander s'il y a une meilleure façon de gérer cette vérification que la variable à chaque fois.

Ce n’est pas vraiment un court-circuit spécifique; c'est un problème plus général de code répétitif. Toutefois, il convient de le mentionner, car il est assez courant de voir du code contenant de nombreuses instructions répétées, comme votre exemple.

Déduplicateur
la source
12
Il convient de noter que si les blocs vérifient si une instance est nulle dans une évaluation de court-circuit, elle devrait probablement être réécrite pour effectuer cette vérification une seule fois.
Neil
1
@ Neil, bon point; J'ai mis à jour la réponse.
2
La seule chose que je puisse penser à ajouter est de noter un autre moyen optionnel de le gérer, ce qui est vraiment préférable si vous voulez vérifier quelque chose dans plusieurs conditions différentes: vérifiez si quelque chose est nul dans un if (), puis return / throw - sinon continuez comme ça. Cela élimine l'imbrication et rend souvent le code plus explicite et plus concis, comme "cela ne doit pas être nul et si c'est le cas, je suis hors de portée" - placé aussi tôt que possible dans le code. Parfait lorsque vous codez quelque chose qui vérifie une condition spécifique plus qu’à un seul endroit dans une méthode.
BrianH
1
@ dan1111 Je généraliserais l'exemple que vous avez donné: "Une condition ne doit pas contenir d'effet secondaire (comme l'attribution de variable ci-dessus). L'évaluation d'un court-circuit ne change pas cela et ne doit pas être utilisée comme un raccourci effet qui aurait tout aussi bien pu être placé dans le corps du if pour mieux représenter la relation si / alors ".
AaronLS
@AaronLS, je suis tout à fait en désaccord avec l'affirmation selon laquelle "une condition ne devrait pas contenir d'effet secondaire". Une condition ne doit pas être source de confusion inutile, mais tant que le code est clair, je peux très bien provoquer un effet secondaire (ou même plus d'un effet secondaire) dans une condition.
26

Supposons que vous utilisiez un langage de style C sans &&et que vous deviez utiliser le code équivalent à celui de votre question.

Votre code serait:

if(smartphone != null)
{
  if(smartphone.GetSignal() > 50)
  {
    // Do stuff
  }
}

Ce modèle se présenterait beaucoup.

Maintenant, imaginez la version 2.0 de notre langage hypothétique &&. Pensez à quel point vous pensez que c'était cool!

&&est le moyen reconnu et idiomatique de faire ce que mon exemple ci-dessus fait. Ce n’est pas une mauvaise pratique de l’utiliser, c’est même une mauvaise pratique de ne pas l’utiliser dans les cas suivants: Un codeur expérimenté dans une telle langue se demanderait où se trouvait la elseou une autre raison de ne pas faire les choses de la manière habituelle.

Le langage est intelligent car il sait que si la première instruction est fausse, il ne sert à rien d’évaluer la seconde et une exception de référence nulle n’est donc jamais levée.

Non, vous êtes intelligent, car vous saviez que si la première déclaration était fausse, il serait alors inutile d'évaluer la deuxième déclaration. La langue est idiote comme une boîte de pierres et fait ce que vous lui avez dit de faire. (John McCarthy était encore plus intelligent et comprit que l'évaluation par court-circuit serait une chose utile pour les langues).

La différence entre votre intelligence et votre langage est importante, car bonne et mauvaise pratique revient souvent à être aussi intelligent que nécessaire, mais pas plus intelligent.

Considérer:

if(smartphone != null && smartphone.GetSignal() > ((range = (range != null ? range : GetRange()) != null && range.Valid ? range.MinSignal : 50)

Cela étend votre code pour vérifier un range. Si le rangeest null alors il essaie de le définir par un appel à GetRange()bien que cela puisse échouer alors rangepeut toujours être null. Si range n'est pas null après cela et que c'est Validalors que sa MinSignalpropriété est utilisée, sinon, la valeur par défaut 50est.

Cela dépend &&aussi, mais le mettre dans une ligne est probablement trop intelligent (je ne suis pas sûr de l'avoir bien compris, et je ne vais pas revérifier car cela montre ce que je veux dire).

Ce n’est pas &&le problème ici cependant, mais la possibilité de l’utiliser pour mettre beaucoup dans une expression (une bonne chose) augmente notre capacité à écrire des expressions difficiles à comprendre inutilement (une mauvaise chose).

Aussi:

if(smartphone != null && (smartphone.GetSignal() == 2 || smartphone.GetSignal() == 5 || smartphone.GetSignal() == 8 || smartPhone.GetSignal() == 34))
{
  // do something
}

Ici, je combine l'utilisation de &&avec un contrôle de certaines valeurs. Ce n'est pas réaliste dans le cas des signaux téléphoniques, mais cela se produit dans d'autres cas. Ici, c’est un exemple de mon manque d’intelligence. Si j'avais fait ce qui suit:

if(smartphone != null)
{
  switch (smartphone.GetSignal())
  {
    case 2: case 5: case 8: case 34:
      // Do something
      break;
  }
}

J'aurais gagné en lisibilité et en performance (les nombreux appels à GetSignal()n'auraient probablement pas été optimisés).

Ici encore, le problème ne consiste pas seulement &&à prendre ce marteau en particulier et à considérer tout le reste comme un clou; ne pas l'utiliser laisse-moi faire quelque chose de mieux que de l'utiliser.

Un dernier cas qui s'éloigne des meilleures pratiques est le suivant:

if(a && b)
{
  //do something
}

Par rapport à:

if(a & b)
{
  //do something
}

L'argument classique qui explique pourquoi nous pourrions favoriser cette dernière est qu'il existe un effet secondaire dans l'évaluation de ce bque nous voulons savoir si ac'est vrai ou non. Je ne suis pas d'accord sur ce point: si cet effet secondaire est si important, créez-le dans une ligne de code séparée.

Cependant, en termes d’efficacité, l’un ou l’autre des deux sera probablement meilleur. Le premier exécutera évidemment moins de code (pas d'évaluation bdu tout dans un chemin de code), ce qui peut nous faire économiser le temps nécessaire à l'évaluation b.

Le premier a aussi une branche supplémentaire. Considérez si nous le réécrivons dans notre hypothétique langage de style C sans no &&:

if(a)
{
  if(b)
  {
    // Do something
  }
}

Cet extra ifest caché dans notre utilisation de &&, mais il est toujours là. Et en tant que tel, il s’agit d’un cas où la prédiction de branche est en cours et potentiellement, par conséquent, de mauvaise prédiction de branche.

Pour cette raison, le if(a & b)code peut être plus efficace dans certains cas.

Ici, je dirais que if(a && b)c’est toujours la meilleure approche pratique pour commencer: Plus commune, la seule viable dans certains cas (où berreur se produira si elle aest fausse) et plus rapidement qu’autrement. Il vaut la peine de noter que cette if(a & b)optimisation est souvent utile dans certains cas.

Jon Hanna
la source
1
Si on a des tryméthodes qui reviennent trueen cas de succès, à quoi penses-tu if (TryThis || TryThat) { success } else { failure }? C'est un langage moins commun, mais je ne sais pas comment faire la même chose sans duplication de code ni ajout d'un indicateur. Bien que la mémoire requise pour un indicateur ne soit pas un problème, du point de vue de l'analyse, l'ajout d'un indicateur divise effectivement le code en deux univers parallèles - l'un contenant des instructions accessibles lorsque l'indicateur est trueet l'autre accessibles lorsqu'il est false. Parfois bon d'un point de vue SEC, mais dégénéré.
Supercat
5
Je ne vois rien de mal avec if(TryThis || TryThat).
Jon Hanna
3
Damn bien répondre, monsieur.
évêque
FWIW, d’excellents programmeurs, notamment Kernighan, Plauger & Pike de Bell Labs, recommandent, par souci de clarté, d’utiliser des structures de contrôle peu profondes, if {} else if {} else if {} else {}plutôt que des structures de contrôle imbriquées if { if {} else {} } else {}, même s’il faut évaluer la même expression plusieurs fois. Linus Torvalds a dit quelque chose de similaire: "Si vous avez besoin de plus de 3 niveaux d’indentation, vous êtes quand même foutu". Prenons cela comme un vote pour les opérateurs de court-circuit.
Sam Watkins
if (a && b) statement; est raisonnablement facile à remplacer. if (a && b && c && d) statement1; else statement2; est une vraie douleur.
gnasher729
10

Vous pouvez aller plus loin et, dans certaines langues, c'est la façon idiomatique de faire les choses: vous pouvez également utiliser l'évaluation de court-circuit en dehors des instructions conditionnelles, qui deviennent elles-mêmes une forme d'instructions conditionnelles. Par exemple, en Perl, il est idiomatique que les fonctions renvoient quelque chose de faux en cas d’échec (et quelque chose de vrai en cas de succès), et quelque chose comme:

open($handle, "<", "filename.txt") or die "Couldn't open file!";

est idiomatique. openretourne quelque chose de différent de zéro en cas de succès (pour que la diepartie suivante orne se produise jamais), mais indéfinie en cas d'échec, puis l'appel à survient die(c'est le moyen utilisé par Perl pour déclencher une exception).

C'est bien dans les langues où tout le monde le fait.

RemcoGerlich
la source
17
La plus grande contribution de Perl à la conception de langage consiste à fournir de nombreux exemples montrant comment ne pas le faire.
Jack Aidley
@JackAidley: Perl me manque unless, et postfix conditionals ( send_mail($address) if defined $address) cependant.
RemcoGerlich
6
@JackAidley pourriez-vous indiquer pourquoi "ou mourir" est mauvais? Est-ce juste une façon naïve de gérer les exceptions?
Bigstones
Il est possible de faire plein de choses terribles en Perl, mais je ne vois pas le problème avec cet exemple. C'est simple, court et clair - c'est exactement ce que vous devez attendre de la gestion des erreurs dans un langage de script.
1
@RemcoGerlich Ruby a hérité d'une partie de ce style:send_mail(address) if address
bonh,
6

Bien que je sois généralement d'accord avec la réponse de dan1111 , il existe un cas particulièrement important qu'il ne couvre pas: le cas où il smartphoneest utilisé simultanément. Dans ce scénario, ce type de modèle est une source bien connue de bogues difficiles à trouver.

Le problème est que l'évaluation des courts-circuits n'est pas atomique. Votre thread peut vérifier, c’est-à smartphone- nulldire qu’un autre thread peut entrer et l’annuler, puis votre thread tente rapidement d’ GetSignal()exploser et explose.

Mais bien sûr, cela n’est possible que lorsque les étoiles s’alignent et que le rythme de vos discussions est juste .

Ainsi, bien que ce type de code soit très courant et utile, il est important de connaître cette mise en garde afin que vous puissiez empêcher ce méchant bogue avec précision.

Telastyn
la source
3

Il existe de nombreuses situations dans lesquelles je souhaite vérifier une condition en premier et ne souhaite contrôler qu'une deuxième condition si la première condition a réussi. Parfois uniquement à des fins d'efficacité (car il est inutile de vérifier la deuxième condition si la première a déjà échoué), parfois parce que sinon mon programme planterait (votre vérification de NULL en premier), parfois parce que sinon les résultats seraient terribles. (SI (je veux imprimer un document) ET (l’impression d’un document a échoué)) ALORS l’affichage d’un message d’erreur - vous ne souhaitez pas imprimer le document et vérifiez si l’échec de l’impression a échoué si vous ne souhaitez pas imprimer un document dans le menu déroulant. première place!

Il y avait donc un énorme besoin d'évaluation de court-circuit garantie d'expressions logiques. Il n’était pas présent chez Pascal (qui avait une évaluation de court-circuit non garantie), ce qui était une douleur majeure; Je l'ai d'abord vu dans Ada et C.

Si vous pensez vraiment que c'est une "mauvaise pratique", prenez s'il vous plaît n'importe quel morceau de code moyennement complexe, réécrivez-le afin qu'il fonctionne correctement sans évaluation du court-circuit et revenez nous dire comment vous l'aimiez. C'est comme si on disait que respirer produit du dioxyde de carbone et se demander si respirer est une mauvaise pratique. Essayez sans pendant quelques minutes.

gnasher729
la source
"Réécris-le pour qu'il fonctionne correctement sans évaluation de court-circuit, et reviens nous dire comment tu l'as aimé." - Oui, cela ramène des souvenirs de Pascal. Et non, je n'ai pas aimé.
Thomas Padron-McCarthy
2

Une considération, non mentionnée dans les autres réponses: parfois, ces vérifications peuvent laisser supposer un refactoring possible du modèle de conception Null Object . Par exemple:

if (currentUser && currentUser.isAdministrator()) 
  doSomething();

Pourrait être simplifié pour être juste:

if (currentUser.isAdministrator())
  doSomething ();

Si currentUserest défini par défaut sur un "utilisateur anonyme" ou un "utilisateur nul" avec une implémentation de secours si l'utilisateur n'est pas connecté.

Pas toujours une odeur de code, mais quelque chose à considérer.

Pete
la source
-4

Je vais être impopulaire et dire oui, c'est une mauvaise pratique. SI la fonctionnalité de votre code repose sur celui-ci pour implémenter une logique conditionnelle.

c'est à dire

 if(quickLogicA && slowLogicB) {doSomething();}

c'est bien, mais

if(logicA && doSomething()) {andAlsoDoSomethingElse();}

est mauvais, et sûrement personne ne serait d'accord avec?!

if(doSomething() || somethingElseIfItFailed() && anotherThingIfEitherWorked() & andAlwaysThisThing())
{/* do nothing */}

Raisonnement:

Votre code est erroné si les deux côtés sont évalués plutôt que de ne pas exécuter la logique. Il a été avancé que ce n’était pas un problème, l’opérateur 'et' étant défini en c #, le sens est clair. Cependant, je soutiens que ce n'est pas vrai.

Raison 1. Historique

Vous utilisez essentiellement (ce qui était initialement prévu) une optimisation de compilateur pour fournir des fonctionnalités dans votre code.

Bien qu'en c #, la spécification du langage garantisse le court-circuit des opérateurs booléen 'et'. Ce n'est pas le cas de toutes les langues et de tous les compilateurs.

wikipeda répertorie diverses langues et leur approche du court-circuit http://en.wikipedia.org/wiki/Short-circuit_evaluation car il existe de nombreuses approches. Et quelques problèmes supplémentaires dans lesquels je n'entre pas ici.

En particulier, FORTRAN, Pascal et Delphi ont des drapeaux de compilateur qui permettent de basculer ce comportement.

Par conséquent: vous constaterez que votre code fonctionne lors de la compilation, mais pas pour le débogage ou avec un compilateur de remplacement, etc.

Raison 2. Différences de langue

Comme vous pouvez le constater sur wikipedia, toutes les langues n’adoptent pas la même approche du court-circuit, même quand elles spécifient comment les opérateurs booléens doivent le gérer.

En particulier, l'opérateur 'et' de VB.Net ne court-circuite pas et TSQL présente certaines particularités.

Etant donné qu'une "solution" moderne est susceptible d'inclure un certain nombre de langages, tels que javascript, regex, xpath, etc., vous devez en tenir compte lorsque vous clarifiez les fonctionnalités de votre code.

Raison 3. Différences au sein d'une langue

Par exemple, VB en ligne si se comporte différemment de son if. Encore une fois, dans les applications modernes, votre code peut se déplacer entre, par exemple, le code en arrière et un formulaire Web en ligne, vous devez être conscient des différences de signification.

Raison 4. Votre exemple spécifique de vérification de null le paramètre d'une fonction

C'est un très bon exemple. En cela, c’est quelque chose que tout le monde fait, mais c’est en fait une mauvaise pratique à y penser.

Il est très courant de voir ce type de vérification car cela réduit considérablement vos lignes de code. Je doute que quiconque puisse en parler lors d'une révision du code. (et évidemment d'après les commentaires et notes que vous pouvez voir c'est populaire!)

Cependant, cela ne tient pas compte des meilleures pratiques pour plus d'une raison!

  1. Son très clair de nombreuses sources, que les meilleures pratiques est de vérifier vos paramètres de validité avant de les utiliser et de lancer une exception si elles ne sont pas valides. Votre extrait de code ne montre pas le code environnant, mais vous devriez probablement faire quelque chose comme:

    if (smartphone == null)
    {
        //throw an exception
    }
    // perform functionality
    
  2. Vous appelez une fonction à partir de l'instruction conditionnelle. Bien que le nom de votre fonction implique qu'il calcule simplement une valeur, il peut masquer les effets secondaires. par exemple

    Smartphone.GetSignal() { this.signal++; return this.signal; }
    
    else if (smartphone.GetSignal() < 1)
    {
        // Do stuff 
    }
    else if (smartphone.GetSignal() < 2)
    {
        // Do stuff 
    }
    

    la meilleure pratique suggérerait:

    var s = smartphone.GetSignal();
    if(s < 50)
    {
        //do something
    }
    

Si vous appliquez ces meilleures pratiques à votre code, vous pouvez constater que votre possibilité de réaliser un code plus court grâce à l'utilisation de la condition cachée disparaît. La meilleure pratique est de faire quelque chose , de gérer le cas où le smartphone est nul.

Je pense que vous constaterez que c'est également le cas pour d'autres usages courants. Vous devez vous rappeler que nous avons aussi:

conditionnels en ligne

var s = smartphone!=null ? smartphone.GetSignal() : 0;

et coalescence nulle

var s = smartphoneConnectionStatus ?? "No Connection";

qui fournissent des fonctionnalités similaires de longueur de code court avec plus de clarté

nombreux liens pour supporter tous mes points:

https://stackoverflow.com/questions/1158614/what-is-the-best-practice-in-case-one-argument-is-null

Validation des paramètres de constructeur en C # - Meilleures pratiques

Faut-il vérifier la valeur null s'il ne s'attend pas à la valeur null?

https://msdn.microsoft.com/en-us/library/seyhszts%28v=vs.110%29.aspx

https://stackoverflow.com/questions/1445867/why-would-a-language-not-use-short-circuit-evaluation

http://orcharddojo.net/orchard-resources/Library/DevelopmentGuidelines/BestPractices/CSharp

exemples de drapeaux de compilation qui affectent les courts-circuits:

Fortran: "sce | nosce Par défaut, le compilateur évalue les courts-circuits dans certaines expressions logiques à l'aide de règles XL Fortran. La spécification de sce permet au compilateur d'utiliser des règles Fortran non-XL. Le compilateur procédera à une évaluation des courts-circuits si les règles actuelles le permettent. La valeur par défaut est nosce. "

Pascal: "B - Une directive d'indicateur qui contrôle l'évaluation des courts-circuits. Voir Ordre d'évaluation des opérandes d'opérateurs dyadiques pour plus d'informations sur l'évaluation des courts-circuits. Voir aussi l'option de compilation -sc du compilateur en ligne de commande pour plus d'informations ( documenté dans le manuel d'utilisation). "

Delphi: "Delphi prend en charge l'évaluation des courts-circuits par défaut. Elle peut être désactivée à l'aide de la directive de compilation {$ BOOLEVAL OFF}."

Ewan
la source
Les commentaires ne sont pas pour une discussion prolongée; cette conversation a été déplacée pour discuter .
Ingénieur mondial