MSDN España

Noticias, avisos, reflexiones... del equipo que hay detrás de MSDN en España... también tendremos alguna que otra firma invitada :)

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

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

Rate This
  • Comments 8

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:

image

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:

image

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 Smile

 

¿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)

Leave a Comment
  • Please add 2 and 8 and type the answer here:
  • Post
  • Excelente artículo. muchas gracias por tus aportes

  • Creo que en el exception flow, en el ejemplo que pones al menos... la cura es peor que la enfermedad. Vale que no se debe hacer flow con excepciones, pero quitar el manejo de excepciones sólo porque no hacemos flow con ello es mucho peor.

    ¿Y si entre el File.Exists() y File.ReadAllText() se ha borrado el archivo?

    Imagino que lo habrás puesto sólo como ejemplo (y lo obvio es quitar directamente el try/catch), pero es un mal ejemplo si lo que estamos poniendo son codesmells, en mi opinión.

    Por lo demás, buen artículo... a destacar que Resharper te avisa de muchos de estos smells (y otros).

  • 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.

  • 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

  • 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

  • Genial articulo Pablo, siempre se aprenden cosas nuevas.

    Pero no me ha quedado claro una cosa. Cuando comentas bajo pena de catapulta, te refieres a que te lanzan con ella, te obligan a transportarla hasta el campo de batalla o que te la tiran a la cabeza con piedra y todo???

    Fuera coñas, un 10 por el post.

  • 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).

  • @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 ;)

Page 1 of 1 (8 items)