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

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?

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.

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.

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.

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.

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.

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.

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}."