¿Qué tan malo es el código no relacionado en el bloque try-catch-finally?

11

Esta es una P relacionada: ¿Es el uso de la cláusula finalmente para hacer el trabajo después de devolver un mal estilo / peligroso?

En la Q referenciada, el código finalmente está relacionado con la estructura utilizada y la necesidad de buscar previamente. Mi pregunta es un poco diferente, y creo que es pertinente para el público en general. Mi ejemplo particular es una aplicación winform de C #, pero esto también se aplicaría al uso de C ++ / Java.

Noto bastantes bloques try-catch-finally donde hay mucho código no relacionado con excepciones y manejo / limpieza de excepciones enterrado dentro del bloque. Y admitiré mi predisposición a tener bloques try-catch-finally muy ajustados con el código estrechamente relacionado con la excepción y el manejo. Aquí hay algunos ejemplos de lo que estoy viendo.

Los bloques de prueba tendrán muchas llamadas preliminares y variables que se establecerán antes del código que podrían arrojarse. La información de registro también se configurará y ejecutará en el bloque de prueba.

Finalmente, los bloques tendrán llamadas de formateo de formato / módulo / control (a pesar de que la aplicación está a punto de finalizar, como se expresa en el bloque catch), así como la creación de nuevos objetos como paneles.

Aproximadamente:

    methodName (...)
    {
        tratar
        {
            // Mucho código para el método ...
            // código que podría arrojar ...
            // Mucho más código para el método y un retorno ...
        }
        atrapar (algo)
        {// manejar excepción}
        finalmente
        {
            // alguna limpieza debido a una excepción, cerrando cosas
            // más código para las cosas que se crearon (ignorando que podrían haberse producido excepciones) ...
            // tal vez cree algunos objetos más
        }
    }

El código funciona, por lo que tiene algún valor. No está bien encapsulado y la lógica es un poco complicada. Estoy (dolorosamente) familiarizado con los riesgos de cambiar el código, así como la refactorización, por lo que mi pregunta se reduce a querer conocer la experiencia de los demás con un código estructurado de manera similar.

¿El mal estilo justifica hacer los cambios? ¿Alguien ha sido gravemente quemado por una situación similar? ¿Te gustaría compartir los detalles de esa mala experiencia? ¿Dejarlo porque estoy exagerando y no es tan mal estilo? ¿Obtener los beneficios de mantenimiento de ordenar las cosas?

Comunidad
fuente
La etiqueta "C ++" no pertenece aquí, ya que C ++ no tiene (y no necesita) finally. Todos los buenos usos están cubiertos por RAII / RRID / SBRM (el acrónimo que desee).
David Thornley
2
@DavidThornley: Aparte del ejemplo donde se usa la palabra clave 'finalmente', el resto de la pregunta se aplica perfectamente a C ++. Soy desarrollador de C ++ y esa palabra clave realmente no me confundió. Y teniendo en cuenta que tengo conversaciones similares con mi propio equipo de forma semi-regular, esta pregunta es muy relevante para lo que hacemos con C ++
DXM
@DXM: Tengo problemas para visualizar esto (y sé lo que finallyhace en C #). ¿Cuál es el equivalente de C ++? En lo que estoy pensando es en el código después de catch, y eso aplica lo mismo para el código después de C # finally.
David Thornley
@DavidThornley: el código finalmente es un código que se ejecuta sin importar qué .
orlp
1
@ Nightcracker, eso no es realmente correcto. No se ejecutará si lo haces Environment.FailFast(); es posible que no se ejecute si tiene una excepción no detectada. Y se vuelve aún más complicado si tiene un bloque iterador con el finallyque itera manualmente.
svick

Respuestas:

10

He pasado por una situación muy similar cuando tuve que lidiar con un terrible código heredado de Windows Forms escrito por desarrolladores que claramente no sabía lo que estaban haciendo.

En primer lugar, no estás exagerando. Este es un mal código. Como dijiste, el bloqueo debería ser sobre abortar y prepararte para parar. No es hora de crear objetos (especialmente paneles). Ni siquiera puedo empezar a explicar por qué esto es malo.

Habiendo dicho eso...

Mi primer consejo es: si no está roto, ¡no lo toques!

Si su trabajo es mantener el código, debe hacer todo lo posible para no romperlo. Sé que es doloroso (he estado allí) pero tienes que hacer todo lo posible para no romper lo que ya está funcionando.

Mi segundo consejo es: si tiene que agregar más funciones, intente mantener la estructura de código existente tanto como sea posible para no romper el código.

Ejemplo: si hay una declaración de caso de cambio horrible que cree que podría ser reemplazada por una herencia adecuada, debe tener cuidado y pensar dos veces antes de decidir comenzar a mover las cosas.

Definitivamente encontrará situaciones en las que una refactorización es el enfoque correcto, pero tenga cuidado: es más probable que la refactorización de código introduzca errores . Debe tomar esa decisión desde la perspectiva de los propietarios de la aplicación, no desde la perspectiva del desarrollador. Por lo tanto, debe pensar si vale la pena refactorizar el esfuerzo (dinero) necesario para solucionar el problema. He visto muchas veces que un desarrollador pasa varios días arreglando algo que no está realmente roto solo porque piensa que "el código es feo".

Mi tercer consejo es: te quemarás si rompes el código, no importa si es tu culpa o no.

Si ha sido contratado para dar mantenimiento, realmente no importa si la aplicación se está desmoronando porque alguien más tomó malas decisiones. Desde la perspectiva del usuario, estaba funcionando antes y ahora lo rompió. ¡Lo rompiste!

Joel pone muy bien en su artículo explicando varias razones por las que no debe reescribir el código heredado.

http://www.joelonsoftware.com/articles/fog0000000069.html

Así que deberías sentirte realmente mal por ese tipo de código (y nunca deberías escribir algo así), pero mantenerlo es un monstruo completamente diferente.

Acerca de mi experiencia: tuve que mantener el código durante aproximadamente 1 año y, finalmente, pude reescribirlo desde cero, pero no todo de una vez.

Lo que sucedió es que el código era tan malo que las nuevas características eran imposibles de implementar. La aplicación existente tenía serios problemas de rendimiento y usabilidad. Finalmente, me pidieron que realizara un cambio que me llevaría de 3 a 4 meses (principalmente porque trabajar con ese código me llevó mucho más tiempo de lo habitual). Pensé que podría reescribir toda esa pieza (incluida la implementación de la nueva función deseada) en unos 5-6 meses. Llevé esta propuesta a las partes interesadas y acuerdan reescribirla (por suerte para mí).

Después de reescribir esta pieza, entendieron que podía entregar cosas mucho mejores de lo que ya tenían. Así que pude reescribir toda la aplicación.

Mi enfoque fue reescribirlo pieza por pieza. Primero reemplacé toda la interfaz de usuario (formularios Windows Forms), luego comencé a reemplazar la capa de comunicación (llamadas al servicio web) y, por último, reemplacé toda la implementación del servidor (era una aplicación de tipo cliente / servidor).

Un par de años después y esta aplicación se ha convertido en una hermosa herramienta clave utilizada por toda la empresa. Estoy 100% seguro de que nunca hubiera sido posible si no hubiera reescrito todo el asunto.

Aunque pude hacerlo, la parte importante es que las partes interesadas lo aprobaron y pude convencerlos de que valía la pena el dinero. Entonces, si bien debe mantener la aplicación existente, haga todo lo posible para no romper nada y si puede convencer a los propietarios sobre el beneficio de reescribirla, intente hacerlo como Jack the Ripper: por partes.

Alex
fuente
1
+1. Pero la última oración se refiere a un asesino en serie. ¿Es eso realmente necesario?
MarkJ
Me gusta parte de esto, pero al mismo tiempo dejar diseños rotos en su lugar y agregar cosas a menudo conduce a un diseño peor. Es mejor averiguar cómo solucionarlo y solucionarlo, aunque en un enfoque medido, por supuesto.
Ricky Clarkson
@RickyClarkson Estoy de acuerdo. Es realmente difícil saber cuándo se está excediendo (una refactorización no es realmente necesaria) y cuándo realmente está dañando la aplicación al agregar cambios.
Alex
@RickyClarkson Estoy de acuerdo con eso tanto que incluso pude reescribir el proyecto en el que estaba involucrado. Pero durante mucho tiempo tuve que agregar cuidadosamente cosas tratando de causar el mínimo daño posible. A menos que la solicitud sea arreglar algo que la causa principal sea una mala decisión de diseño (que generalmente no es el caso), diría que no se debe tocar el diseño de la aplicación.
Alex
@RickyClarkson Eso fue parte de la experiencia de la comunidad que quería aprovechar. Declarar que todo el código heredado es malo es un antipatrón igualmente malo. Sin embargo, hay cosas en este código en las que estoy trabajando que realmente no deberían hacerse como parte de un bloque finalmente. Los comentarios y la respuesta de Alex son lo que estaba buscando leer.
2

Si no hay necesidad de cambiar el código, déjelo como está. Pero si tiene que cambiar el código porque tiene que corregir algún error o cambiar alguna funcionalidad, tendrá que cambiarlo, le guste o no. En mi humilde opinión, a menudo es más seguro y menos propenso a errores, si primero lo refactoriza en piezas más pequeñas, mejor comprensibles y luego agrega la funcionalidad. Cuando tenga a mano herramientas de refactorización automáticas, esto se puede hacer de manera relativamente segura, con un riesgo muy pequeño de introducir nuevos errores. Y le recomiendo que obtenga una copia del libro de Michael Feathers

http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

lo que le dará sugerencias valiosas sobre cómo hacer que el código sea más comprobable, agregar pruebas unitarias y evitar romper el código al agregar nuevas funciones.

Si lo hace bien, es de esperar que su método sea lo suficientemente simple como el try-catch-finally ya no contendrá ningún código no relacionado en los lugares incorrectos.

Doc Brown
fuente
2

Suponga que tiene seis métodos para llamar, cuatro de los cuales solo deberían llamarse si el primero se completa sin errores. Considere dos alternativas:

try {
     method1();  // throws
     method2();
     method3();
     method4();
     method5();
 } catch(e) {
     // handle error
 }
 method6();

vs

 try {
      method1();  // throws
 }
 catch(e) {
      // handle error
 }
 if(! error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

En el segundo código, está tratando excepciones como códigos de retorno. Conceptualmente, esto es idéntico a:

rc = method1();
if( rc != error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Si vamos a ajustar todos los métodos que pueden arrojar una excepción en un bloque try / catch, ¿cuál es el punto de tener excepciones en una característica del lenguaje? No hay beneficio y hay más mecanografía.

La razón por la que tenemos excepciones en los idiomas en primer lugar es que en los viejos tiempos de los códigos de retorno, a menudo tuvimos situaciones anteriores en las que teníamos múltiples métodos que podían devolver errores, y cada error significaba abortar todos los métodos por completo. Al comienzo de mi carrera, en los viejos días de C, vi un código como este por todas partes:

rc = method1();
if( rc != error ) {
     rc = method2();
     if( rc != error ) {
         rc = method3();
         if( rc != error ) {
             rc = method4();
             if(rc != error ) {
                 method5();
             }
         }
     }
 }
 method6();

Este fue un patrón increíblemente común, y uno que las excepciones se resolvieron de manera muy clara al permitirle volver al primer ejemplo anterior. Una regla de estilo que dice que cada método tiene su propio bloque de excepción lo arroja completamente por la ventana. ¿Por qué molestarse, entonces?

Siempre debe tratar de hacer que el bloque try sobrepase el conjunto de código que conceptualmente es una unidad. Debe rodear el código para el cual si hay algún error en la unidad de código, entonces no tiene sentido ejecutar ningún otro código en la unidad de código.

Volviendo al propósito original de las excepciones: recuerde que fueron creadas porque a menudo el código no puede manejar fácilmente los errores justo cuando realmente ocurren. El punto de excepciones es permitirle lidiar con errores significativamente más adelante en el código, o más arriba en la pila. La gente olvida eso y, en muchos casos, los captura localmente cuando estarían mejor simplemente documentando que el método en cuestión puede lanzar esta excepción. Hay demasiado código en el mundo que se ve así:

void method0() : throws MyNewException 
{
    try {
        method1();  // throws MyOtherException
    }
    catch(e) {
        if(e == MyOtherException)
            throw MyNewException();
    }
    method2();
}

Aquí, solo está agregando capas, y las capas agregan confusión. Solo haz esto:

void method0() : throws MyOtherException
{
    method1();
    method2();
}

Además de eso, creo que si sigue esa regla y se encuentra con más de un par de bloques try / catch en el método, debe preguntarse si el método en sí es demasiado complejo y debe dividirse en varios métodos. .

La conclusión: un bloque de prueba debe contener el conjunto de código que debe abortarse si se produce un error en cualquier parte del bloque. Si este conjunto de código es una línea o mil líneas, no importa. (Aunque si tienes mil líneas en un método, tienes otros problemas).

Gort the Robot
fuente
Steven: gracias por una respuesta bien presentada, y estoy totalmente de acuerdo con tus pensamientos. No tengo ningún problema con el código de tipo razonablemente relacionado o secuencial que vive en un solo bloque de prueba. Si hubiera podido publicar un fragmento de código real, habría mostrado entre 5 y 10 llamadas completamente no relacionadas antes de la primera llamada que se puede lanzar. Finalmente, un bloque en particular fue mucho peor: se estaban separando varios subprocesos nuevos y se llamaron rutinas adicionales que no son de limpieza. Y para el caso, todas las llamadas en ese bloque finalmente no estaban relacionadas con nada que pudiera haberse lanzado.
Un problema importante con este enfoque es que no hace distinción entre los casos en los method0que casi se puede esperar que se produzca una excepción, y aquellos en los que no. Por ejemplo, supongamos method1que a veces arroja un InvalidArgumentException, pero si no lo hace, pondrá un objeto en un estado temporalmente no válido que será corregido por method2, lo que se espera que siempre vuelva a poner el objeto en un estado válido. Si el método 2 arroja un InvalidArgumentExceptioncódigo que espera capturar tal excepción, method1lo capturará, aunque ...
supercat
... el estado del sistema coincidirá con las expectativas del código. Si una excepción lanzada desde el método1 debe manejarse de manera diferente a la lanzada por el método2, ¿cómo puede lograrse eso de manera sensata sin envolverlos?
supercat
Idealmente, sus excepciones son lo suficientemente granulares como para que esta sea una situación rara.
Gort the Robot