Comment devrais-je programmer de manière défensive? [fermé]

Question

Je travaillais avec une petite routine utilisée pour créer une connexion à une base de données:

Avant

public DbConnection GetConnection(String connectionName)
{
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);
   DbConnection conn = factory.CreateConnection();
   conn.ConnectionString = cs.ConnectionString;
   conn.Open();

   return conn;
}

Ensuite, j'ai commencé à regarder dans la documentation du framework .NET pour voir quel était le comportement documenté de diverses choses et voir si je pouvais les gérer.

Par exemple:

ConfigurationManager.ConnectionStrings...

La documentation indique que l'appel < strong> ConnectionStrings lève une ConfigurationErrorException si il n'a pas pu récupérer la collection. Dans ce cas, je ne peux rien faire pour gérer cette exception, je vais donc le laisser aller.

La partie suivante correspond à l'indexation réelle des chaînes de connexion pour rechercher nom de connexion :

...ConnectionStrings[connectionName];

Dans ce cas, la documentation de ConnectionStrings indique que la propriété sera renvoyée null si le nom de la connexion est introuvable. Je peux vérifier si cela se produit et lancer une exception pour permettre à une personne en haut d'avoir donné un nom de connexion invalide:

ConnectionStringSettings cs= 
      ConfigurationManager.ConnectionStrings[connectionName];
if (cs == null)
   throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

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

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

La méthode GetFactory ne contient aucune documentation sur ce qui se passe si une fabrique pour le ProviderName spécifié, introuvable. Il n'est pas documenté de retourner null, mais je peux toujours être défensif et rechercher pour null:

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

Vient ensuite la construction de l'objet DbConnection:

DbConnection conn = factory.CreateConnection()

Encore une fois, la documentation ne le fait pas. ne dites pas ce qui se passe s’il ne parvient pas à créer une connexion, mais là encore, je peux rechercher un objet 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'objet Connexion:

conn.ConnectionString = cs.ConnectionString;

La documentation ne dit pas ce qui se passe si elle ne peut pas définir la chaîne de connexion. Est-ce qu'il jette une exception? Est-ce qu'il l'ignore? Comme dans la plupart des cas exceptionnels, si une erreur s'est produite lors de la tentative de définition du ConnectionString d'une connexion, il n'y a rien que je puisse faire pour le récupérer. Donc, je ne ferai rien.

Et enfin, ouvrir la connexion à la base de données:

conn.Open();

La Méthode ouverte de DbConnection est abstrait, il appartient donc à tout fournisseur descendant de DbConnection de décider des exceptions qu'il génère. Il n’existe pas non plus de documentation abstraite sur les méthodes ouvertes sur ce que je peux espérer voir se produire s’il ya une erreur. Si une erreur s'est produite lors de la connexion, je sais que je ne peux pas la gérer. Je dois la laisser déborder de manière à ce que l'appelant puisse afficher une interface utilisateur à l'utilisateur et le laisser essayer à nouveau.

Après

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
    if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

   //Get the factory for the given provider (e.g. "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 string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

Résumé

Ainsi, ma fonction sur quatre lignes est devenue 12 lignes et a nécessité 5 minutes de consultation de la 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, c’est de convertir une exception de violation d’accès (si j’essaie d’appeler des méthodes sur une référence nulle) en une InvalidArgumentException .

J'attrape également deux cas possibles où il pourrait y avoir null des objets de retour; mais encore une fois, je n'ai échangé qu'une exception contre une autre.

Du côté positif, deux problèmes ont été détectés et expliquent ce qui est arrivé dans le message d’exception, plutôt que les mauvaises choses à venir (c’est-à-dire que la responsabilité s’arrête là)

Mais cela en vaut-il la peine? Est-ce exagéré? Cette programmation défensive a-t-elle mal tourné?

Answer 1

Vérifier manuellement une configuration et lever une exception n'est pas mieux que de laisser le framework lever l'exception si la configuration est manquante. Vous ne faites que dupliquer des vérifications de précondition qui se produisent de toute façon dans les méthodes du framework, et cela rend votre code verbeux sans aucun avantage. (En fait, vous pourriez peut-être supprimer des informations en lançant tout en tant que classe d'exception de base. Les exceptions émises par le cadre sont généralement plus spécifiques.)

Éditer: Cette réponse semble être quelque peu controversée, alors un peu de précision: une programmation défensive signifie & "Préparez-vous à l’imprévu &"; (ou & "; soyez paranoïaque &";) et l’une des façons de le faire est de faire de nombreuses vérifications de précondition. Dans de nombreux cas, il s’agit là d’une bonne pratique. Toutefois, comme pour toutes les pratiques, les coûts doivent être comparés aux avantages.

Par exemple, lancer un & "" Impossible d'obtenir une connexion de la fabrique & "; exception, puisqu'il ne dit rien sur pourquoi le fournisseur n'a pas pu être obtenu - et que la ligne suivante lève de toute façon une exception si le fournisseur est null. Le coût de la vérification préalable (en termes de temps de développement et de complexité du code) n'est donc pas justifié.

D'autre part, la vérification de la présence de la configuration de la chaîne de connexion peut être justifiée, car cette exception peut aider le développeur à résoudre le problème. L'exception null que vous obtiendrez dans la ligne suivante n'indiquera néanmoins pas le nom de la chaîne de connexion manquante. Votre vérification de condition préalable fournit donc une certaine 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 requiert.

Selon une interprétation différente de la programmation défensive, vous ne devez pas uniquement détecter les conditions d'erreur, vous devez également essayer de récupérer toute erreur ou exception susceptible de se produire. Je ne crois pas que ce soit une bonne idée en général.

En principe, vous ne devez gérer que les exceptions pour lesquelles vous pouvez faire quelque chose. Les exceptions pour lesquelles 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 uniquement une page d'erreur générique. Mais il n’ya pas grand-chose à faire dans la plupart des cas, si la base de données est hors ligne ou si une configuration cruciale est manquante.

Dans certains cas, ce type de programmation défensive a du sens, c’est si vous acceptez les entrées de l’utilisateur, ce qui peut entraîner des erreurs. Si, par exemple, l'utilisateur fournit une URL en tant qu'entrée et que l'application essaie d'extraire quelque chose de cette URL, il est très important que vous vérifiiez que l'URL semble correcte et que vous gérez les exceptions pouvant résulter de la demande. Cela vous permet de fournir de précieux commentaires à l'utilisateur.

Answer 2

Cela dépend de votre public cible.

Si vous écrivez du code de bibliothèque que vous vous attendez à utiliser avec de nombreuses autres personnes, qui ne vous diront pas comment l'utiliser, alors ce n'est pas excessif. Ils apprécieront vos efforts.

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

Mais si vous ne l'utilisez que vous-même (ou votre copain), il est évident que c'est exagéré et qu'il vous blesse probablement en fin de compte en rendant votre code moins lisible.

Answer 3

J'aimerais que mon équipe puisse coder de la sorte. La plupart des gens ne comprennent même pas l'intérêt de la programmation défensive. Le mieux est d’envelopper la méthode dans une instruction try et de laisser toutes les exceptions gérées par le bloc d’exception générique!

Chapeau à toi Ian. Je peux comprendre votre dilemme. J'ai vécu la même chose moi-même. Mais ce que vous avez fait a probablement aidé un développeur à effectuer plusieurs heures de frappe au clavier.

N'oubliez pas que, lorsque vous utilisez une API de structure .net, qu'attendez-vous? Qu'est-ce qui semble naturel? Faites la même chose avec votre code.

Je sais que cela 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. Rappelez-vous que votre méthode ne sera utilisée que par d'autres développeurs. Ils devraient pouvoir déterminer eux-mêmes les exceptions du cadre commun. CELA n'en vaut pas la peine.

Answer 4

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

Si quelque chose ne va pas, une exception sera éventuellement levée par le framework. Si vous ne pouvez rien faire à propos de l'exception, vous pouvez également la laisser se propager dans la pile d'appels.

Cependant, il arrive parfois qu'une exception soit lancée profondément dans le cadre, qui n'éclaire pas vraiment le problème. Si votre problème est que vous n'avez pas de chaîne de connexion valide, mais que le framework lève une exception comme & "Utilisation non valide de null, &"; alors, parfois, il est préférable d’attraper l’exception et de la relancer avec un message plus significatif.

Je vérifie beaucoup les objets nuls, car j’ai besoin d’un objet à utiliser, et si cet objet est vide, l’exception levée sera oblique, pour le moins. Mais je ne vérifie les objets nuls que si je sais que c'est ce qui se produira. Certaines fabriques d'objets ne renvoient pas d'objets nuls; à la place, ils lèvent une exception et la recherche de null sera inutile dans ces cas.

Answer 5

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 dont les paramètres de configuration sont issus du fichier de configuration de l'application vérifient tous ces paramètres au démarrage. Je construis habituellement une classe statique pour contenir les paramètres 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.

D'abord, si l'application n'est pas configurée correctement, elle ne fonctionnera pas. Je préférerais le savoir au moment où le programme lit le fichier de configuration que plus tard, lorsque j'essaie de créer une connexion à une base de données.

Deuxièmement, la vérification de la validité de la configuration ne devrait pas vraiment être une préoccupation des objets qui dépendent de la configuration. Il est inutile de faire vous-même insérer des contrôles partout dans votre code si vous avez déjà effectué ces contrôles à l'avance. (Il existe certainement des exceptions à cette règle - par exemple, les applications de longue durée pour lesquelles vous devez pouvoir modifier la configuration pendant l'exécution du programme et pour que ces modifications soient reflétées dans le comportement du programme; dans de tels programmes, vous devrez accédez à la GetFactory chaque fois que vous avez besoin d'un paramètre.)

Je ne ferais pas non plus la vérification de référence nulle sur les appels CreateConnection et NullReferenceException. Comment écririez-vous un scénario de test pour exercer ce code? Vous ne pouvez pas, car vous ne savez pas comment faire en sorte que ces méthodes renvoient null - vous ne savez même pas qu'il est possible de faire renvoyer ces méthodes par null. Vous avez donc remplacé un problème - votre programme peut lancer un <=> dans des conditions incompréhensibles - par un autre, plus important: dans ces mêmes conditions mystérieuses, votre programme exécutera un code que vous n'avez pas testé.

Answer 6

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

  

ne pas attraper si le message de la   exception levée est pertinente pour la   appelant.

Ainsi, NullReferenceException n’a pas de message pertinent, je voudrais vérifier si elle est nulle et lancer une exception avec un meilleur message. ConfigurationErrorException est pertinent, 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 indiquant que la connexion n'a pas pu être extraite, vous pouvez alors envelopper la 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 & "ExceptionHandler &"; à votre classe.

Answer 7

La documentation de votre méthode est manquante. ; -)

Chaque méthode a des paramètres d'entrée et de sortie définis et un comportement résultant défini. Dans votre cas, quelque chose comme: & Quot; Retourne une connexion ouverte valide en cas de succès, sinon renvoie null (ou lève une XXXException, comme vous le souhaitez). En gardant ce comportement à l’esprit, vous pouvez maintenant décider de la manière dont vous devriez programmer la défense.

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

• Mais, si vous êtes simplement intéressé par une connexion DBC ouverte ou si null (ou une exception définie par l'utilisateur) en cas d'échec, enroulez tout dans un try / catch et retournez null (ou une exception) en cas d'erreur, et l'objet else.

Donc, je dirais que cela dépend du comportement de la méthode et du résultat attendu.

Answer 8

En général, les exceptions spécifiques à une base de données doivent être capturées et redéfinies comme quelque chose de plus général, comme une exception (hypothétique) DataAccessFailure. Dans la plupart des cas, le code de niveau supérieur n'a pas besoin de savoir que vous lisez les données de la base de données.

Une autre raison de piéger ces erreurs rapidement est qu’elles incluent souvent dans leurs messages certaines informations relatives à la base de données, comme & "Aucune table de ce type: ACCOUNTS_BLOCKED &"; ou " Clé utilisateur invalide: 234234 " ;. Si cela se propage à l'utilisateur final, c'est mauvais de plusieurs manières:

1. Confus.
2. Faille de sécurité potentielle.
3. Embarrassant pour l'image de votre entreprise (imaginez un client lisant un message d'erreur avec une grammaire grossière).

Answer 9

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

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

Je n'aime vraiment pas traduire les exceptions comme le font vos autres versions, car il est très difficile de savoir pourquoi elles se sont cassées (base de données en panne? Vous n'êtes pas autorisé à lire le fichier de configuration, etc.?).

Answer 10

La version modifiée n'ajoute pas beaucoup de valeur, tant que l'application a un gestionnaire AppDomain.UnexpectedException qui vide la chaîne exception.InnerException et toutes les traces de pile dans un fichier journal quelconque (ou mieux, capture un minidump ) puis appelez Environment.FailFast.

À partir de ces informations, il sera assez simple d'identifier ce qui s'est mal passé, sans avoir à compliquer le code de la méthode avec une vérification d'erreur supplémentaire.

Notez qu'il est préférable de gérer try/catch (Exception x) et d'appeler finally au lieu d'avoir un main niveau supérieur car avec ce dernier, la cause initiale du problème sera probablement masquée par d'autres exceptions.

En effet, si vous attrapez une exception, tous les try/catch blocs ouverts seront exécutés et renverront probablement plus d'exceptions, ce qui masquera l'exception d'origine (ou pire, ils supprimeront des fichiers dans le but de rétablir un état, peut-être les mauvais fichiers, peut-être même des fichiers importants). Vous ne devez jamais intercepter une exception qui indique un état de programme non valide que vous ne savez pas comment gérer, même dans un bloc de niveau supérieur <=> function <=>. Manipuler <=> et appeler <=> constituent une combinaison de poissons différente (et plus souhaitable), car ils empêchent <=> les blocs de fonctionner et si vous essayez d'arrêter votre programme et de consigner certaines informations utiles sans causer d'autres dommages. , vous ne voulez certainement pas exécuter vos <=> blocs.

Answer 11

N'oubliez pas de vérifier les exceptions OutOfMemoryExceptions ... vous savez, cela pourrait se produire.

Answer 12

Les changements de Iain me semblent sensibles.

Si j'utilise un système et que je ne l'utilise pas correctement, je souhaite obtenir le maximum d'informations sur l'utilisation abusive. Par exemple. si j'oublie d'insérer certaines valeurs dans une configuration avant d'appeler une méthode, je souhaite une exception InvalidOperationException avec un message détaillant mon erreur, et non une exception KeyNotFoundException / NullReferenceException.

Tout est une question de contexte, IMO. J'ai vu des messages d'exception assez impénétrables à mon époque, mais d'autres fois, l'exception par défaut provenant 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 qui se trouve au plus profond du graphique des appels, où l'erreur est plus difficile à diagnostiquer.

J'essaie toujours de me rappeler qu'en tant que développeur d'un élément de code ou d'un système, je suis mieux placé pour diagnostiquer les échecs que celui qui l'utilise juste. Il arrive parfois que quelques lignes de code de vérification + un message d'exception personnalisé permettent de gagner de manière cumulative des heures de débogage (et vous simplifient également la vie, car vous n'êtes pas amené à la machine de quelqu'un d'autre pour résoudre le problème).

Answer 13

À mes yeux, votre & "après &"; l'échantillon n'est pas vraiment défensif. Parce que défensif serait de vérifier les pièces sous votre contrôle, ce qui serait l'argument connectionName (recherchez null ou empty et lancez une ArgumentNullException).

Answer 14

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 distinctes. Pourquoi? Dans ce cas, vous encapsulerez la logique qui appartient ensemble et votre méthode publique résultante connecte simplement ces blocs de la bonne manière.

Quelque chose comme ça (édité dans l'éditeur de SO, donc aucune vérification de syntaxe / compilateur. Peut ne pas compiler; -))

private string GetConnectionString(String connectionName)
{

   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
   if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");
   return cs;
}

private DbProviderFactory GetFactory(String ProviderName)
{
   //Get the factory for the given provider (e.g. "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(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs = GetConnectionString(connectionName);

   //Get the factory for the given provider (e.g. "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 string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

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