Faut-il utiliser «else» dans les situations où le flux de contrôle le rend redondant?

18

Je tombe parfois sur du code similaire à l'exemple suivant (ce que fait exactement cette fonction sort du cadre de cette question):

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  else if (check2(value)) {
    return value;
  }
  else {
    return false;
  }
}

Comme vous pouvez le voir, if, else ifet les elsedéclarations sont utilisées conjointement avec la returndéclaration. Cela semble assez intuitif à un observateur occasionnel, mais je pense qu'il serait plus élégant (du point de vue d'un développeur de logiciels) de supprimer le else-s et de simplifier le code comme ceci:

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  if (check2(value)) {
    return value;
  }
  return false;
}

Cela a du sens, car tout ce qui suit une returninstruction (dans la même portée) ne sera jamais exécuté, ce qui rend le code ci-dessus sémantiquement égal au premier exemple.

Laquelle des réponses ci-dessus correspond le mieux aux bonnes pratiques de codage? Y a-t-il des inconvénients à l'une ou l'autre méthode en ce qui concerne la lisibilité du code?

Modifier: Une suggestion en double a été faite avec cette question fournie comme référence. Je crois que ma question touche à un sujet différent, car je ne demande pas à éviter les déclarations en double comme présenté dans l'autre question. Les deux questions visent à réduire les répétitions, quoique de manières légèrement différentes.

rhinocéros
la source
14
C'est elseun problème moindre, le plus gros problème est que vous renvoyez évidemment deux types de données à partir d'une seule fonction, ce qui rend l'imprévisibilité de l'API de la fonction.
Andy
3
E_CLEARLY_NOTADUPE
kojiro
3
@DavidPacker: au moins deux; pourrait être trois. -1est un nombre, falseest un booléen et valuen'est pas spécifié ici, il peut donc s'agir de n'importe quel type d'objet.
Yay295
1
@ Yay295 J'espérais que valuec'est en fait un entier. Si cela peut être quelque chose, c'est encore pire.
Andy
@DavidPacker C'est un point très important, surtout lorsqu'il est soulevé à propos d'une question sur les bonnes pratiques. Je suis d'accord avec vous et Yay295, mais les exemples de code servent d'exemple pour démontrer une technique de codage sans rapport. Néanmoins, je garde à l'esprit que c'est une question importante et ne doit pas être négligée dans le code réel.
rhino

Réponses:

23

J'aime celui sans elseet voici pourquoi:

function doSomething(value) {
  //if (check1(value)) {
  //  return -1;
  //}
  if (check2(value)) {
    return value;
  }
  return false;
}

Parce que faire ça n'a rien cassé ce qu'il n'était pas censé faire.

Détestez l'interdépendance sous toutes ses formes (y compris en nommant une fonction check2()). Isolez tout ce qui peut être isolé. Parfois, vous en avez besoin, elsemais je ne vois pas cela ici.

candied_orange
la source
Je préfère généralement cela aussi, pour la raison indiquée ici. Cependant, je place généralement un commentaire à la fin de chaque ifbloc de } //elsepour indiquer clairement, lors de la lecture du code, que la seule exécution prévue après le ifbloc de code est si la condition dans l' ifinstruction est fausse. Sans quelque chose comme ça, en particulier si le code dans le ifbloc devient plus complexe, il peut ne pas être clair pour quiconque gère le code que l'intention était de ne jamais s'exécuter après le ifbloc lorsque la ifcondition est vraie.
Makyen
@Makyen, vous soulevez un bon point. Celui que j'essayais d'ignorer. Je crois également que quelque chose doit être fait après le ifpour indiquer clairement que cette section de code est terminée et que la structure est terminée. Pour ce faire, je fais ce que je n'ai pas fait ici (parce que je ne voulais pas me distraire du point principal du PO en faisant d'autres changements). Je fais la chose la plus simple possible qui permet d'atteindre tout cela sans être distrayante. J'ajoute une ligne vierge.
candied_orange
27

Je pense que cela dépend de la sémantique du code. Si vos trois cas dépendent les uns des autres, indiquez-le explicitement. Cela augmente la lisibilité du code et le rend plus facile à comprendre pour n'importe qui d'autre. Exemple:

if (x < 0) {
    return false;
} else if (x > 0) {
    return true;
} else {
    throw new IllegalArgumentException();
}

Ici, vous dépendez clairement de la valeur de xdans les trois cas. Si vous omettiez le dernier else, ce serait moins clair.

Si vos cas ne dépendent pas directement les uns des autres, omettez-les:

if (x < 0) {
    return false;
}
if (y < 0) {
    return true;
}
return false;

Il est difficile de montrer cela dans un exemple simple sans contexte, mais j'espère que vous comprendrez mon point.

André Stannek
la source
6

Je préfère la deuxième option (séparée ifsavec non else ifet précoce return) MAIS qui est aussi longue que les blocs de code sont courts .

Si les blocs de code sont longs, il vaut mieux les utiliser else if, car sinon, vous n'auriez pas une compréhension claire des différents points de sortie du code.

Par exemple:

function doSomething(value) {
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    return -1;
  }
  if (check2(value)) {
    // ...
    // imagine 150 lines of code
    // ...
    return value;
  }
  return false;
}

Dans ce cas, il vaut mieux l'utiliser else if, stocker le code retour dans une variable et avoir une seule phrase retour à la fin.

function doSomething(value) {
  retVal=0;
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    retVal=-1;
  } else if (check2(value)) {
    // ...
    // imagne 150 lines of code
    retVal=value;
  }
  return retVal;
}

Mais comme il faut s'efforcer que les fonctions soient courtes, je dirais de rester court et de quitter tôt comme dans votre premier extrait de code.

Tulains Córdova
la source
1
Je suis d'accord avec votre prémisse, mais tant que nous discutons de la façon dont le code devrait être, pourrions-nous aussi convenir que les blocs de code devraient être courts de toute façon? Je pense qu'il est pire d'avoir un long bloc de code non divisé en fonctions que d'utiliser autrement si, si je devais choisir.
kojiro
1
Un argument curieux. returnest dans les 3 lignes de ifdonc ce n'est pas si différent que else ifmême après 200 lignes de code. Le style de retour unique que vous introduisez ici est une tradition de c et d'autres langages qui ne signalent pas les erreurs avec des exceptions. Le but était de créer un endroit unique pour nettoyer les ressources. Les langues d'exception utilisent un finallybloc pour cela. Je suppose que cela crée également un endroit pour mettre un point d'arrêt si votre débogueur ne vous laisse pas en mettre un sur les fonctions fermant l'accolade.
candied_orange
4
Je n'aime pas du tout l'affectation retVal. Si vous pouvez quitter une fonction en toute sécurité, faites-le immédiatement, sans affecter la valeur sûre à renvoyer à une variable d'une seule sortie de la fonction. Lorsque vous utilisez le point de retour unique dans une fonction très longue, je ne fais généralement pas confiance aux autres programmeurs et je finis par chercher dans le reste de la fonction d'autres modifications de la valeur de retour. Échouer rapidement et revenir tôt sont, entre autres, deux règles que je respecte.
Andy
2
À mon avis, c'est encore pire pour les longs blocs. J'essaie de quitter les fonctions le plus tôt possible, quel que soit le contexte extérieur.
Andy
2
Je suis avec DavidPacker ici. Le style de retour unique nous oblige à étudier tous les points d'affectation ainsi que le code après les points d'affectation. Étudier les points de sortie d'un style de retour précoce me serait préférable, long ou court. Tant que le langage autorise un blocage final.
candied_orange
4

J'utilise les deux dans des circonstances différentes. Lors d'un contrôle de validation, j'omet le reste. Dans un flux de contrôle, j'utilise le reste.

bool doWork(string name) {
  if(name.empty()) {
    return false;
  }

  //...do work
  return true;
}

contre

bool doWork(int value) {
  if(value % 2 == 0) {
    doWorkWithEvenNumber(value);
  }
  else {
    doWorkWithOddNumber(value);
  }
}

Le premier cas se lit plus comme si vous vérifiez d'abord toutes les conditions préalables, en les éliminant, puis en progressant vers le code que vous souhaitez exécuter. En tant que tel, le code juteux que vous voulez vraiment exécuter appartient directement à la portée de la fonction; c'est vraiment la fonction.
Le deuxième cas se lit plus comme si les deux chemins sont un code valide à exécuter qui est tout aussi pertinent pour la fonction que l'autre en fonction d'une condition. En tant que tels, ils appartiennent à des niveaux de portée similaires les uns aux autres.

Altainia
la source
0

La seule situation que j'ai jamais vue vraiment importante est un code comme celui-ci:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    }
    if (left != null) {
        return Arrays.asList(left);
    }
    if (right != null) {
        return Arrays.asList(right);
    }
    return Arrays.asList();
}

Il y a clairement quelque chose qui cloche ici. Le problème est qu'il n'est pas clair s'il returnfaut l'ajouter ou le Arrays.asListsupprimer. Vous ne pouvez pas résoudre ce problème sans une inspection plus approfondie des méthodes connexes. Vous pouvez éviter cette ambiguïté en utilisant toujours des blocs if-else exhaustifs. Cette:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    } else if (left != null) {
        return Arrays.asList(left);
    } else if (right != null) {
        return Arrays.asList(right);
    } else {
        return Arrays.asList();
    }
}

ne compilera pas (dans les langues vérifiées statiquement) sauf si vous ajoutez d'abord un retour explicite if.

Certaines langues ne permettent même pas le premier style. J'essaie de ne l'utiliser que dans un contexte strictement impératif ou pour des conditions préalables dans les méthodes longues:

List<?> toList() {
    if (left == null || right == null) {
        throw new IllegalStateException()
    }
    // ...
    // long method
    // ...
}
Banthar
la source
-2

Avoir trois sorties de la fonction comme celle-ci est vraiment déroutant si vous essayez de suivre le code et de mauvaises pratiques.

Si vous avez un seul point de sortie pour la fonction, les autres sont requis.

var result;
if(check1(value)
{
    result = -1;
}
else if(check2(value))
{
    result = value;
}
else 
{
    result = 0;
}
return result;

En fait, allons encore plus loin.

var result = 0;
var c1 = check1(value);
var c2 = check2(value);

if(c1)
{
    result = -1;
}
else if(c2)
{
    result = value;
}

return result;

Assez juste, vous pourriez plaider pour un retour anticipé unique sur la validation des entrées. Évitez simplement d'envelopper la masse entière si votre logique dans un if.

Ewan
la source
3
Ce style provient de langues avec une gestion explicite des ressources. Un point unique était nécessaire pour libérer les ressources allouées. Dans les langues avec gestion automatique des ressources, le point de retour unique n'est pas si important. Vous devriez plutôt vous concentrer sur la réduction du nombre et de la portée des variables.
Banthar
a: aucune langue n'est spécifiée dans la question. b: Je pense que vous vous trompez à ce sujet. il y a plein de bonnes raisons pour un seul retour. il réduit la complexité et améliore la lisibilité
Ewan
1
Parfois, cela réduit la complexité, parfois cela l'augmente. Voir wiki.c2.com/?ArrowAntiPattern
Robert Johnson
Non, je pense que cela réduit toujours la complexité cyclomatique, voir mon deuxième exemple. check2 s'exécute toujours
Ewan
avoir le retour anticipé est une optimisation qui complique le code en déplaçant du code dans un conditionnel
Ewan
-3

Je préfère omettre la déclaration else redondante. Chaque fois que je le vois (en révision ou refactorisation de code), je me demande si l'auteur a compris le flux de contrôle et qu'il pourrait y avoir un bug dans le code.

Si l'auteur pensait que la déclaration else était nécessaire et ne comprenait donc pas les implications du flux de contrôle de la déclaration de retour, un tel manque de compréhension est certainement une source majeure de bogues, dans cette fonction, et en général.

Andreas Warberg
la source
3
Bien que je sois d'accord avec vous, ce n'est pas un sondage d'opinion. Veuillez sauvegarder vos affirmations, sinon les électeurs ne vous traiteront pas avec bonté.
candied_orange