c# - ¿Qué tan defensivamente debería programar?

exception-handling defensive-programming

Question

Estaba trabajando con una pequeña rutina que se utiliza para crear una conexión de base de datos:

antes de

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;
}

Luego comencé a buscar en la documentación de .NET Framework, para ver cuál es el comportamiento documentado de varias cosas y ver si puedo manejarlas.

Por ejemplo:

ConfigurationManager.ConnectionStrings...

La documentation dice que llamar a ConnectionStrings arroja una excepción ConfigurationErrorException si no pudo recuperar la colección. En este caso, no hay nada que pueda hacer para manejar esta excepción, así que lo dejaré ir.

La siguiente parte es la indexación real de ConnectionStrings para encontrar connectionName :

...ConnectionStrings[connectionName];

En este caso, la documentación de ConnectionStrings dice que la propiedad devolverá nulo si no se pudo encontrar el nombre de la conexión. Puedo verificar que esto ocurra, y lanzar una excepción para dejar a alguien en alto que dieron un connectionName inválido:

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

Repito el mismo ejercicio con:

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

El método GetFactory no tiene documentación sobre lo que sucede si no se puede encontrar una fábrica para el Nombre del ProviderName especificado. No está documentado devolver null , pero aún puedo estar a la defensiva y verificar nulo:

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

A continuación está la construcción del objeto DbConnection:

DbConnection conn = factory.CreateConnection()

De nuevo, la documentation no dice qué sucede si no puede crear una conexión, pero de nuevo puedo verificar si hay un objeto de devolución nulo:

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

A continuación, establece una propiedad del objeto Connection:

conn.ConnectionString = cs.ConnectionString;

Los documentos no dicen qué sucede si no puede establecer la cadena de conexión. ¿Lanza una excepción? ¿Lo ignora? Como con la mayoría de las excepciones, si hubo un error al intentar establecer ConnectionString de una conexión, no hay nada que pueda hacer para recuperarlo. Entonces no haré nada.

Y finalmente, abriendo la conexión a la base de datos:

conn.Open();

El método Open de DbConnection es abstracto, por lo que depende de cualquier proveedor que esté descendiendo desde DbConnection para decidir qué excepciones arrojan. Tampoco hay orientación en el resumen. Documentación de métodos abiertos sobre lo que puedo esperar que ocurra si hay un error. Si se produjo un error al conectar, sé que no puedo manejarlo, tendré que dejarlo burbujear donde la persona que llama puede mostrarle al usuario una IU, y dejar que lo intenten de nuevo.

Despué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;
}

Resumen

Entonces mi función de cuatro líneas se convirtió en 12 líneas y requirió 5 minutos de búsqueda de documentación. Al final atrapé un caso en el que se permite que un método devuelva nulo. Pero en la práctica todo lo que hice fue convertir una excepción de violación de acceso (si intento invocar métodos en una referencia nula) en una InvalidArgumentException .

también atrapo dos casos posibles donde podría haber objetos de devolución nulos ; pero nuevamente cambié una excepción por otra.

En el lado positivo, atrapó dos problemas y explicó lo que sucedió en el mensaje de excepción, en lugar de que sucedan cosas malas en el camino (es decir, el dinero se detiene aquí)

Pero ¿vale la pena? ¿Es esto exagerado? ¿Esta programación defensiva ha fallado?

Answer 1

¿Por qué no dividir el método que tienes después de agregar toda la programación defensiva? Usted tiene un montón de bloques lógicos distintos que garantizan métodos separados. ¿Por qué? Porque entonces encapsulas la lógica que pertenece, y tu método público resultante simplemente conecta esos bloques de la manera correcta.

Algo así (editado en el editor de SO, por lo que no comprueba la sintaxis / el compilador. Puede que no compile ;-))

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;
}

PD: Este no es un refactor completo, solo hizo los primeros dos bloques.

Answer 2

Bueno, depende de quién sea tu audiencia.

Si está escribiendo un código de biblioteca que espera que sea utilizado por muchas otras personas, que no le hablarán sobre cómo usarlo, entonces no es excesivo. Ellos apreciarán tu esfuerzo.

(Dicho esto, si lo hace, le sugiero que defina excepciones mejores que solo System.Exception, para que sea más fácil para las personas que desean ver algunas de sus excepciones pero no otras).

Pero si lo va a utilizar usted mismo (o usted y su amigo), obviamente es exagerado, y probablemente lo lastime al hacer que su código sea menos legible.

Answer 3

En general, las excepciones específicas de la base de datos se deben capturar y volver a lanzar como algo más general, como la excepción (hipotética) DataAccessFailure . El código de nivel superior en la mayoría de los casos no necesita saber que está leyendo los datos de la base de datos.

Otra razón para atrapar estos errores rápidamente es que a menudo incluyen algunos detalles de la base de datos en sus mensajes, como "No such table: ACCOUNTS_BLOCKED" o "User clave invalid: 234234". Si esto se propaga al usuario final, es malo de varias maneras:

1. confuso
2. posible violación de seguridad
3. vergonzoso para la imagen de su empresa (imagínese un cliente leyendo un mensaje de error con gramática cruda)

Answer 4

En mi opinión, tu muestra de "después" no es realmente defensiva. Porque defensivo sería verificar las partes bajo su control, que sería el argumento connectionName (buscar null o empty, y lanzar ArgumentNullException).

Answer 5

La versión modificada no agrega mucho valor, siempre y cuando la aplicación tenga un controlador AppDomain.UnexpectedException que vuelque la cadena exception.InnerException y todos los rastreos de la pila en un archivo de registro de algún tipo (o mejor aún, capture un minivolcado) y luego llama a Environment.FailFast .

A partir de esa información, será razonablemente sencillo identificar qué fue lo que salió mal, sin la necesidad de complicar el código del método con una comprobación de errores adicional.

Tenga en cuenta que es preferible manejar AppDomain.UnexpectedException y llamar a Environment.FailFast lugar de tener un try/catch (Exception x) nivel superior try/catch (Exception x) porque con este último, la causa original del problema probablemente quedará oculta por otras excepciones.

Esto se debe a que si atrapa una excepción, se ejecutará cualquier bloque finally abierto, y probablemente lanzará más excepciones, que ocultará la excepción original (o peor, eliminarán los archivos en un intento de revertir algún estado, posiblemente los archivos incorrectos, tal vez incluso archivos importantes). Nunca debe detectar una excepción que indique un estado de programa no válido que no sabe cómo manejar, incluso en una función main nivel superior try/catch block. Manejar AppDomain.UnexpectedException y llamar a Environment.FailFast es una caldera de pescado diferente (y más deseable), ya que al final detiene la ejecución de bloques y, si intenta detener su programa y registrar información útil sin causar más daño, Definitivamente no quiero ejecutar tus bloques finally .

Answer 6

Lo habría codificado exactamente como tu primer intento.

Sin embargo, el usuario de esa función protegería el objeto de conexión con un bloque USING.

Realmente no me gusta traducir excepciones como las otras versiones, ya que hace que sea muy difícil averiguar por qué se rompió (¿Base de datos? ¿No tiene permiso para leer el archivo de configuración, etc.?).

Answer 7

Los cambios de Iain me parecen sensatos.

Si estoy usando un sistema y lo uso de manera incorrecta, quiero la máxima información sobre el uso indebido. Por ejemplo, si olvido insertar algunos valores en una configuración antes de llamar a un método, quiero una InvalidOperationException con un mensaje que detalle mi error, no una excepción KeyNotFoundException / NullReferenceException.

Todo se trata de contexto IMO. He visto algunos mensajes de excepción bastante impenetrables en mi tiempo, pero otras veces la excepción predeterminada que proviene del framework está perfectamente bien.

En general, creo que es mejor ser cauteloso, especialmente cuando escribe algo que usan otras personas o, por lo general, en el gráfico de llamadas donde el error es más difícil de diagnosticar.

Siempre trato de recordar que, como desarrollador de un código o sistema, estoy en una mejor posición para diagnosticar fallas que alguien que solo lo está usando. A veces ocurre que unas pocas líneas de código de comprobación + un mensaje de excepción personalizado pueden ahorrar acumulativamente horas de depuración (y también hacen su vida más fácil ya que no se maneja con la máquina de otra persona para solucionar el problema).

Answer 8

Mi regla de oro es:

no detectar si el mensaje de la excepción lanzada es relevante para la persona que llama.

Por lo tanto, NullReferenceException no tiene un mensaje relevante, verificaría si es nulo y arrojaría una excepción con un mensaje mejor. ConfigurationErrorException es relevante, así que no lo veo.

La única excepción es si el "contrato" de GetConnection no recupera la cadena de conexión necesariamente en un archivo de configuración.

Si es el caso, GetConnection DEBERÍA tener un contrato con una excepción personalizada que indique que no se pudo recuperar la conexión, entonces puede ajustar ConfigurationErrorException en su excepción personalizada.

La otra solución es especificar que GetConnection no puede lanzar (pero puede devolver nulo), luego agrega un "ExceptionHandler" a su clase.

Answer 9

No creo que escriba ninguna de esas lógicas de verificación de referencia nula, al menos, no de la manera en que lo ha hecho.

Mis programas que obtienen configuraciones del archivo de configuración de la aplicación comprueban todas esas configuraciones al inicio. Normalmente construyo una clase estática para contener la configuración y hacer referencia a las propiedades de esa clase (y no al ConfigurationManager ) en otra parte de la aplicación. Hay dos razones para esto.

Primero, si la aplicación no está configurada correctamente, no va a funcionar. Prefiero saber esto en el momento en que el programa lee el archivo de configuración que en algún momento en el futuro cuando intento crear una conexión de base de datos.

En segundo lugar, comprobar la validez de la configuración no debe ser realmente la preocupación de los objetos que dependen de la configuración. No tiene sentido hacer comprobaciones de inserción en todas partes a lo largo de su código si ya ha realizado esas comprobaciones por adelantado. (Hay excepciones a esto, por ejemplo, por ejemplo, aplicaciones de larga duración en las que necesita poder cambiar la configuración mientras el programa se está ejecutando y tener esos cambios reflejados en el comportamiento del programa; en programas como este, necesitará vaya a ConfigurationManager cada vez que necesite una configuración).

Tampoco haría la verificación de referencia nula en las llamadas GetFactory y CreateConnection . ¿Cómo escribirías un caso de prueba para ejercer ese código? No puede, porque no sabe cómo hacer que esos métodos devuelvan nulo; ni siquiera sabe que es posible hacer que esos métodos devuelvan nulo. Así que ha reemplazado un problema, su programa puede arrojar una NullReferenceException en condiciones que no comprende, con otro más significativo: en esas mismas condiciones misteriosas, su programa ejecutará un código que no ha probado.

Answer 10

No olvides verificar OutOfMemoryExceptions ... ya sabes, podría suceder.

Answer 11

Ojalá pudiera hacer que mi equipo codificara así. La mayoría de las personas ni siquiera obtienen el punto de la programación defensiva. Lo mejor que pueden hacer es ajustar todo el método en una declaración try catch y permitir que todas las excepciones sean manejadas por el bloque genérico de excepciones.

Sombreros de ti Ian. Puedo entender tu dilema He pasado lo mismo yo mismo. Pero lo que hiciste probablemente ayudó a algunos desarrolladores a varias horas de ataques con el teclado.

Recuerde, cuando está utilizando una API .NET, ¿qué espera de ella? ¿Qué parece natural? Haz lo mismo con tu código.

Sé que toma tiempo. Pero luego la calidad tiene un costo.

PD: Realmente no tienes que manejar todos los errores y lanzar una excepción personalizada. Recuerde que su método solo será utilizado por otros desarrolladores. Deberían ser capaces de descubrir excepciones de marcos comunes ellos mismos. ESO no vale la pena el problema.

Answer 12

Su documentación de método falta. ;-)

Cada método tiene algunos parámetros de entrada y salida definidos y un comportamiento resultante definido. En su caso, algo así como: "Devuelve una conexión válida válida en caso de éxito, de lo contrario devuelve nulo (o arroja una XXXException, como quiera). Teniendo en cuenta este comportamiento, ahora puede decidir qué tan defensivo es programar.

• Si su método debe exponer información detallada de por qué y qué falló, hágalo como lo hizo y verifique y capture todo y todo y devuelva la información adecuada.

• Pero, si solo está interesado en una DBConnection abierta o simplemente nula (o alguna excepción definida por el usuario) en caso de error, simplemente envuelva todo en try / catch y devuelva null (o alguna excepción) en caso de error, y el objeto else.

Entonces, yo diría que depende del comportamiento del método y del resultado esperado.

Answer 13

Su ejemplo "anterior" tiene la distinción de ser claro y conciso.

Si algo está mal, el marco generará una excepción. Si no puede hacer nada respecto de la excepción, también podría dejar que se propague por la pila de llamadas.

Sin embargo, hay ocasiones en que se lanza una excepción dentro del marco que realmente no arroja luz sobre cuál es el problema real. Si su problema es que no tiene una cadena de conexión válida, pero el marco arroja una excepción como "uso no válido de nulo", a veces es mejor detectar la excepción y volver a lanzarla con un mensaje que sea más significativo.

Compruebo mucho los objetos nulos, ya que necesito un objeto real para operar, y si el objeto está vacío, la excepción que se lanza será oblicua, por decir lo menos. Pero solo reviso si hay objetos nulos si sé que eso es lo que ocurrirá. Algunas fábricas de objetos no devuelven objetos nulos; arrojan una excepción en su lugar, y la comprobación de nulos será inútil en estos casos.

Answer 14

Verificar manualmente una configuración y lanzar una excepción no es mejor que dejar que el marco arroje la excepción si falta la configuración. Simplemente está duplicando las verificaciones de condiciones previas que ocurren dentro de los métodos de framework de todos modos, y hace que el código sea detallado sin ningún beneficio. (En realidad, es posible que esté eliminando información lanzando todo como la clase Excepción base. Las excepciones arrojadas por el marco suelen ser más específicas).

Editar: Esta respuesta parece ser un tanto controvertida, por lo que un poco de elaboración: la programación defensiva significa "prepararse para lo inesperado" (o "ser paranoico") y una de las formas de hacerlo es hacer muchas verificaciones de precondición. En muchos casos, esta es una buena práctica, sin embargo, como con todas las prácticas, el costo debe sopesarse con los beneficios.

Por ejemplo, no proporciona ningún beneficio arrojar una excepción "No se pudo obtener la conexión de fábrica", ya que no dice nada sobre por qué no se pudo obtener el proveedor, y la siguiente línea emitirá una excepción de todos modos si el proveedor es nulo. Por lo tanto, el costo de la verificación de precondición (en tiempo de desarrollo y complejidad del código) no está justificado.

Por otro lado, la verificación para verificar que la configuración de la cadena de conexión está presente puede estar justificada, ya que la excepción puede ayudar a decirle al desarrollador cómo resolver el problema. La excepción nula que obtendrá en la siguiente línea de todos modos no indica el nombre de la cadena de conexión que falta, por lo que su verificación previa proporciona algún valor. Si su código es parte de un componente, por ejemplo, el valor es bastante grande, ya que el usuario del componente puede no saber qué configuraciones requiere el componente.

Una interpretación diferente de la programación defensiva es que no solo debe detectar condiciones de error, sino que también debe tratar de recuperarse de cualquier error o excepción que pueda ocurrir. No creo que esta sea una buena idea en general.

Básicamente, solo debes manejar excepciones de las que puedas hacer algo. Las excepciones de las que no se puede recuperar de todos modos, deberían pasarse al manejador de nivel superior. En una aplicación web, el controlador de nivel superior probablemente solo muestre una página de error genérica. Pero realmente no hay mucho que hacer en la mayoría de los casos, si la base de datos está fuera de línea o falta alguna configuración crucial.

Algunos casos en los que ese tipo de programación defensiva tiene sentido, es si acepta la entrada del usuario, y esa entrada puede conducir a errores. Si, por ejemplo, el usuario proporciona una URL como entrada, y la aplicación intenta recuperar algo de esa URL, entonces es muy importante que verifique que la URL se ve correcta y que maneje cualquier excepción que pueda resultar de la solicitud. Esto le permite proporcionar retroalimentación valiosa al usuario.