Si-sinon, une échelle censée remplir toutes les conditions - faut-il ajouter une clause finale redondante?

10

C'est une chose que je fais beaucoup ces derniers temps.

Exemple:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Souvent, l'échelle if / else est beaucoup plus compliquée que cela ...

Voir la clause finale? C'est redondant. L'échelle est censée finalement capturer toutes les conditions possibles. Ainsi, il pourrait être réécrit comme ça:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

C'est ainsi que j'écrivais du code, mais je n'aime pas ce style. Ma plainte est que la condition dans laquelle la dernière partie du code sera exécutée n'est pas évidente d'après le code. J'ai donc commencé à écrire explicitement cette condition pour la rendre plus évidente.

Toutefois:

  • L'écriture explicite de la condition exhaustive finale est ma propre idée, et j'ai de mauvaises expériences avec mes propres idées - généralement les gens me crient à quel point ce que je fais est horrible - et (parfois beaucoup) plus tard, je découvre que c'était effectivement le cas sous-optimal;
  • Une indication pourquoi cela peut être une mauvaise idée: non applicable à Javascript, mais dans d'autres langues, les compilateurs ont tendance à émettre des avertissements ou même des erreurs lorsque le contrôle atteint la fin de la fonction. Faire allusion à quelque chose comme ça pourrait ne pas être trop populaire ou je me trompe.
    • Les plaintes du compilateur m'ont parfois fait écrire la condition finale dans un commentaire, mais je suppose que cela est horrible car les commentaires, contrairement au code, n'ont aucun effet sur la sémantique réelle du programme:
    } else { // i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }

Suis-je en train de manquer quelque chose? Ou est-ce OK de faire ce que j'ai décrit ou est-ce une mauvaise idée?

gaazkam
la source
+1 pour l'excellente réponse de @ Christophe que la redondance du code augmente les risques de défauts. Il y a aussi un problème d'efficacité beaucoup moins important. Pour développer cela, en ce qui concerne l'exemple de code en question, nous pourrions écrire la série des if-else-if comme des ifs séparés sans aucun elses, mais nous ne le ferions généralement pas car les if-else donnent au lecteur la notion d'alternatives exclusives plutôt qu'une série de conditions indépendantes (ce qui serait également moins efficace, car cela dit que toutes les conditions devraient être évaluées même après un match).
Erik Eidt
@ErikEidt> car cela dit que toutes les conditions doivent être évaluées même après une correspondance à +1, sauf si vous revenez d'une fonction ou "coupez" d'une boucle (ce qui n'est pas le cas dans l'exemple ci-dessus).
Darek Nędza
1
+1, belle question. Soit dit en passant, il pourrait vous intéresser qu'en présence de NaN, l'exemple n'est pas exhaustif.
monocell

Réponses:

6

Les deux approches sont valides. Mais regardons de plus près les avantages et les inconvénients.

Pour une ifchaîne avec des conditions triviales comme ici, cela n'a pas vraiment d'importance:

  • avec une finale else, il est évident pour le lecteur de savoir dans quelle condition le reste est déclenché;
  • avec une finale else if, il est aussi évident pour le lecteur qu'aucun supplément elsen'est nécessaire puisque vous avez tout couvert.

Cependant, il existe de nombreuses ifchaînes qui reposent sur des conditions plus complexes, combinant des états de plusieurs variables, peut-être avec une expression logique complexe. Dans ce cas, c'est moins évident. Et voici la conséquence de chaque style:

  • final else: vous êtes sûr que l'une des branches est prise. S'il vous arrive d'oublier un cas, il passera par cette dernière branche, donc pendant le débogage, si la dernière branche a été choisie et que vous vous attendiez à autre chose, vous comprendrez rapidement.
  • final else if: vous devez dériver la condition redondante à coder, ce qui crée une source d'erreur potentielle avec le risque de ne pas couvrir tous les cas. De plus, si vous avez manqué un cas, rien ne sera exécuté et il pourrait être plus difficile de découvrir que quelque chose manquait (par exemple, si certaines variables que vous vous attendiez à définir conservent des valeurs des itérations précédentes).

La dernière condition redondante est donc une source de risque. C'est pourquoi je préfère proposer d'aller en finale else.

Edit: codage haute fiabilité

Si vous développez avec une grande fiabilité en tête, vous pourriez être intéressé par une autre variante: compléter votre finale explicite redondante else ifpar une finale elseafin de détecter toute situation inattendue.

Il s'agit d'un codage défensif. Il est recommandé par certaines spécifications de sécurité telles que SEI CERT ou MISRA . Certains outils d'analyse statique implémentent même cela comme une règle systématiquement vérifiée (cela pourrait expliquer les avertissements de votre compilateur).

Christophe
la source
7
Et si après la dernière condition de redondance j'ajoute un autre qui lève toujours une exception qui dit "Tu n'étais pas censé me joindre!" - Cela atténuera-t-il certains des problèmes de mon approche?
gaazkam
3
@gaazkam oui, c'est un style de codage très défensif. Vous devez donc toujours calculer la condition finale, mais pour les chaînes complexes sujettes aux erreurs, vous le découvrirez au moins rapidement. Le seul problème que je vois avec cette variante est que c'est un peu exagéré pour un cas évident.
Christophe
@gaazkam J'ai édité ma réponse pour répondre également à votre idée supplémentaire mentionnée dans votre commentaire
Christophe
1
@gaazkam Lancer une exception est une bonne chose. Si vous le faites, n'oubliez pas d'inclure la valeur inattendue dans le message. Il peut être difficile à reproduire et la valeur inattendue peut fournir un indice sur la source du problème.
Martin Maat
1
Une autre option est de mettre un asserten finale else if. Cependant, votre guide de style peut varier selon qu'il s'agit d'une bonne idée. (La condition d'un assertne devrait jamais être fausse, à moins que le programmeur ne se trompe. Donc, cela utilise au moins la fonctionnalité pour son usage prévu. Mais tellement de gens en abusent que beaucoup de magasins l'ont interdit.)
Kevin
5

Quelque chose qui manque jusqu'à présent dans les réponses est une question de savoir quel type d'échec est moins nocif.

Si votre logique est bonne, peu importe ce que vous faites, le cas important est ce qui se passe si vous avez un bug.

Vous omettez le conditionnel final: l'option finale s'exécute même si ce n'est pas la bonne chose à faire.

Vous ajoutez simplement le conditionnel final: il n'exécute aucune option, selon la situation, cela peut simplement signifier que quelque chose ne s'affiche pas (faible préjudice), ou cela peut signifier une exception de référence nulle à un moment ultérieur (ce qui pourrait être un débogage douleur.)

Vous ajoutez le conditionnel final et une exception: il lève.

Vous devez décider laquelle de ces options est la meilleure. Dans le code de développement, je considère cela comme une évidence - prenez le troisième cas. Cependant, je définirais probablement circle.src sur une image d'erreur et circle.alt sur un message d'erreur avant de lancer - au cas où quelqu'un déciderait de désactiver les assertions plus tard, cela échouerait sans danger.

Une autre chose à considérer - quelles sont vos options de récupération? Parfois, vous n'avez pas de chemin de récupération. Ce que je pense en est l'ultime exemple, c'est le premier lancement de la fusée Ariane V. Une erreur non capturée / 0 (en fait un débordement de division) a entraîné la destruction du booster. En réalité, le code qui s'est écrasé ne servait à rien à ce moment-là, il était devenu sans objet à l'instant où les boosters de ceinture s'allumaient. Une fois qu'ils ont allumé l'orbite ou le boom, vous faites de votre mieux, les erreurs ne peuvent pas être autorisées. (Si la fusée s'égare à cause de cela, le gars de la sécurité de la gamme tourne sa clé.)

Loren Pechtel
la source
4

Ce que je recommande, c'est d'utiliser une assertdéclaration dans votre autre final, dans l'un de ces deux styles:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else {
        assert i > current
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    }
}

Ou une assertion de code mort:

setCircle(circle, i, { current }) {
    if (i == current) {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
    } else if (i < current) {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
    } else if (i > current) {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
    } else {
        assert False, "Unreachable code"
    }
}

L'outil de couverture de code peut souvent être configuré pour ignorer le code tel que «affirmer faux» du rapport de couverture.


En plaçant la condition dans une assertion, vous documentez efficacement la condition d'une branche explicitement, mais contrairement à un commentaire, la condition d'assertion peut en fait être vérifiée et échouera si vous gardez les assertions activées pendant le développement ou en production (je recommande généralement de garder les assertions activées en production s'ils n'affectent pas trop les performances).

Lie Ryan
la source
1
Je n'aime pas votre première option, la seconde est beaucoup plus claire que c'est un cas qui devrait être impossible.
Loren Pechtel
0

J'ai défini une macro «affirmée» qui évalue une condition, et dans une version de débogage tombe dans le débogueur.

Donc, si je suis sûr à 100% qu'une des trois conditions doit être vraie, j'écris

If condition 1 ...
Else if condition 2 .,,
Else if asserted (condition3) ...

Cela montre assez clairement qu'une condition sera vraie et qu'aucune branche supplémentaire pour une assertion n'est nécessaire.

gnasher729
la source
-2

Je recommande d' éviter complètement le reste . Permet ifde déclarer ce que le bloc de code est censé gérer et de terminer le bloc en quittant la fonction.

Il en résulte un code très clair:

setCircle(circle, i, { current })
{
    if (i == current)
    {
        circle.src = 'images/25CE.svg'
        circle.alt = 'Now picking'
        return
    }
    if (i < current)
    {
        circle.src = 'images/25C9.svg'
        circle.alt = 'Pick failed'
        return
    }
    if (i > current)
    {
        circle.src = 'images/25CB.svg'
        circle.alt = 'Pick chance'
        return
    }
    throw new Exception("Condition not handled.");
}

La finale ifest bien sûr redondante ... aujourd'hui. Cela peut devenir très important si / quand un futur développeur réorganise les blocs. Il est donc utile de le laisser là-dedans.

John Wu
la source
1
C'est en fait très peu clair, car maintenant je dois me demander si l'exécution de la première branche pourrait changer le résultat du deuxième test. Plus des retours multiples inutiles. Plus la possibilité qu'aucune condition ne soit vraie.
gnasher729
J'écris en fait si des déclarations comme celle-ci, alors que je m'attends intentionnellement à ce que plusieurs cas puissent être pris. Il est particulièrement utile dans les langues avec des instructions switch qui ne permettent pas de passer. Je pense que si les choix s'excluent mutuellement, il devrait être écrit de manière à ce qu'il soit évident que les cas s'excluent mutuellement (en utilisant autre chose). Et si ce n'est pas écrit comme ça, alors l'implication est que les cas ne s'excluent pas mutuellement (une chute est possible).
Jerry Jeremiah
@ gnasher729 Je comprends parfaitement votre réaction. Je connais des gens très intelligents qui ont eu des problèmes avec "éviter autre chose" et "retour anticipé" ... au début. Je vous encourage à prendre un certain temps pour vous habituer à l'idée et à l'examiner - parce que ces idées réduisent objectivement et mesurablement la complexité. Le retour anticipé répond en fait à votre premier point (vous devez vous demander si une branche peut changer un autre test). Et la «possibilité qu'aucune condition ne soit vraie» existe toujours; avec ce style, vous obtenez une exception évidente plutôt qu'un défaut de logique caché.
John Wu
Mais la complexité cyclomatique des deux styles n'est-elle pas exactement la même? c'est-à-dire égal au nombre de conditions
gaazkam