Est-ce une mauvaise pratique d'utiliser return dans une méthode void?

92

Imaginez le code suivant:

void DoThis()
{
    if (!isValid) return;

    DoThat();
}

void DoThat() {
    Console.WriteLine("DoThat()");
}

Est-il acceptable d'utiliser un retour dans une méthode void? Y a-t-il une pénalité de performance? Ou il serait préférable d'écrire un code comme celui-ci:

void DoThis()
{
    if (isValid)
    {
        DoThat();
    }
}

la source
1
Qu'en est-il de: void DoThis () {if (isValid) DoThat (); }
Dscoduc
30
imaginez le code? Pourquoi? C'est juste là! :-D
STW
C'est une bonne question, je pense toujours que c'est une bonne pratique d'utiliser le retour; pour quitter la méthode ou la fonction. Surtout dans une méthode d'exploration de données LINQ ayant plusieurs résultats IQueryable <T> et tous dépendent les uns des autres. Si l'un d'eux n'a pas de résultat, alertez et quittez.
Cheung

Réponses:

173

Un retour dans une méthode void n'est pas mauvais, est une pratique courante pour inverser les ifinstructions pour réduire l'imbrication .

Et avoir moins d'imbrication sur vos méthodes améliore la lisibilité et la maintenabilité du code.

En fait, si vous avez une méthode void sans aucune instruction return, le compilateur générera toujours une instruction ret à la fin de celle-ci.

Christian C. Salvadó
la source
33

Il y a une autre bonne raison d'utiliser des gardes (par opposition au code imbriqué): si un autre programmeur ajoute du code à votre fonction, il travaille dans un environnement plus sûr.

Considérer:

void MyFunc(object obj)
{
    if (obj != null)
    {
        obj.DoSomething();
    }
}

contre:

void MyFunc(object obj)
{
    if (obj == null)
        return;

    obj.DoSomething();
}

Maintenant, imaginez qu'un autre programmeur ajoute la ligne: obj.DoSomethingElse ();

void MyFunc(object obj)
{
    if (obj != null)
    {
        obj.DoSomething();
    }

    obj.DoSomethingElse();
}

void MyFunc(object obj)
{
    if (obj == null)
        return;

    obj.DoSomething();
    obj.DoSomethingElse();
}

C'est évidemment un cas simpliste, mais le programmeur a ajouté un crash au programme dans la première instance (code imbriqué). Dans le deuxième exemple (sortie anticipée avec des gardes), une fois que vous avez dépassé la garde, votre code est à l'abri de l'utilisation involontaire d'une référence nulle.

Bien sûr, un grand programmeur ne fait pas d'erreurs comme celle-ci (souvent). Mais mieux vaut prévenir que guérir - nous pouvons écrire le code de manière à éliminer complètement cette source potentielle d'erreurs. L'imbrication ajoute de la complexité. Les meilleures pratiques recommandent donc de refactoriser le code pour réduire l'imbrication.

Jason Williams
la source
Oui, mais d'un autre côté, plusieurs couches d'imbrication, avec leurs conditions, rendent le code encore plus sujet aux bogues, la logique plus difficile à suivre et, surtout, plus difficile à déboguer. Les fonctions plates sont un moindre mal, IMO.
Skrim
18
Je plaide pour une nidification réduite! :-)
Jason Williams
Je suis d'accord avec ça. De plus, du point de vue du refactor, il est plus facile et plus sûr de refactoriser la méthode si obj devient une structure ou quelque chose que vous pouvez garantir ne sera pas nul.
Phil Cooper
18

Mauvaise pratique ??? En aucune façon. En fait, il est toujours préférable de gérer les validations en revenant de la méthode au plus tôt si les validations échouent. Sinon, cela entraînerait une énorme quantité de ifs & autres imbriqués. La résiliation anticipée améliore la lisibilité du code.

Vérifiez également les réponses à une question similaire: Dois-je utiliser l'instruction return / continue au lieu de if-else?

Utilisateur SO
la source
8

Ce n'est pas une mauvaise pratique (pour toutes les raisons déjà évoquées). Cependant, plus vous avez de retours dans une méthode, plus elle devrait être divisée en méthodes logiques plus petites.

Mike Hall
la source
8

Le premier exemple utilise une instruction de garde. De Wikipedia :

En programmation informatique, une garde est une expression booléenne qui doit être évaluée à vrai si l'exécution du programme doit se poursuivre dans la branche en question.

Je pense qu'avoir un groupe de gardes au sommet d'une méthode est une façon parfaitement compréhensible de programmer. Il dit essentiellement "n'exécutez pas cette méthode si l'une de celles-ci est vraie".

Donc en général, il aimerait ceci:

void DoThis()
{
  if (guard1) return;
  if (guard2) return;
  ...
  if (guardN) return;

  DoThat();
}

Je pense que c'est beaucoup plus lisible alors:

void DoThis()
{
  if (guard1 && guard2 && guard3)
  {
    DoThat();
  }
}
cdmckay
la source
3

Il n'y a aucune pénalité de performance, cependant le deuxième morceau de code est plus lisible et donc plus facile à maintenir.

Russell
la source
Russell Je ne suis pas d'accord avec votre opinion, mais vous n'auriez pas dû être voté à la baisse. +1 pour égaliser. Btw, je crois qu'un test booléen et un retour sur une seule ligne suivi d'une ligne vide est une indication claire de ce qui se passe. par exemple le premier exemple de Rodrigo.
Paul Sasik
Je ne suis pas d'accord avec cela. L'augmentation de l'imbrication n'améliore pas la lisibilité. Le premier morceau de code utilise une instruction "guard", qui est un modèle parfaitement compréhensible.
cdmckay
Je suis également en désaccord. Les clauses de garde qui renflouent tôt une fonction sont généralement considérées comme une bonne chose de nos jours pour aider un lecteur à comprendre l'implémentation.
Pete Hodgson
2

Dans ce cas, votre deuxième exemple est un meilleur code, mais cela n'a rien à voir avec le retour d'une fonction void, c'est simplement parce que le deuxième code est plus direct. Mais revenir d'une fonction vide est tout à fait acceptable.

Imagiste
la source
0

C'est parfaitement correct et pas de «pénalité de performance», mais n'écrivez jamais une instruction «si» sans crochets.

Toujours

if( foo ){
    return;
}

C'est beaucoup plus lisible; et vous ne supposerez jamais accidentellement que certaines parties du code se trouvent dans cette instruction alors qu'elles ne le sont pas.

Soie de midi
la source
2
lisible est subjectif. à mon humble avis, tout ce qui est ajouté au code qui est inutile le rend moins lisible ... (je dois en lire plus, puis je me demande pourquoi il est là et perdre du temps à essayer de m'assurer que je ne manque pas quelque chose) ... mais c'est mon opinion subjective
Charles Bretana
10
La meilleure raison de toujours inclure les accolades est moins une question de lisibilité que de sécurité. Sans les accolades, il est trop facile pour quelqu'un plus tard de corriger un bogue qui nécessite des instructions supplémentaires dans le cadre du if, ne prêtez pas suffisamment attention et ajoutez-les sans ajouter également les accolades. En incluant toujours les accolades, ce risque est éliminé.
Scott Dorman
2
Silky, veuillez appuyer sur Entrée avant votre {. Cela vous aligne {avec votre }dans la même colonne, ce qui facilite grandement la lisibilité (beaucoup plus facile de trouver les accolades ouvertes / fermées correspondantes).
Imagist
1
@Imagist Je laisse cela à mes préférences personnelles; et c'est fait comme je préfère :)
Noon Silk
1
Si chaque accolade fermée correspond à une accolade ouverte qui est placée au même niveau d'indentation , il ifsera alors facile de distinguer visuellement quelles instructions nécessitent des accolades fermées, et ainsi avoir des ifinstructions contrôlant une seule instruction sera sûr. Le fait de repousser l'accolade ouverte sur la ligne avec le ifenregistre une ligne d'espace vertical sur chaque instruction multiple if, mais nécessitera l'utilisation d'une ligne d'accolade fermée autrement inutile.
supercat
0

Je ne suis pas d'accord avec vous tous les jeunes whippersnappers sur celui-ci.

Utiliser le retour au milieu d'une méthode, nulle ou non, est une très mauvaise pratique, pour des raisons qui ont été énoncées assez clairement, il y a près de quarante ans, par le regretté Edsger W. Dijkstra, à partir de la célèbre déclaration GOTO considérée comme nuisible ", et continuant dans" Programmation structurée ", par Dahl, Dijkstra et Hoare.

La règle de base est que chaque structure de contrôle et chaque module doivent avoir exactement une entrée et une sortie. Un retour explicite au milieu du module enfreint cette règle et rend beaucoup plus difficile de raisonner sur l'état du programme, ce qui à son tour rend beaucoup plus difficile de dire si le programme est correct ou non (ce qui est une propriété beaucoup plus forte que "que cela semble fonctionner ou non").

La «déclaration GOTO considérée comme nuisible» et la «programmation structurée» ont lancé la révolution de la «programmation structurée» des années 1970. Ces deux éléments sont les raisons pour lesquelles nous avons aujourd'hui if-then-else, while-do et d'autres constructions de contrôle explicites, et pourquoi les déclarations GOTO dans des langages de haut niveau sont sur la liste des espèces en danger. (Mon opinion personnelle est qu'elles doivent figurer sur la liste des espèces disparues.)

Il convient de noter que Message Flow Modulator, le premier logiciel militaire qui a JAMAIS passé les tests d'acceptation du premier essai, sans écarts, dérogations ou verbiage "ouais, mais", a été écrit dans une langue qui n'avait même pas une instruction GOTO.

Il convient également de mentionner que Nicklaus Wirth a changé la sémantique de l'instruction RETURN dans Oberon-07, la dernière version du langage de programmation Oberon, ce qui en fait un élément final de la déclaration d'une procédure typée (c'est-à-dire une fonction), plutôt qu'un instruction exécutable dans le corps de la fonction. Son explication du changement dit qu'il l'a fait précisément parce que le formulaire précédent était une violation du principe à une sortie de la programmation structurée.

John R. Strohm
la source
2
@John: nous avons surmonté l'injonction Dykstra sur les retours multiples à peu près au moment où nous avons surmonté Pascal (la plupart d'entre nous, en tout cas).
John Saunders
Les cas où plusieurs retours sont requis sont souvent des signes qu'une méthode essaie d'en faire trop et qu'elle doit être réduite. Je ne vais pas aller aussi loin que John avec cela, et une déclaration de retour dans le cadre de la validation des paramètres peut être une exception raisonnable, mais je comprends d'où vient l'idée.
kyoryu
@nairdaen: Il y a encore une controverse sur les exceptions dans ce trimestre. Ma ligne directrice est la suivante: si le système en cours de développement DOIT résoudre le problème qui a causé la condition exceptionnelle d'origine, et que cela ne me dérange pas d'énerver le type qui devra écrire ce code, je vais lancer une exception. Ensuite, on me crie dessus lors d'une réunion, parce que le gars n'a pas pris la peine d'attraper l'exception, et l'application s'est plantée pendant les tests, et j'explique POURQUOI il doit résoudre le problème, et les choses se rétablissent.
John R. Strohm
Il y a une grande différence entre les déclarations de garde et les gotos. Le mal des gotos est qu'ils peuvent sauter n'importe où, ce qui peut être très déroutant à démêler et à se souvenir. Les instructions de garde sont exactement le contraire - elles fournissent une entrée fermée à une méthode, après quoi vous savez que vous travaillez dans un environnement "sûr", ce qui réduit le nombre de choses que vous devez considérer lorsque vous écrivez le reste du code (par exemple "Je sais que ce pointeur ne sera jamais nul, donc je n'ai pas besoin de gérer ce cas tout au long du code").
Jason Williams
@Jason: La question initiale ne portait pas spécifiquement sur les instructions de garde, mais sur les déclarations de retour aléatoires au milieu d'une méthode. L'exemple donné paraissait être un garde. Le problème clé est que, sur le site de retour, vous voulez être en mesure de raisonner sur ce que la méthode a fait ou n'a pas fait, et les retours aléatoires rendent cela plus difficile, pour EXACTEMENT les mêmes raisons que les GOTO aléatoires le rendent plus difficile. Voir: Dijkstra, "Déclaration GOTO considérée comme nuisible". Du côté de la syntaxe, cdmckay a donné, dans une autre réponse, sa syntaxe préférée pour les gardes; Je ne suis pas d'accord avec son opinion sur la forme la plus lisible.
John R. Strohm
0

Lorsque vous utilisez des gardes, assurez-vous de suivre certaines directives pour ne pas confondre les lecteurs.

  • la fonction fait une chose
  • les gardes ne sont introduits que comme première logique de la fonction
  • la partie non imbriquée contient l' intention principale de la fonction

Exemple

// guards point you to the core intent
void Remove(RayCastResult rayHit){

  if(rayHit== RayCastResult.Empty)
    return
    ;
  rayHit.Collider.Parent.Remove();
}

// no guards needed: function split into multiple cases
int WonOrLostMoney(int flaw)=>
  flaw==0 ? 100 :
  flaw<10 ? 30 :
  flaw<20 ? 0 :
  -20
;
Orangle
la source
-3

Lancer une exception au lieu de ne rien renvoyer lorsque l'objet est nul, etc.

Votre méthode s'attend à ce que l'objet ne soit pas nul et n'est pas le cas, vous devez donc lever une exception et laisser l'appelant gérer cela.

Mais le retour anticipé n'est pas une mauvaise pratique autrement.

Dhananjay
la source
1
La réponse ne répond pas à la question. La question est une méthode vide donc il n'y a rien retourné. De plus, les méthodes n'ont pas de paramètres. J'obtiens le point de ne pas renvoyer null si le type de retour est un objet mais cela ne s'applique pas à cette question.
Luke Hammer