Antiprogramación

platanada

Muchas veces se habla sobre cómo deberían hacerse las cosas, pero a veces no se muestran los contraejemplos. A raíz de una conversación en la lista de correo se me ha ocurrido que podría ser útil tener una lista de ejemplos de malas prácticas de programación, cosas a evitar, desde fallos de novato hasta cosas más complejas.

Empezaré con un par de ejemplos sencillos que me vienen a la cabeza, ejemplos reales que he visto, y espero que os animéis y publiquéis los que recordéis. Aquí los que más aportarán serán los profesores, que corrigen nuestras prácticas y seguro que más de una vez se llevan las manos a la cabeza (ahora los modernos lo llaman facepalm):

Condición múltiple

Este contraejemplo apareció en una práctica sobre un juego de ajedrez. Se trataba de comprobar las coordenadas de la pieza:

if (row==1 || row==2 || row==3 || row==4 || row==5 || row==6 || row==7 || row==8) {
    if (col=='A' || col=='B' || col=='C' || col=='D' || col=='E' || col=='F' || col=='G' || col=='H' ){
 
        // coordenadas correctas, hacer cosas
 
    } else {
        // columna incorrecta
    }
 
} else {
    // fila incorrecta
}

Una solución mejor habría sido algo como:

if (row<1 || row>8 || col<'A' || col>'H'){
    throw new InvalidCoordException(row, col);
}
 
// todo correcto, hacer cosas
}

Condición inversa

En la misma práctica también encontré esto:

if (condición) {
} else {
    // hacer cosas
}

No recuerdo exactamente cuál era la condición, pero la idea era esa. Obviamente, hubo que sustituirlo por:

if(!condición){
    //hacer cosas
}

Comentarios

Opciones de visualización de comentarios

Seleccione la forma que prefiera para mostrar los comentarios y haga clic en «Guardar las opciones» para activar los cambios.
Imagen de magmax

El mejor compendio de Anti-patrones de programación

Tras leer esto (sacado de aquí: http://msdn.microsoft.com/en-us/library/ms682629%28v=vs.85%29.aspx):

It is a good idea to use a large array, because it is hard to predict how many processes there will be at the time you call EnumProcesses.

To determine how many processes were enumerated, divide the pBytesReturned value by sizeof(DWORD). There is no indication given when the buffer is too small to store all process identifiers. Therefore, if pBytesReturned equals cb, consider retrying the call with a larger array.

...me he quedado así: Jawdropping!

Y ésta es la dirección del mejor compendio de antipatrones y mala programación que he visto: http://msdn.microsoft.com/en-us/library/dd162863%28v=VS.85%29.aspx

Odio con todas mis fuerzas tener que hacer algo en el innombrable...

Miguel Ángel García
http://magmax.org

Imagen de magmax

Mejora

Mejorando aún más tu primer ejemplo ("Condición Múltiple"):

{ 
    coords.validate();
    // hacer cosas
}
 
//[...]
 
class Coordinate {
   int row, col;
   public void validate() {
      if (row<1 || row>8 || col<'A' || col>'H')
          throw new InvalidCoordException(row, col);
   }
}

Con eso has conseguido dos cosas: ya no tienes "row" y "col", sino "Coordinates", que es más orientado a objetos. Y te has cargado un comentario que no aportaba nada, mejorando la legibilidad del código.

Si me apuras, hasta la propia función validate es innecesaria, ya que al tener un objeto con las coordenadas puedes elevar la excepción en el momento en el que las coordenadas no sean válidas.

Finalmente, ahí va una lista de antipatrones: http://en.wikipedia.org/wiki/Anti-pattern

Miguel Ángel García
http://magmax.org

Imagen de brue

imho...

... validate() no está de más, pero yo no dejaría al cliente utilizarlo. Sería un método privado. Si hay que comprobar en más de un sitio, validate() evitaría la redundancia.

My 2 cents,

si yo tuviera que resolver el problema, usaría una clase Tablero, que encapsularía una matriz de 8 x 8. Este objeto se encargaría de validar movimientos y demás. Supongo que en la matriz estaría compuesta de objetos que representasen si una casilla tiene una ficha o no, y cuál de ellas es. Sea como sea, las casillas no tendrían row/col, y si decidiera utilizar algún tipo de clase Casilla, usaría algo parecido al patrón "pesopluma", usando la información de row/col desde el tablero, no teniendo así la información de la posición por duplicado, y evitando posibles inconsistencias de datos. Nótese que el array/matriz donde colocásemos esas "coordenadas" ya tendría esa información.

brue

Imagen de nacho

Muy bien. Matrícula.

Hombre, estás rediseñando la solución entera, cuando en ese pequeño snippet de código, lo que te apuñala los ojos es la semántica, la forma de expresar algo. Obviamente, soluciones (diseños) hay muchas, sólo me limité a poner la que era semántica y sintácticamente más parecida (de hecho, equivalente).

Me viene a la cabeza un caso que nos ha comentado David en alguna ocasión, y que sí que es un problema de diseño: un alumno, para resolver un juego de 3 en raya, anidó tropecientos if, de forma que en cada uno resolvía una posición particular del tablero. Y ni siquiera consideraba las posiciones equivalentes (por ejemplo, rotado 90, 180 y 270 grados).

Nacho

Imagen de brue

Pero ...

... es que yo creo que un mal diseño te conduce a esos "hacks" de manera directa.

El problema de ese alumno es un problema de diseño, entre otros. De hecho, así lo apuntas tú mismo al mencionar "rotaciones".

Una hoja y un boli suelen ser unos buenos elementos para programar. Otra cosa es que sólo vayas a picar código, esto es, "programming versus coding". Yo creo que codificar es parte de programar, y no al revés. Para programar hay que diseñar, para codificar no.

brue

Imagen de david.villa

lo pondría aquí, pero son

lo pondría aquí, pero son como 3000 líneas y el comentario iba a quedar algo largo Sticking out tongue

No soy portavoz de ningún colectivo, grupo o facción. Mi opinión es personal e intransferible.

Imagen de nacho

Muy guapo lo de los

Muy guapo lo de los anti-patrones, no lo conocía.

Nacho

Imagen de brue

Si leyeras más CRySoL ...

... sí que los conocerías Eye-wink

http://crysol.org/es/node/1393

brue