Est-ce qu’avoir une déclaration de retour suffit à satisfaire la syntaxe mauvaise pratique?

Considérez le code suivant:

public Object getClone(Cloneable a) throws TotallyFooException { if (a == null) { throw new TotallyFooException(); } else { try { return a.clone(); } catch (CloneNotSupportedException e) { e.printStackTrace(); } } //cant be reached, in for syntax return null; } 

Le return null; est nécessaire car une exception peut être interceptée, mais dans un tel cas puisque nous avons déjà vérifié si elle était nulle (et supposons que nous connaissons la classe que nous appelons prend en charge le clonage), nous soaps que l’instruction try ne sera jamais échouée.

Est-ce une mauvaise pratique de mettre la déclaration de retour supplémentaire à la fin juste pour satisfaire la syntaxe et éviter les erreurs de compilation (avec un commentaire expliquant qu’elle ne sera pas atteinte) ou existe-t-il un meilleur moyen de coder déclaration de retour est inutile?

Une manière plus claire sans déclaration de retour supplémentaire est la suivante. Je ne rattraperais pas CloneNotSupportedException non plus, mais laisse-la aller à l’appelant.

 if (a != null) { try { return a.clone(); } catch (CloneNotSupportedException e) { e.printStackTrace(); } } throw new TotallyFooException(); 

Il est presque toujours possible de manipuler l’ordre pour aboutir à une syntaxe plus simple que ce que vous aviez initialement.

On peut vraiment l’atteindre. Notez que vous imprimez uniquement le stacktrace dans la clause catch .

Dans le cas où a != null et il y aura une exception, le return null sera atteint. Vous pouvez supprimer cette instruction et la remplacer par throw new TotallyFooException(); .

En général * , si null est un résultat valide d’une méthode (c.-à-d. Que l’utilisateur l’ attend et que cela signifie quelque chose), le renvoyer comme un signal pour des “données non trouvées” ou une exception n’est pas une bonne idée. Sinon, je ne vois aucun problème pour lequel vous ne devriez pas retourner null .

Prenons par exemple la méthode Scanner#ioException :

Renvoie la dernière IOException lancée par le fichier lisible sous-jacent de ce scanner. Cette méthode renvoie null si aucune exception n’existe .

Dans ce cas, la valeur renvoyée null a un sens clair, quand j’utilise la méthode, je peux être sûr que je suis devenu null parce qu’il n’y avait pas d’exception et que la méthode a essayé de faire quelque chose et qu’elle a échoué.

* Notez que parfois vous voulez retourner null même si la signification est ambiguë. Par exemple, le HashMap#get :

Une valeur de retour de null n’indique pas nécessairement que la carte ne contient aucun mappage pour la clé; il est également possible que la carte mappe explicitement la clé sur null . L’opération containsKey peut être utilisée pour distinguer ces deux cas.

Dans ce cas, null peut indiquer que la valeur null été trouvée et renvoyée, ou que le hashmap ne contient pas la clé demandée.

Est-ce une mauvaise pratique de mettre la déclaration de retour supplémentaire à la fin juste pour satisfaire la syntaxe et éviter les erreurs de compilation (avec un commentaire expliquant qu’il ne sera pas atteint)

Je pense que return null est une mauvaise pratique pour le terminus d’une twig inaccessible. Il est préférable de lancer une RuntimeException ( AssertionError serait également acceptable) pour accéder à cette ligne, quelque chose s’est mal passé et l’application est dans un état inconnu. Tout comme ceci est (comme ci-dessus) parce que le développeur a manqué quelque chose (les objects peuvent être non-nulles et non clonables).

Je n’utiliserais probablement pas InternalError sauf si je suis très sûr que le code est inaccessible (par exemple après un System.exit() ) car il est plus probable que je commette une erreur que la VM.

Je n’utiliserais qu’une exception personnalisée (telle que TotallyFooException ) si obtenir cette “ligne inaccessible” signifie la même chose que partout où vous lancez cette exception.

Vous avez intercepté CloneNotSupportedException ce qui signifie que votre code peut le gérer. Mais après l’avoir attrapé, vous n’avez littéralement aucune idée de ce qu’il faut faire lorsque vous atteignez la fin de la fonction, ce qui implique que vous ne pouvez pas le gérer. Donc, vous avez raison de dire que c’est une odeur de code dans ce cas, et à mon avis, vous ne devriez pas avoir pris CloneNotSupportedException .

Je préférerais utiliser Objects.requireNonNull() pour vérifier si le paramètre a n’est pas nul. Donc, il est clair lorsque vous lisez le code que le paramètre ne doit pas être nul.

Et pour éviter les exceptions vérifiées, je lancerais l’ CloneNotSupportedException tant que RuntimeException .

Pour les deux, vous pourriez append du texte avec l’intention de savoir pourquoi cela ne devrait pas arriver ou être le cas.

 public Object getClone(Object a) { Objects.requireNonNull(a); try { return a.clone(); } catch (CloneNotSupportedException e) { throw new IllegalArgumentException(e); } } 

Dans ce genre de situation, j’écrirais

 public Object getClone(SomeInterface a) throws TotallyFooException { // Precondition: "a" should be null or should have a someMethod method that // does not throw a SomeException. if (a == null) { throw new TotallyFooException() ; } else { try { return a.someMethod(); } catch (SomeException e) { throw new IllegalArgumentException(e) ; } } } 

Fait intéressant, vous dites que “l’instruction ne va jamais échouer”, mais vous avez quand même pris la peine d’écrire une déclaration e.printStackTrace(); que vous prétendez ne sera jamais exécuté. Pourquoi?

Peut-être que votre croyance n’est pas si fermement tenue. C’est bien (à mon avis), puisque votre croyance n’est pas basée sur le code que vous avez écrit, mais plutôt sur l’attente que votre client ne viole pas la condition préalable. Mieux vaut programmer les méthodes publiques de manière défensive.

Au fait, votre code ne sera pas compilé pour moi. Vous ne pouvez pas appeler a.clone() même si le type de est Cloneable . Au moins, le compilateur d’Eclipse le dit. Expression a.clone() donne une erreur

La méthode clone () n’est pas définie pour le type Cloneable

Ce que je ferais pour votre cas particulier est

 public Object getClone(PubliclyCloneable a) throws TotallyFooException { if (a == null) { throw new TotallyFooException(); } else { return a.clone(); } } 

PubliclyCloneable est défini par

 interface PubliclyCloneable { public Object clone() ; } 

Ou, si vous avez absolument besoin que le type de paramètre soit Cloneable , les éléments suivants au moins sont Cloneable .

 public static Object getClone(Cloneable a) throws TotallyFooException { // Precondition: "a" should be null or point to an object that can be cloned without // throwing any checked exception. if (a == null) { throw new TotallyFooException(); } else { try { return a.getClass().getMethod("clone").invoke(a) ; } catch( IllegalAccessException e ) { throw new AssertionError(null, e) ; } catch( InvocationTargetException e ) { Throwable t = e.getTargetException() ; if( t instanceof Error ) { // Unchecked exceptions are bubbled throw (Error) t ; } else if( t instanceof RuntimeException ) { // Unchecked exceptions are bubbled throw (RuntimeException) t ; } else { // Checked exceptions indicate a precondition violation. throw new IllegalArgumentException(t) ; } } catch( NoSuchMethodException e ) { throw new AssertionError(null, e) ; } } } 

Les exemples ci-dessus sont valides et très Java. Cependant, voici comment je voudrais répondre à la question du PO sur la façon de gérer ce retour:

 public Object getClone(Cloneable a) throws CloneNotSupportedException { return a.clone(); } 

Il n’y a aucun avantage à vérifier a pour voir s’il est nul. Cela va à NPE. L’impression d’une trace de stack n’est également pas utile. La trace de la stack est la même quel que soit l’endroit où elle est gérée.

Il n’y a aucun avantage à utiliser le code avec des tests nuls inutiles et une gestion des exceptions inutile. En supprimant le courrier indésirable, le problème de retour est sans object.

(Notez que l’OP comprenait un bogue dans la gestion des exceptions; c’est pourquoi le retour était nécessaire. L’OP n’aurait pas eu tort la méthode que je propose.)

Est-ce qu’avoir une déclaration de retour suffit à satisfaire la syntaxe mauvaise pratique?

Comme d’autres l’ont mentionné, dans votre cas, cela ne s’applique pas réellement.

Pour répondre à la question, cependant, les programmes de type Lint n’ont certainement pas compris! J’ai vu deux personnes différentes lutter contre cela dans une déclaration de changement.

  switch (var) { case A: break; default: return; break; // Unreachable code. Coding standard violation? } 

L’un d’eux s’est plaint du fait que le fait de ne pas avoir eu de pause constituait une violation de la norme de codage. L’autre s’est plaint que l’ avoir été un parce que c’était du code inaccessible.

Je l’ai remarqué parce que deux programmeurs différents ont continué à vérifier le code avec la pause ajoutée puis supprimée puis ajoutée puis supprimée, selon l’parsingur de code qu’ils ont utilisé ce jour-là.

Si vous vous retrouvez dans cette situation, choisissez-en une et commentez l’anomalie, qui est la bonne forme que vous vous êtes montrée. C’est le meilleur et le plus important à emporter.

Ce n’est pas juste pour satisfaire la syntaxe. C’est une exigence sémantique du langage que chaque chemin de code mène à un retour ou à un jet. Ce code n’est pas conforme. Si l’exception est interceptée, un retour suivant est requirejs.

Pas de «mauvaise pratique» à ce sujet, ni de satisfaction du compilateur en général.

Quoi qu’il en soit, qu’il s’agisse de syntaxe ou de sémantique, vous n’avez pas le choix.

Je réécrirais ceci pour avoir le retour à la fin. Pseudocode:

 if a == null throw ... // else not needed, if this is reached, a is not null Object b try { b = a.clone } catch ... return b 

Personne n’a encore mentionné cela, alors voici:

 public static final Object ERROR_OBJECT = ... //... public Object getClone(Cloneable a) throws TotallyFooException { Object ret; if (a == null) throw new TotallyFooException(); //no need for else here try { ret = a.clone(); } catch (CloneNotSupportedException e) { e.printStackTrace(); //something went wrong! ERROR_OBJECT could also be null ret = ERROR_OBJECT; } return ret; } 

Je n’aime pas return intérieur des blocs d’ try pour cette raison même.

Le retour nul; est nécessaire car une exception peut être interceptée, mais dans un tel cas puisque nous avons déjà vérifié si elle était nulle (et supposons que nous connaissons la classe que nous appelons prend en charge le clonage), nous soaps que l’instruction try ne sera jamais échouée.

Si vous connaissez les détails des entrées impliquées d’une manière telle que vous savez que l’instruction try ne peut jamais échouer, quel est l’intérêt de l’avoir? Évitez de l’ try si vous savez avec certitude que les choses vont toujours réussir (bien qu’il soit rare que vous soyez absolument sûr de toute la durée de vie de votre base de code).

En tout cas, le compilateur n’est malheureusement pas un lecteur d’esprit. Il voit la fonction et ses entrées, et compte tenu des informations dont il dispose, il a besoin de cette déclaration de return au bas de la page.

Est-ce une mauvaise pratique de mettre la déclaration de retour supplémentaire à la fin juste pour satisfaire la syntaxe et éviter les erreurs de compilation (avec un commentaire expliquant qu’elle ne sera pas atteinte) ou existe-t-il un meilleur moyen de coder déclaration de retour est inutile?

Au contraire, je vous conseille d’éviter les avertissements du compilateur, par exemple, même si cela coûte une autre ligne de code. Ne vous inquiétez pas trop du nombre de lignes ici. Établissez la fiabilité de la fonction grâce à des tests, puis passez à autre chose. En prétendant que vous pourriez omettre la déclaration de return , imaginez revenir à ce code un an plus tard et essayez ensuite de décider si cette déclaration de return en bas causera plus de confusion que certains commentaires détaillant les détails de son omission à cause d’hypothèses vous pouvez faire sur les parameters d’entrée. Très probablement, la déclaration de return sera plus facile à traiter.

Cela dit, spécifiquement à propos de cette partie:

 try { return a.clone(); } catch (CloneNotSupportedException e) { e.printStackTrace(); } ... //cant be reached, in for syntax return null; 

Je pense qu’il y a quelque chose de bizarre avec la mentalité de gestion des exceptions ici. Vous voulez généralement avaler des exceptions sur un site où vous avez quelque chose de significatif à faire en réponse.

Vous pouvez penser à try/catch comme mécanisme de transaction. try faire ces modifications, si elles échouent et que nous entrons dans le bloc catch , faites cela (quel que soit le bloc catch ) en réponse au processus de restauration et de restauration.

Dans ce cas, il suffit d’imprimer une stacktrace et de forcer le renvoi de null si ce n’est pas exactement un état d’esprit transaction / récupération. Le code transfère la responsabilité de la gestion des erreurs à tous les codes appelant getClone pour vérifier manuellement les échecs. Vous préférerez peut-être intercepter CloneNotSupportedException et le traduire sous une autre forme plus significative d’exception et la lancer, mais vous ne voulez pas simplement avaler l’exception et renvoyer une valeur NULL dans ce cas, car ce n’est pas comme un site de récupération de transaction. .

Vous finirez par divulguer les responsabilités aux appelants pour vérifier manuellement et traiter les échecs de cette façon, lorsque le lancement d’une exception éviterait cela.

C’est comme si vous chargiez un fichier, c’est la transaction de haut niveau. Vous pourriez avoir un try/catch là-bas. Pendant le processus de chargement d’un fichier, vous pouvez cloner des objects. S’il y a une défaillance dans cette opération de haut niveau (chargement du fichier), vous devez généralement renvoyer des exceptions jusqu’à ce bloc d’ try/catch transaction de haut niveau afin de pouvoir récupérer avec succès d’un échec lors du chargement d’un fichier. (que ce soit à cause d’une erreur de clonage ou autre). Donc, en général, nous ne voulons pas simplement engloutir une exception dans un endroit granulaire comme celui-ci, puis renvoyer un null, par exemple, car cela irait à l’encontre de la valeur et du but des exceptions. Au lieu de cela, nous voulons propager les exceptions jusqu’à un site où nous pouvons le traiter de manière significative.

Votre exemple n’est pas idéal pour illustrer votre question comme indiqué dans le dernier paragraphe:

Est-ce une mauvaise pratique de mettre la déclaration de retour supplémentaire à la fin juste pour satisfaire la syntaxe et éviter les erreurs de compilation (avec un commentaire expliquant qu’elle ne sera pas atteinte) ou existe-t-il un meilleur moyen de coder déclaration de retour est inutile?

Un meilleur exemple serait l’implémentation du clone lui-même:

  public class A implements Cloneable { public Object clone() { try { return super.clone() ; } catch (CloneNotSupportedException e) { throw new InternalError(e) ; // vm bug. } } } 

Ici, la clause catch ne doit jamais être saisie. La syntaxe nécessite toujours de lancer quelque chose ou de renvoyer une valeur. Étant donné que le retour de quelque chose n’a pas de sens, une InternalError est utilisée pour indiquer une condition de machine virtuelle grave.