Code Smells: Cosas que creías que hacías bien... pero no.

Las cosas cotidianas se hacen de forma casi automática, sin pensar. Esto no sólo ocurre cuando necesitamos cambiar de marcha o abrir nuestro portátil, no. También nos ocurre programando… y a veces no es muy buena idea.

NSA Counting

Nos ha pasado a todos. Tenemos que llamar a un método que nos devuelve un IEnumerable, pero sólo queremos hacer algo si contiene algún elemento. ¿Os suena este código?

 var enumerable = ThisReturnsAnEnumerable();            
if (enumerable.Count() > 0)
{
    // Do something
}

Lo malo de este tipo de código, es que a nosotros solo nos interesa saber si existe al menos un elemento, lo que debería ser una operación O(1). Sin embargo, estamos preguntando la cantidad total de elementos, lo que implica una complejidad O(n) .

Algunas veces la estructura de datos tendrá una propiedad Count que hará que Count() tenga complejidad O(1), pero eso ya depende de la estructura en concreto y de cómo la hayan implementado…

Podemos reemplazar el código anterior por este con complejidad O(1) :

 var enumerable = ThisReturnsAnEnumerable();
if (enumerable.Any())
{
    // Do something
}

Cualquier elemento satisface la condición vacía, por lo que de existir algún elemento, la condición resuelve cierto al primer elemento, eliminando la necesidad de continuar la operación .Any

Pokemon Exception Handling

Este término en realidad se usa para varios casos, cada cual más hiriente que el anterior. El más común es este:

 try
{
    // Do something
}
catch
{
    // Catch 'em all!
}

En general, hacer un manejo silencioso de una excepción es una mala idea que en ocasiones puede ser muy mala idea. Ahora bien, hacer un manejo silencioso de TODAS las excepciones es una práctica prohibida bajo pena de catapulta. El fix es sencillo… no captures excepciones que no manejas.

Otra variante del Pokemon Excepcion Handling es el retrowing:

 private void SomeoneElsesMethod()
{
    MethodPokemonExceptionHandling();
}

private void MethodPokemonExceptionHandling()
{
    try
    {
        MethodThatFails();
    }
    catch (NullReferenceException) { throw new NullReferenceException(); }
}

private void MethodThatFails() { throw new NullReferenceException(); }

El método “MethodThatFails” quiere lanzar una excepción. El programador del método “MethodPokemonExceptionHandling”, sabe que “MethodThatFails” puede dar una excepción, así que la captura. El único problema es que no tiene ni idea de cómo solucionar la excepción, así que decide relanzarla. Cuando “SomeoneElsesMethod” llama al “MethodPokemonExceptionHandling” obtiene un NullReferenceException… todo parece correcto ¿o no?

El problema aquí es que el método pokemon, al relanzar la excepción está cercenando la pila de llamadas, haciendo que cuando depuremos veamos esto:

Ni rastro de “MethodThadFails” en el call stack. Teniendo en cuenta que tenemos el código y que son cuatro líneas de código, no nos costará encontrar el error. Ahora bien, imaginad que esto es una librería de la que no tenemos el código, pensaríamos que el método pokemon es el causante y se complicaría la depuración. Sin embargo, si eliminamos la captura pokemon:

 private void SomeoneElsesMethod()
{
    MethodPokemonExceptionHandling();
}

private void MethodPokemonExceptionHandling()
{
    MethodThatFails();
}

private void MethodThatFails() { throw new NullReferenceException(); }

Ahora podríamos ver en el call stack al verdadero causante del problema:

If I can’t see it, it ain’t that bad

C# soporta regiones de código. Esta característica fue introducida, principalmente, para poder introducir bloques de código generado automáticamente y que estos pudieran ocultarse evitando distraer al programador. C# 2.0 introdujo las clases parciales, que solucionan de forma mucho más elegante ese problema. Sin embargo, muchos programadores insisten en seguir usando regiones de código esgrimiendo dos argumentos:

El código queda más organizado

La organización del código debe dejarse a herramientas como StyleCop, que compilación tras compilación, verificarán que efectivamente todo esté en orden. Una región de código con nombre “Private methods” nunca va a ser verificada.

Puedo ocultar métodos largos

Un método lo suficientemente largo como para querer ser ocultado no debería existir en primer lugar. Si esto ocurre, hay que refactorizar. Al margen de eso, Visual Studio permite colapsar métodos ;)

Exception Flow

Este uso de las excepciones no tiene nada que ver con el Pokemon, pero también debemos tener mucho cuidado con él, ya que el impacto en el rendimiento es enorme y podemos causar un infarto de miocardio al siguiente programador que lea el código.

Supongamos que tenemos una lista de ficheros y queremos mostrar su contenido al usuario. Si el fichero no existiera, también queremos informar al usuario. Alguien podría escribir un código tal que así:

 foreach (var name in Enumerable.Range(0, 10).Select(x => "File" + x + ".txt"))
{
    try
    {
        Console.WriteLine(File.ReadAllText(name));
    }
    catch (FileNotFoundException e)
    {
        Console.WriteLine("File " + e.FileName + " doesn't exist!");
    }
}

Efectivamente el código funciona como se espera, el problema es que estamos usando las excepciones como flujo de control. Esto es, siempre, una mala idea. El manejo de excepciones es una te las tareas más costosas en términos computacionales debido a la cantidad de comprobaciones y cambios de contexto que deben realizarse.

Este código resolvería el problema:

 foreach (var name in Enumerable.Range(0, 10).Select(x => "File" + x + ".txt"))
{
    if (File.Exists(name))
    {
        Console.WriteLine(File.ReadAllText(name));
    }
    else
    {                    
        Console.WriteLine("File " + name + " doesn't exist!");
    }
}

As Is

Otro de los errores más comunes es no tener muy claro el uso de las palabras reservadas as e is. Cuando queremos realizar una casting explícito, podemos usar el operador “is” para saber si el cast podrá realizarse sin producir excepciones. Así pues, este código es correcto y llamará al método SayHello, sólo cuando obj no sea nulo y además pueda castear a MyType:

 private void InterestingMethod(Object obj)
{
    if (obj is MyType)
    {
        ((MyType)obj).SayHello();
    }
}

Ahora bien, este no es el uso apropiado, ya que el código MSIL generado realizará el cast dos veces, pues is generará el opcode isinst y el casting generará un castclass. Este proceso puede optimizarse con el uso de “as”.

El operador “as” se usa para relizar castings a tipos sin generar excepciones, ya que si la conversión no es posible, retornará null. En este caso podemos relizar la conversión de forma mucho más rápida con el siguiente código:

 private void InterestingMethod(Object obj)
{        
    var myType = obj as MyType;
    if (myType != null)
    {
        myType.SayHello();
    }
}

A veces menos código no significa más eficiente

 

¿Haces código con alguno de los code smells que menciono? ¿Cuáles añadirías a la lista? ¡Cuéntanos!

Happy hacking!

Pablo Carballude (@carballude_es)

Comments

• Anonymous
  July 14, 2014
  Excelente artículo. muchas gracias por tus aportes

• Anonymous
  July 14, 2014
  The comment has been removed

• Anonymous
  July 15, 2014
  Antes de nada, buen artículo Pablo, sobre todo con algunas cosas como el manejo de excepciones, ese a veces gran desconocido. Por otro lado, discrepo un poco en el tema de las regiones, no sólo te sirven para el ejemplo y a mi me gusta dejar ordenadas las cosas... tenemos nuestra hoja de estilo en la empresa. Y aunque StyleCop puede ser genial, no siempre será la mejor solución. Hay cosas que uno siempre debe de hacer... ser ordenado es una de ellas. En mi humilde opinión.

• Anonymous
  July 16, 2014
  En el ejemplo "correcto" de "exception flow": if (File.Exists(name)) {    Console.WriteLine(File.ReadAllText(name)); } else {                        Console.WriteLine("File " + name + " doesn't exist!"); } Hay un problema. Entre la ejecución del if en la CPU y la ejecución de la rama correspondiente (sea el if o el else) también en la CPU la condición puede haber dejado de ser cierta/falsa y el fichero existir o no (cambios de contexto, ficheros borrados/creados, etc). Vaya, veo que Javier Campos ya comentó este hecho. Sea como fuere publico el comentario igual

• Anonymous
  August 13, 2014
  Pues la verdad que con Lo del Any toda la razon y mas Cuando Hay cientos  o miles de items. Ahora mismo los cambio todos muchas gracias

• Anonymous
  August 13, 2014
  The comment has been removed

• Anonymous
  August 14, 2014
  Aquí aporto mi pequeño grano de arena. Se trata de concatenar cadenas. Estamos acostumbrados a hacer lo siguiente: string cad = "Hola"; cad += " Mundo!"; Esto es correcto y rápido cuando se tratan de cadenas cortas o pocas, pero cuando son cadenas largas y/o se concatenan muchas, baja drásticamente el rendimiento de la aplicación. Para solucionarlo mejor crear una variable de tipo StringBuilder y con el método Append(string) ir añadiendo las cadenas y finalmente mostrar todas con ToString(): StringBuilder cad2 = new StringBuilder(); cad2.Append("Hola"); cad2.Apoend(" Mundo!"); cad2.ToString(); Espero les sirva este tip. Saludos y excelente artículo (varias de esas no me las sabía).

• Anonymous
  August 15, 2014
  @Javier Campos @David, efectivamente podría haber una condición de Carrera al comprobar la existencia del fichero y luego leerlo. Cuando pensé el ejemplo quería que se viese claramente el control de flujo... pero efectivamente en este caso en concreto debería haber ese control. También es puntería... jeje @David Carrascosa, bien usadas, las regiones de código no tienen ningún problema. Lo que ocurre es que, en la mayoría de las ocasiones, no se usan bien, simplemente porque son dadas a que no se actualicen y a que el IDE suele tener cero control sobre ellas. De ahí que estén desaconsejadas, lo que no quiere decir que siempre que se usen estén mal, ni mucho menos ;)