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)