Análisis estático de código y malos diseños

Bad DesignAprender a desarrollar y a diseñar software es una tarea en la que hay que invertir muchos años. Hay que ser obsesivo, fanático, hay que equivocarse miles de veces y aprender de esos errores. No contentarse con los aparentes aciertos porque al poco de andar se descubre que no se hizo lo mejor sino que alguien más en un blogs perdido en la web lo resolvió mucho mejor que nosotros y ese alguien también encontrará mejores maneras 10 minutos después de su posteo. Pero sobre todo, hay que leer muchísimo.

En las empresas de desarrollo como mucho existe un 10%-20% de gente con este perfil (atención que hablo de perfil y no de skills) por lo que el restante 90%-80% hará código (y Dios no lo permita, diseños) que van desde extremadamente malo a bastante bueno, mientras que los más experimientados, haran código que va desde lo aceptable a muy bueno con algunos chispazos de excelencia.

El análisis estático de código intenta achicar “un poco” esa brecha haciendo posible la implementacion de estándares y mejores prácticas en la codificación. La idea es extraer conocimiento de los profesionales más experimentados y plasmarlo en un conjunto de reglas que luego guien a la tropa de desarrolladores. En cierto modo esto se logra, porque a cualquiera por más experimentado que sea, una herramienta de análisis estático de código le dirá que su código tiene aspectos a observa, esto es lo mismo que ocurre cuando escribimos 500 u 800 lineas y presionamos F5, el compilador casi siempre nos dice “Eyy! y el punto y coma?” o “Y esta variable no la pensas declarar?” y cosas por el estilo. Sí, el primer análisis estático es el que hace el compilador sobre el arbol de sintaxis. Pero al igual que en el caso del compilador, el nivel análisis que se pueden plasmar en estas herramientas, al menos por ahora, es muy reducido y voy a explicar por qué.

Hace un par de dias, un compañero me preguntó como podia resolver un problema que el “sentía” que no lo estaba resolviendo bien. En una aplicación ASP.NET, tenia un formulario y un conjunto de botones que disparaban distintas acciones sobre los controles del formulario, ejemplo: Clear (limpiaba el formulario), Validate (validaba las entradas), Submit (posteaba el formulario previo tratamiento). El código del método Clear() era similar a este:

   1: private void Clear()
   2: {
   3:     foreach (WebControl wctrl in this.Controls)
   4:     {
   5:         switch (wctrl.GetType().FullName)
   6:         {
   7:             case "System.Web.UI.WebControls.TextBox":
   8:                 ((TextBox)wctrl).Text = String.Empty;
   9:                 break;
  10:             case "System.Web.UI.WebControls.CheckBox":
  11:                 ((CheckBox)wctrl).Checked = false;
  12:                 break;
  13:             case "System.Web.UI.WebControls.DropDownList":
  14:                 ((DropDownList)wctrl).SelectedIndex = 0;
  15:                 break;
  16:         }
  17:     }
  18: }

El problema salta a la vista muy rapidamente, una instrucción switch en cuya expresión interviene una instancia de Type!  Esta no es una solución orientada a objetos!. Aquí debe utilizarse un Adapter dije, pero el FxCop no advertía sobre esto. La opción es homogeneizar las operaciones mediante adaptadores:

   1: namespace Patterns.Test
   2: {
   3:     public partial class _Default : System.Web.UI.Page
   4:     {
   5:         private void Clear()
   6:         {
   7:             foreach (WebControl wctrl in this.Controls)
   8:             {
   9:                 ControlAdapterFactory.Instance.GetAdapter(wctrl).Clear();
  10:             }
  11:         }
  12:     }
  13:  
  14:  
  15:     interface IControlAdapter
  16:     {
  17:         void Clear();
  18:         void Submit();
  19:         void Validate();
  20:     }
  21:  
  22:     internal class TextBoxAdapter : IControlAdapter
  23:     {
  24:         private TextBox textbox;
  25:  
  26:         public TextBoxAdapter(TextBox t)
  27:         {
  28:             this.textbox = t;
  29:         }
  30:  
  31:         void Clear()
  32:         {
  33:             textbox.Text = String.Empty;
  34:         }
  35:  
  36:         void Submit() { }
  37:         void Validate() { }
  38:     }
  39:  
  40:     internal class ControlAdapterFactory
  41:     {
  42:         public static readonly ControlAdapterFactory Instance;
  43:  
  44:         private ControlAdapterFactory() { }
  45:         static ControlAdapterFactory()
  46:         {
  47:             Instance = new ControlAdapterFactory();
  48:         }
  49:  
  50:         public IControlAdapter GetAdapter(WebControl wctrl)
  51:         {
  52:             IControlAdapter adapter = null;
  53:             switch (wctrl.GetType().FullName)
  54:             {
  55:                 case "System.Web.UI.WebControls.TextBox":
  56:                     adapter = new TextBoxAdapter(wctrl);
  57:                     break;
  58:                 case "System.Web.UI.WebControls.CheckBox":
  59:                     adapter = new CheckBoxAdapter(wctrl);
  60:                     break;
  61:                 case "System.Web.UI.WebControls.DropDownList":
  62:                     adapter = new DropDownListAdapter(wctrl);
  63:                     break;
  64:                 default:
  65:                     throw new ArgumentException(
  66:                         String.Format("The control {0} has not an Adapter implemented", wctrl.Name));
  67:                     break;
  68:             }
  69:         }
  70:     }
  71: }

Así que pensé: esta regla es muy sencilla de implementar (en este caso no lo fué porque en el switch, cuando intervienen cadenas, no se utiliza el SwitchInstruction sino un infierno de IFs anidados) y me puse a escribir la regla.

Luego se me ocurrió que sería posible identificar patrones de mal diseño (antipatrones de diseño) que pudiesen mapearse con soluciones orientadas o objetos pero pronto llegué  a la conclusión de que esto no es posible debido a que el analizador, en este caso una regla de FxCop, debía “entender” primero lo que el código debia hacer para luego sugerir la solución.

Por otro lado, detectar un mal diseño puede ser factible mediante la detección de patrones de mal diseño o bien mediante algunos otros intentos como el de la “complejidad ciclomática” del código la que, amén de los falsos positivos, permite identificar puntos de mejora pero no puede recomendar nada concreto, más allá de un “divida este método en algunos otros”. Sin duda este último análisis puede dar una pista que el desarrollador más experimentado puede “considerar” para un análisis pero que pone al desarrollador junior ante un callejón sin salida: “esto está mal”. Pero más allá de esto, el problema mayor es que todas estas alternativas llegan tarde, cuando el esfuerzo para el desarrollo de esas piezas de software ya fueron consumidas. Esto puede verse en herramientas que hacen análisis de algunos aspectos de la arquitectura como Klocwork.

Por esto, aún cuando una herramienta pudiese identificar malos mini-diseños y proponer soluciones, no basta con solo una descripción de la solución al estilo “Reemplace este switch con un Adapter y una Factoría” sino que debería tener capacidades de refactorig que asistiese en la tarea.

Ni el análisis estático de código, ni el análisis dinámico de código nos salvan del mal código. ¿Será la programación con pares o las revisiones de código la solución?

 

Lucas Ontivero

Sin categoría

10 thoughts on “Análisis estático de código y malos diseños

  1. Interesante reflexión… leyendo tu post se me ocurre que hay todo un futuro por delante para ir construyendo herramientas de análisis estático, tanto de diseño como de arquitectura, cada vez más potentes.

    Sin duda la herramientas son útiles, pero creo que como tu apuntas, más útil es la programación en pares y las revisiones formales. El problema es que son dificiles de implementar por diferentes motivos. Lo que si funciona muy bien y ayuda mucho según mi experiencia es mantener la propiedad de la arquitectura, del diseño y del código compartida. Que todo el mundo pueda revisar y refactorizar cualquiera de estos aspectos, lógicamente comunicandolo al resto del equipo. Creo que da unos resultados optimos de coste de implantación frente a beneficiós obtenidos.

    Un saludo!

  2. Tal cual! Que el código sea público y no solo de un desarrollador es muy bueno, incluso es la manera más natural en la que en código puede ir evolucionando.
    En cuanto a la programación en pares y a las revisiones formales, algún día comentaré un análisis muy interesante que se hizo en la empresa en la que trabajo sobre estas dos y el porqué se terminó optando por las revisiones.

    Saludos.

  3. Pues… yo siento tener que discrepar, pero preferiré mil veces una función de 18 líneas donde lo que hay que hacer se ve muy clarito, antes que liarme a meter interfaces, factorías “singletonianas” y demás encajes de bolillos… ¡que al final terminan incluyendo, además, exactamente la misma función que en el primer caso!

    Por no hablar de que seguramente el código MSIL resultante sea bastante más eficiente en el primer caso que en el segundo.

  4. PabloNetrix, entiendo que tu desacuerdo es solo con el ejemplo de código ¿no?. Bueno, el caso es que ese método ni era tan pequeño ni claro. No era solo un switch sino que eran 5 o 6 métodos en los cuales habia un ‘for’ y adentro un switch bastante más ummm… largo.

    Otra cosa es la mantenibilidad. Si mañana cambiamos una grilla, digamos un GridView, por otra como esas grillas de terceros (Infragistics, DevX o ComponentOne) tendremos que ir por todos lados (los switchs) agregando comportamiento para realizar las acciones sobre el nuevo control. En cambio, de esta manera solo escribes el nuevo Adapter y listo (y lo agregas a la fábrica, claro).

    Te imaginaras que este código se repite en cada formulario. Cada webform que tiene que limpiarse tiene su método ‘Clear()’ con su ‘for’ y su ‘switch’. Para evitar esto, o se hace mediante adapters (OOP) o bien se crea una clase ‘FormUtils’ con un método ‘Clear()’ que recibe la colección de controles y los recorre y los limpia. Esta última es más al estilo procedural.

    La OOP siempre tiene más código pero se espera que a la larga otorgue ventajas como las que menciono arriba.

    Pero lo más importante es tu criterio. Cuando aplicar esto y cuando no. Yo creo que el ejemplo (el segmento de código) no estuvo muy bien y por eso todas estas linea en el comentario. En este caso particular, en el del ejemplo, parece un despropósito tanto código para hacer lo mismo que se puede hacer en 18 lineas, eso está claro y entiendo que tenes razón.

    El singleton lo puse porque no tiene sentido crear más instancias de
    esta factoria. Pero quizás a modo de ejemplo, no hacia falta. Y La fábrica tiene un diseño no muy bueno, quizás mediante reflection puedan crearse instancias el tipo (wctrl.Name + “Adaptor”)  pero esto siempre trae problemas, amén de la performance.

    Saludos y gracias por tus aportes en mi blogs

  5. Bueno en este caso concreto sí, mi desacuerdo es con el ejemplo de código (quizás me precipité, lo siento, jeje :P)

    “Te imaginaras que este código se repite en cada formulario. Cada webform que tiene que limpiarse tiene su método ‘Clear()’ con su ‘for’ y su ‘switch’. Para evitar esto, o se hace mediante adapters (OOP) o bien se crea una clase ‘FormUtils’ con un método ‘Clear()’ … ”

    ¿Y no se puede extender la clase Page, como hacías en el segundo ejemplo, y añadirle ese primer método llamándolo, yo que sé, ‘ClearWebControls’?

    No me des las gracias por mis “aportes”, que yo aporto más bien poquito… Si alguien tiene que dar las gracias soy yo, que cada dia aprendo una cosa nueva en tu blog. 🙂

  6. Cada vez me convenzo más de que la única salida es la programación de a pares, de acuerdo con el “estado del arte” a la fecha. Porque hay que tener en cuenta que las buenas prácticas no sólo se establecen de acuerdo a estándares “externos” sino también (y sobre todo, diría) a lo que el equipo está acostumbrado a ver en el desarrollo. Creo que (sin extremismos, ojo) a veces es mejor código “común” -estructuras que se repiten mucho dentro del proyecto o dentro de un equipo- que “óptimo”. Esto sólo se logra si cada uno ve, participa o revisa en la creación del código del otro.
    De todas maneras estas herramientas ayudan a detectar problemas para evitar nuevos casos a futuro. La corrección es algo que siempre se da más lentamente, de a poco. Y nos dejan más tiempo para controlar aquello que sólo un humano puede controlar.

  7. Ummm, extender la clase Page aquí no es tiene sentido por cuanto no conoces que tipo de controles tendrá.
    Hoy por hoy, todos estos patrones van perdiendo de a poco su relevancia inicial porque surgen nuevas maneras de hacer aquellas cosas para las que fueron pensados. Hoy quizás algo mas amigable sería inyectarles a los controles los métodos ‘Clear()’, ‘Validate()’, ‘etc’ mediante los extension methods y santísimo remedio pero bueno, el ejemplo era solo eso, un ejemplo.

  8. Andrés, comparto. En cuanto al código si se repite o no, creo que, como dije en otro comentario, lo más importante es el criterio de los desarrolladores. No obstante cuando el código se empieza a repetir producto muchas veces del copiar y pegar, mantener el producto se vuelve un poco mas complicado y hay que andar buscando “en donde más tengo que tocar”.

    Pero sí, muchas veces puede ser preferible por diversas razones.

  9. Algo de lo que comenta “Andrés Panitsch”, es acertado “De todas maneras estas herramientas ayudan a detectar problemas para evitar nuevos casos a futuro. La corrección es algo que siempre se da más lentamente, de a poco. Y nos dejan más tiempo para controlar aquello que sólo un humano puede controlar.” hay cosas que las herramientas no podran hacer y que solo un humano tendra que pensar como resolver el problema, volvemos al caso de los entornos de desarrollo de 3G (Genexus…), hay varias cosas que hay que dejarlo a un humano y no a una herramienta 🙂

  10. Quiero decir que la información que he estado viendo en el blog es muy interesante, y coincido plenamente en que llegar a programar profesionalmente lleva años.
    Considero por mi parte que a mi me falta mucho.

    Llegar a programar como un programador OO lleva mucho esfuerzo, lectura y practica.
    El programador ha pasado de un lenguaje estructurado a uno orientado a objetos sin tener claros o haber madurado los conceptos de la OO.
    Aparentemente estos conceptos son muy sencillos, pero ya se sabe el diablo esta en los pequeños detalles.

    La vida no ha sido justa con la Orientacion a Objetos.La pobre ha tenido – y tiene – mas metodologos de los que necesitaba;y muchos lenguajes y herramientas le han hecho un flaco favor.

Deja un comentario

Tu dirección de correo electrónico no será publicada. Los campos obligatorios están marcados con *