Lors d'une révision de code, un autre développeur et moi avons eu un petit débat sur la manière appropriée ou correcte de gérer plusieurs retours, et si attribuer un pointeur à null, puis le définir donne une différence par rapport au simple renvoi de la valeur, un retourne plus tard null.
private ServiceParam getRequestServiceParam(SlingHttpServletRequest request) {
try {
return new ServiceParam(IOUtils.toString(request.getReader()));
} catch (IOException e) {
LOGGER.error("IOException", e);
}
return null;
}
vs
private ServiceParam getRequestServiceParam(SlingHttpServletRequest request) {
ServiceParam sp = null;
try {
sp = new ServiceParam(IOUtils.toString(request.getReader()));
} catch (IOException e) {
LOGGER.error("IOException", e);
}
return sp;
}
Fonctionnellement, ils semblent identiques, mais nous ne savons pas si l'un est plus correct que le autre. Bien que je crains que ce ne soit juste un argument tabulations vs espaces.
4 Réponses :
Je suivrais les conseils de @khelwood. Utilisez Facultatif si vous ne souhaitez pas gérer l'exception levée en dehors de l'appel de fonction comme indiqué par @ControlAltDel
private ServiceParam getRequestServiceParam(SlingHttpServletRequest request) {
try {
return new ServiceParam(IOUtils.toString(request.getReader()));
} catch (IOException e) {
LOGGER.error("IOException", e);
return null;
}
}
si vous devez utiliser null Je préférerais la deuxième approche
private Optional<ServiceParam> getRequestServiceParam(SlingHttpServletRequest request) {
try {
return Optional.of(new ServiceParam(IOUtils.toString(request.getReader())));
} catch (IOException e) {
LOGGER.error("IOException", e);
return Optional.empty();
}
}
parce que je n'ai pas à réfléchir à deux fois, tout est dit tel quel. Donc pas de déclaration de variable inutile, pas besoin de se souvenir de ce qu'il contient. (Cet argument est également vrai pour l'approche facultative.)
Mise à jour 1 : Ceci est vrai pour deux, peut-être trois instructions de retour dans une seule méthode, s'il y a plusieurs instructions de retour d'autres solutions doivent être envisagées.
Mise à jour 2 : il existe également une règle de sondeur squid: S1142
IMHO
Les retours multiples sont sujets aux erreurs en ce qui concerne la maintenance du code. Les retours multiples ne sont pas les plus faciles à lire.
Quand il s'agit de choix comme celui-ci, je choisis toujours le code qui est plus facile à lire par quelqu'un qui ne l'a pas écrit.
Quand quelqu'un d'autre le lit dans un an, il se peut qu'il soit pressé, qu'il travaille sur une urgence, et qu'il souhaite un code qu'il puisse comprendre en un coup d'œil.
Je m'efforce d'écrire du code très facile à lire, c'est-à-dire moins risqué à refactoriser pour quelqu'un d'autre.
Il est toujours bon d'éviter les variables locales partout où vous le pouvez, pour de meilleures performances. Si vous êtes trop préoccupé par la lisibilité, choisissez l'option 1, sinon choisissez l'option 2. Jetez un œil à cette réponse pour plus de détails sur l'attribution du résultat à une variable locale avant de retourner.
La question de savoir si plus d'un retour d'une méthode est acceptable ou non est une question d'opinion. Mais il y a des compromis ici pour dire où certaines des solutions sont meilleures que d'autres. Les plaintes contre Sonarqube sont un bon point de départ, mais il y a des problèmes plus importants.
Certaines personnes pensent qu'il est mauvais d'avoir plus d'un retour et veulent suivre les règles de programmation structurée quoi qu'il arrive. La programmation structurée est meilleure que l'utilisation de goto, et elle produit un code d'apparence cohérente.
D'autres personnes soulignent que l'application d'un retour rend l'imbrication des instructions de contrôle plus profonde, ce qui entraîne un code plus difficile à lire. Dans le cas du premier exemple, je me retrouve à parcourir de haut en bas dans la méthode à la recherche de l'endroit où la variable locale reçoit une valeur qui lui est attribuée, et je pense que c'est ennuyeux.
De plus, rien de tout cela n'implique la performance. C'est juste une question de codage maladroit et de gestion des exceptions loin d'être idéale. Lorsque vous retournez null, vous créez une opportunité pour que le code appelant échoue à vérifier la référence pour null, il serait préférable de laisser l'exception être levée.
Je n'utiliserais pas Optionnel pour cela à moins que cela ne ressemble à quelque chose que j'utiliserais dans un contexte monadique où j'enchaîne des éléments et j'utilise flatMap pour gérer les cas vides facultatifs à ma place. Sinon, il est ennuyeux de devoir envelopper et déballer cela, c'est un peu mieux que de renvoyer null, uniquement parce que cela force le code appelant à considérer le cas vide.
Dans ce cas, la gêne réelle est causée par la lecture à partir de httprequest, ce qui déclenche l'exception IOException. Vous pouvez ajouter throws IOException à la méthode et laisser l'exception être levée. Mais il serait préférable de déplacer ce code vers n'importe quel servlet ou contrôleur Web qui reçoit la httprequest, et que toute IOException lancée en l'appelant soit gérée de la même manière que tout ce que le servlet ou le contrôleur fait.
Une fois que vous avez déplacé ce code de lecture de requête, il n'y a aucune raison impérieuse pour que cette méthode existe. La suppression du code inutile est une bonne chose.
Merci pour la réponse détaillée, et pour répondre à ce que les autres semblent avoir lancé sans contexte. Cette méthode a été initialement signalée lors d'une session de groupe comme "des méthodes à ne jamais créer" pour son manque de besoin. Je crains que cela ait pollué l'intention de la question, car de nombreux commentaires s'y sont accrochés.
@ K-Dandy: il est parfois très difficile d'écrire une question sans introduire quelque chose qui distrait les gens de ce que vous voulez voir discuté.
Le premier a encore une ligne ...
Lequel trouvez-vous le plus clair? Vous pouvez également déplacer le
return nullvers le haut dans le bloccatchpour l'associer plus clairement à la gestion des exceptions.Le vrai problème est que vous
renvoyez null.À mon avis, la meilleure façon de gérer cela serait soit de ne pas attraper IOException (déclarer "throws IOException") ou de l'attraper et de lancer une exception différente, peut-être une RuntimeException. En général, vous ne devriez pas attraper et ne pas relancer les exceptions à moins qu'il n'y ait un autre comportement que vous pouvez fournir efficacement
Ou je suppose que vous pouvez renvoyer un
Optional si cela vous semble approprié.Cela visait davantage à traiter les retours multiples, plutôt que l'aspect pratique de return null.