Comment défensivement devrais-je programmer?

Je travaillais avec une petite routine utilisée pour créer une connexion à une firebase database:

Avant

public DbConnection GetConnection(Ssortingng connectionName) { ConnectionSsortingngSettings cs= ConfigurationManager.ConnectionSsortingngs[connectionName]; DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName); DbConnection conn = factory.CreateConnection(); conn.ConnectionSsortingng = cs.ConnectionSsortingng; conn.Open(); return conn; } 

Ensuite, j’ai commencé à examiner la documentation du framework .NET pour voir quel était le comportement documenté de différentes choses et voir si je pouvais les gérer.

Par exemple:

 ConfigurationManager.ConnectionSsortingngs... 

La documentation indique que l’appel de ConnectionSsortingngs lève une ConfigurationErrorException s’il n’a pas pu récupérer la collection. Dans ce cas, je ne peux rien faire pour gérer cette exception, alors je vais laisser tomber.


La prochaine partie est l’indexation réelle des ConnectionSsortingngs pour trouver connectionName :

 ...ConnectionSsortingngs[connectionName]; 

Dans cette instance, la documentation ConnectionSsortingngs indique que la propriété retournera null si le nom de la connexion est introuvable. Je peux vérifier cela et lancer une exception pour laisser quelqu’un de haut en bas qu’il a donné un nom de connexion invalide:

 ConnectionSsortingngSettings cs= ConfigurationManager.ConnectionSsortingngs[connectionName]; if (cs == null) throw new ArgumentException("Could not find connection ssortingng \""+connectionName+"\""); 

Je répète le même exercice avec:

 DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName); 

La méthode GetFactory n’a pas de documentation sur ce qui se passe si une fabrique pour le ProviderName spécifié n’a pas été trouvée. Il n’est pas documenté de renvoyer null , mais je peux toujours être défensif et vérifier la valeur null:

 DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName); if (factory == null) throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\""); 

Suivant est la construction de l’object DbConnection:

 DbConnection conn = factory.CreateConnection() 

Encore une fois, la documentation ne dit pas ce qui se passe si elle ne peut pas créer de connexion, mais encore une fois, je peux rechercher un object de retour nul:

 DbConnection conn = factory.CreateConnection() if (conn == null) throw new Exception.Create("Connection factory did not return a connection object"); 

Ensuite, vous définissez une propriété de l’object Connection:

 conn.ConnectionSsortingng = cs.ConnectionSsortingng; 

Les documents ne disent pas ce qui se passe s’il n’a pas pu définir la chaîne de connexion. Jette-t-il une exception? Est-ce qu’il l’ignore? Comme pour la plupart des exceptions, en cas d’erreur lors de la définition du ConnectionSsortingng d’une connexion, je ne peux rien faire pour le récupérer. Donc je ne ferai rien.


Et enfin, en ouvrant la connexion à la firebase database:

 conn.Open(); 

La méthode Open de DbConnection est abstraite. Il appartient donc au fournisseur de DbConnection de décider des exceptions. Dans la documentation abstraite sur les méthodes ouvertes, il n’ya pas non plus d’indications sur ce que je peux espérer en cas d’erreur. Si une erreur s’est produite lors de la connexion, je sais que je ne peux pas le gérer – je devrai le laisser bouillonner là où l’appelant peut montrer une interface utilisateur à l’utilisateur et le laisser réessayer.


Après

 public DbConnection GetConnection(Ssortingng connectionName) { //Get the connection ssortingng info from web.config ConnectionSsortingngSettings cs= ConfigurationManager.ConnectionSsortingngs[connectionName]; //documented to return null if it couldn't be found if (cs == null) throw new ArgumentException("Could not find connection ssortingng \""+connectionName+"\""); //Get the factory for the given provider (eg "System.Data.SqlClient") DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName); //Undefined behaviour if GetFactory couldn't find a provider. //Defensive test for null factory anyway if (factory == null) throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\""); //Have the factory give us the right connection object DbConnection conn = factory.CreateConnection(); //Undefined behaviour if CreateConnection failed //Defensive test for null connection anyway if (conn == null) throw new Exception("Could not obtain connection from factory"); //Knowing the connection ssortingng, open the connection conn.ConnectionSsortingng = cs.ConnectionSsortingng; conn.Open() return conn; } 

Résumé

Donc, ma fonction à quatre lignes est devenue 12 lignes et a nécessité 5 minutes de recherche de documentation. En fin de compte, j’ai attrapé un cas où une méthode est autorisée à renvoyer null. Mais en pratique, tout ce que j’ai fait était de convertir une exception de violation d’access (si j’essaie d’appeler des méthodes sur une référence null) dans une exception InvalidArgumentException .

J’attrape aussi deux cas possibles où il pourrait y avoir des objects retour nuls ; mais encore une fois je n’ai échangé qu’une exception pour une autre.

Sur le plan positif, il a rencontré deux problèmes et a expliqué ce qui s’est passé dans le message d’exception, plutôt que les mauvaises choses qui se sont produites (c’est-à-dire que la responsabilité s’arrête ici).

Mais est-ce que ça en vaut la peine? Est-ce excessif? Cette programmation défensive a-t-elle mal tourné?

Vérifier manuellement une configuration et lancer une exception ne vaut pas mieux que de laisser le cadre jeter l’exception si la configuration est manquante. Vous ne faites que dupliquer les vérifications de précondition qui se produisent à l’intérieur des méthodes du framework, et vous obtenez un code détaillé sans aucun avantage. (En fait, vous supprimez peut-être des informations en lançant tout en tant que classe d’exception de base. Les exceptions émises par la structure sont généralement plus spécifiques.)

Edit: Cette réponse semble quelque peu controversée, donc un peu d’élaboration: la programmation défensive signifie “se préparer à l’inattendu” (ou “être paranoïaque”) et l’une des façons de le faire est de faire beaucoup de vérifications préalables. Dans de nombreux cas, il s’agit d’une bonne pratique. Cependant, toutes les pratiques doivent être évaluées en fonction des avantages.

Par exemple, il ne fournit aucun avantage à lancer une exception “Impossible d’obtenir une connexion depuis l’usine”, car il ne dit rien sur la raison pour laquelle le fournisseur n’a pu être obtenu – et la toute dernière ligne jettera une exception quand le fournisseur est nul. Le coût de la vérification de la précondition (en termes de temps de développement et de complexité du code) n’est donc pas justifié.

D’autre part, la vérification pour vérifier que la configuration de la chaîne de connexion est présente peut être justifiée, car l’exception peut aider le développeur à résoudre le problème. L’exception null que vous obtiendrez dans la ligne suivante n’indique pas le nom de la chaîne de connexion manquante, donc votre vérification de précondition fournit une valeur. Si votre code fait partie d’un composant par exemple, la valeur est assez grande, car l’utilisateur du composant peut ne pas savoir quelles configurations le composant requirejs.

Une interprétation différente de la programmation défensive est que vous ne devez pas simplement détecter les conditions d’erreur, vous devez également essayer de vous remettre de toute erreur ou exception pouvant survenir. Je ne crois pas que ce soit une bonne idée en général.

Fondamentalement, vous ne devez gérer que les exceptions pour lesquelles vous pouvez faire quelque chose. Les exceptions que vous ne pouvez pas récupérer de toute façon doivent simplement être transmises au gestionnaire de niveau supérieur. Dans une application Web, le gestionnaire de niveau supérieur affiche probablement une page d’erreur générique. Mais il n’y a pas grand chose à faire dans la plupart des cas, si la firebase database est hors ligne ou si une configuration cruciale est manquante.

Certains cas où ce type de programmation défensive a du sens, c’est si vous acceptez la saisie de l’utilisateur et que cette entrée peut entraîner des erreurs. Si, par exemple, l’utilisateur fournit une URL en entrée et que l’application tente d’extraire quelque chose de cette URL, il est très important de vérifier que l’URL semble correcte et que vous gérez toute exception pouvant résulter de la requête. Cela vous permet de fournir de précieux commentaires à l’utilisateur.

Eh bien, cela dépend de votre public.

Si vous écrivez du code de bibliothèque que vous pensez être utilisé par beaucoup d’autres personnes, qui ne vous parleront pas de la manière de l’utiliser, ce n’est pas exagéré. Ils apprécieront votre effort.

(Cela dit, si vous faites cela, je vous suggère de définir de meilleures exceptions que simplement System.Exception, pour faciliter la tâche des personnes qui souhaitent intercepter certaines de vos exceptions, mais pas d’autres.)

Mais si vous allez simplement l’utiliser vous-même (ou vous et votre ami), alors évidemment, c’est exagéré, et cela vous blesse probablement en rendant votre code moins lisible.

J’aurais aimé que mon équipe code comme ça. La plupart des gens n’obtiennent même pas l’intérêt de la programmation défensive. Le mieux, c’est de regrouper toute la méthode dans une instruction try catch et de laisser toutes les exceptions être gérées par le bloc d’exception générique!

Chapeau à toi Ian. Je peux comprendre votre dilemme. J’ai moi-même vécu la même chose. Mais ce que vous avez probablement aidé certains développeurs à passer plusieurs heures à dénigrer le clavier.

Rappelez-vous, lorsque vous utilisez un API Framework .net, à quoi vous attendez-vous? Qu’est-ce qui semble naturel? Faites de même avec votre code.

Je sais que ça prend du temps. Mais alors la qualité a un coût.

PS: Vous n’avez vraiment pas à gérer toutes les erreurs et à lancer une exception personnalisée. N’oubliez pas que votre méthode ne sera utilisée que par d’autres développeurs. Ils devraient être capables de trouver eux-mêmes des exceptions au cadre commun. Cela ne vaut pas la peine.

Votre exemple “avant” a la particularité d’être clair et concis.

Si quelque chose ne va pas, une exception sera éventuellement lancée par le framework. Si vous ne pouvez rien faire contre cette exception, vous pouvez tout aussi bien la laisser se propager dans la stack d’appels.

Il y a des moments, cependant, où une exception est lancée profondément dans le cadre, ce qui ne permet pas vraiment de savoir quel est le problème réel. Si votre problème est que vous n’avez pas de chaîne de connexion valide, mais que la structure génère une exception telle que “utilisation non valide de null”, il est parfois préférable d’attraper l’exception et de la renvoyer avec un message plus significatif.

Je vérifie souvent la présence d’objects nuls, car j’ai besoin d’un object réel pour fonctionner, et si l’object est vide, l’exception qui est lancée sera pour le moins oblique. Mais je ne vérifie que les objects nuls si je sais que c’est ce qui va se passer. Certaines fabriques d’objects ne renvoient pas d’objects null. ils jettent une exception à la place et la vérification de null sera inutile dans ces cas.

Je ne pense pas que j’écrirais cette logique de vérification de référence nulle – du moins, pas comme vous l’avez fait.

Mes programmes qui obtiennent des parameters de configuration à partir du fichier de configuration de l’application vérifient tous ces parameters au démarrage. Je construis généralement une classe statique pour contenir les parameters et référencer les propriétés de cette classe (et non le ConfigurationManager ) ailleurs dans l’application. Il y a deux raisons à cela.

Premièrement, si l’application n’est pas configurée correctement, cela ne fonctionnera pas. Je le saurais au moment où le programme lit le fichier de configuration à un moment donné lorsque j’essaie de créer une connexion à une firebase database.

Deuxièmement, vérifier la validité de la configuration ne devrait pas vraiment concerner les objects qui dépendent de la configuration. Si vous avez déjà effectué ces vérifications, il est inutile de faire des vérifications partout dans votre code. (Il existe des exceptions à cela, notamment – par exemple, les applications de longue durée où vous devez pouvoir modifier la configuration pendant que le programme est en cours d’exécution et avoir ces changements dans le comportement du programme. Accédez à ConfigurationManager chaque fois que vous avez besoin d’un paramètre.)

Je ne ferais pas non plus la vérification des références NULL sur les GetFactory et CreateConnection . Comment écririez-vous un cas de test pour exercer ce code? Vous ne pouvez pas, parce que vous ne savez pas comment rendre ces méthodes nulles – vous ne savez même pas qu’il est possible de rendre ces méthodes nulles. Donc, vous avez remplacé un problème – votre programme peut lancer une NullReferenceException dans des conditions que vous ne comprenez pas – avec une autre, plus importante: dans ces mêmes conditions mystérieuses, votre programme exécutera du code que vous n’avez pas testé.

Votre documentation de méthode est manquante. 😉

Chaque méthode a des parameters d’entrée et de sortie définis et un comportement résultant défini. Dans votre cas, quelque chose comme: “Retourne une connexion ouverte valide en cas de succès, sinon retourne null (ou lance une XXXException, comme vous le souhaitez). En gardant ce comportement à l’esprit, vous pouvez maintenant décider de la défensive à programmer.

  • Si votre méthode doit exposer des informations détaillées, pourquoi et ce qui a échoué, faites-le comme vous l’avez fait et vérifiez et attrapez tout et tout et renvoyez les informations appropriées.

  • Mais si vous êtes simplement intéressé par une DBConnection ouverte ou tout simplement null (ou une exception définie par l’utilisateur) en cas d’échec, alors il vous suffit d’envelopper tout dans try / catch et de retourner null (ou une exception) en cas d’erreur.

Donc, je dirais que cela dépend du comportement de la méthode et des résultats attendus.

En général, les exceptions spécifiques à une firebase database doivent être interceptées et renvoyées comme quelque chose de plus général, comme une exception DataAccessFailure (hypothétique). Dans la plupart des cas, un code de niveau supérieur n’a pas besoin de savoir que vous lisez les données de la firebase database.

Une autre raison de piéger rapidement ces erreurs est qu’elles incluent souvent des détails de firebase database dans leurs messages, comme “Pas de table de ce type: ACCOUNTS_BLOCKED” ou “Clé utilisateur non valide: 234234”. Si cela se propage à l’utilisateur final, c’est mauvais à plusieurs égards:

  1. déroutant
  2. brèche de sécurité potentielle
  3. gênant pour l’image de votre entreprise (imaginez un client lisant un message d’erreur avec une grammaire grossière)

Ma règle générale est la suivante:

Ne pas intercepter si le message de l’exception renvoyée est pertinent pour l’appelant.

Ainsi, NullReferenceException n’a pas de message pertinent, je vérifierais si elle est nulle et lance une exception avec un meilleur message. ConfigurationErrorException est pertinente, donc je ne l’attrape pas.

La seule exception est si le “contrat” ​​de GetConnection ne récupère pas nécessairement la chaîne de connexion dans un fichier de configuration.

Si c’est le cas, GetConnection DEVRAIT avoir un contrat avec une exception personnalisée qui dit que la connexion n’a pas pu être récupérée, vous pouvez alors envelopper ConfigurationErrorException dans votre exception personnalisée.

L’autre solution consiste à spécifier que GetConnection ne peut pas lancer (mais peut renvoyer null), puis vous ajoutez un “gestionnaire d’exception” à votre classe.

Je l’aurais codé exactement comme votre première tentative.

Cependant, l’utilisateur de cette fonction protégerait l’object de connexion avec un bloc USING.

Je n’aime vraiment pas traduire des exceptions comme vos autres versions, car cela rend très difficile la recherche de la raison de cette rupture (firebase database en panne? Vous n’avez pas la permission de lire le fichier de configuration, etc.?).

La version modifiée n’ajoute pas beaucoup de valeur, à condition que l’application dispose d’un gestionnaire AppDomain.UnexpectedException qui vide la chaîne exception.InnerException et tous les suivis de stack dans un fichier journal (ou même mieux, capture un minidump), puis appelez Environment.FailFast .

À partir de ces informations, il sera assez simple d’identifier ce qui n’a pas fonctionné, sans avoir à compliquer le code de la méthode avec une vérification supplémentaire des erreurs.

Notez qu’il est préférable de gérer AppDomain.UnexpectedException et d’appeler Environment.FailFast au lieu d’avoir un try/catch (Exception x) niveau supérieur try/catch (Exception x) car avec cette dernière, la cause d’origine du problème sera probablement masquée par d’autres exceptions.

En effet, si vous rencontrez une exception, tous les blocs finally ouverts seront exécutés et émettront probablement plus d’exceptions, ce qui masquera l’exception originale (ou pire, supprimera les fichiers pour tenter de rétablir certains états, éventuellement les mauvais fichiers). peut-être même des fichiers importants). Vous ne devriez jamais intercepter une exception indiquant un état de programme invalide que vous ne savez pas comment gérer, même dans un bloc try/catch fonction main niveau supérieur. Manipuler AppDomain.UnexpectedException et appeler Environment.FailFast est une AppDomain.UnexpectedException différente (et plus souhaitable) car elle empêche les blocs de fonctionner et si vous essayez d’arrêter votre programme et de consigner des informations utiles sans endommager davantage, vous certainement pas envie de lancer vos derniers blocs.

N’oubliez pas de vérifier les exceptions OutOfMemoryExceptions … vous savez, cela pourrait arriver.

Les changements d’Iain me semblent sensibles.

Si j’utilise un système et que je l’utilise de manière incorrecte, je souhaite obtenir le maximum d’informations sur les abus. Par exemple, si j’oublie d’insérer des valeurs dans une configuration avant d’appeler une méthode, je veux une exception InvalidOperationException avec un message détaillant mon erreur, pas une exception KeyNotFoundException / NullReferenceException.

Tout est question de contexte IMO. J’ai vu des messages d’exception assez impénétrables dans mon temps, mais d’autres fois, l’exception par défaut venant du framework est parfaitement correcte.

En général, je pense qu’il est préférable de faire preuve de prudence, en particulier lorsque vous écrivez quelque chose qui est fortement utilisé par d’autres personnes ou généralement dans le graphique d’appel où l’erreur est plus difficile à diagnostiquer.

J’essaie toujours de me souvenir qu’en tant que développeur d’un morceau de code ou d’un système, je suis mieux placé pour diagnostiquer les échecs que quelqu’un qui l’utilise. Parfois, quelques lignes de code de vérification + un message d’exception personnalisé peuvent cumuler des heures de débogage (et vous simplifier la vie car vous n’êtes pas entraîné sur la machine de quelqu’un d’autre pour résoudre le problème).

A mes yeux, votre “après” échantillon n’est pas vraiment défensif. Parce que défensif serait de vérifier les parties sous votre contrôle, ce qui serait l’argument connectionName (vérifiez la valeur null ou vide, et lancez une ArgumentNullException).

Pourquoi ne pas diviser la méthode que vous avez après avoir ajouté toute la programmation défensive? Vous avez un tas de blocs logiques distincts qui justifient des méthodes séparées. Pourquoi? Car alors, vous encapsulez la logique qui appartient ensemble, et votre méthode publique résultante ne fait que connecter ces blocs de la bonne manière.

Quelque chose comme ceci (édité dans l’éditeur SO, donc pas de vérification de syntaxe / compilateur. Peut ne pas comstackr ;-))

 private ssortingng GetConnectionSsortingng(Ssortingng connectionName) { //Get the connection ssortingng info from web.config ConnectionSsortingngSettings cs= ConfigurationManager.ConnectionSsortingngs[connectionName]; //documented to return null if it couldn't be found if (cs == null) throw new ArgumentException("Could not find connection ssortingng \""+connectionName+"\""); return cs; } private DbProviderFactory GetFactory(Ssortingng ProviderName) { //Get the factory for the given provider (eg "System.Data.SqlClient") DbProviderFactory factory = DbProviderFactories.GetFactory(ProviderName); //Undefined behaviour if GetFactory couldn't find a provider. //Defensive test for null factory anyway if (factory == null) throw new Exception("Could not obtain factory for provider \""+ProviderName+"\""); return factory; } public DbConnection GetConnection(Ssortingng connectionName) { //Get the connection ssortingng info from web.config ConnectionSsortingngSettings cs = GetConnectionSsortingng(connectionName); //Get the factory for the given provider (eg "System.Data.SqlClient") DbProviderFactory factory = GetFactory(cs.ProviderName); //Have the factory give us the right connection object DbConnection conn = factory.CreateConnection(); //Undefined behaviour if CreateConnection failed //Defensive test for null connection anyway if (conn == null) throw new Exception("Could not obtain connection from factory"); //Knowing the connection ssortingng, open the connection conn.ConnectionSsortingng = cs.ConnectionSsortingng; conn.Open() return conn; } 

PS: Ce n’est pas un refactor complet, seulement les deux premiers blocs.