Clase 10 - Refactoring

publicado a la‎(s)‎ 25 jun. 2012 9:34 por Nicolas Passerini   [ actualizado el 28 ago. 2012 16:15 por Fernando Dodino ]

Introducción y definiciones

  • Arrancamos definiendolas ideas de refactor y code smell.
  • Mencionamos algunos code smells y los contamos brevemente:
    • Código duplicado
    • Métodos o clases demasiado grandes
    • Muchos parámetros
    • Data class
    • Feature envy
    • Message chain
    • Refused bequest
    • Temporary field.
  • Refactor = "behaviour preserving program transformation"
    • Se separan los momentos de agregar funcionalidad de los momentos de mejorar la calidad del código.
    • Al hacer modificaciones que no cambian la funcionalidad, los tests me asisten a asegurar que no cometo errores.
    • Hablamos sobre los problemas de "anidar refactors".
    • Estrategia: partir un refactor grande en muchos más chiquitos, para tener siempre todo andando.
      • Muchos refactors chiquitos son más fáciles que uno grande.
      • Puedo correr los tests después de cada cambio.
      • El límite es lo que me entra en la cabeza.
    • Non-localized plan.

Estrategia de refactoring

En primer lugar aclaramos que un buen refactoring debe hacerse con un repositorio de código (por ejemplo: svn) y tetsts automáticos. Esto nos permite verificar que nuestros cambios funcionan correctamente y publicarlos frecuentemente. En lo que sigue, cada vez que llegamos a tener algo funcionando se commitea (previa verificación mediante tests, obviamente).

Mirando el código del ejemplo, el code smell que encontramos es  que se setea un string tipo movimiento, y el if que se hace por él. Lo que quisieramos es reemplazar eso por polimorfismo. 

Comenzamos a buscar los usos de ese String y pensamos en cómo eliminar ese if... no está fácil así que vamos a ir paso a paso, entonces proponemos:
  • Ir acorralando el cambio
  • Pensar dónde podría estar ese polimorfismo: si son tipos de movimiento podríamos subclasear Movimiento.

Aquí podemos mencionar que la clase tiene dos objetivos:
  • Si pensamos en los refactorings de bajo nivel, ahí podemos entender el concepto básico y las herramientas que nos ayudan: el repositorio de versiones, los refactorings automáticos del IDE, los tests.
  • Si pensamos en un refactoring más grande, los refactorings automáticos del IDE ya no nos sirven, entonces tenemos que ver qué estrategia podemos utilizar para que nuestro código siga siendo maleable.
En cuanto a la metodología para hacer refactors:
  • Pequeños pasos
  • Iterativo
Herramientas:
  • SVN
  • Testing
  • Refactors automáticos
  • Backlog

El refactoring paso a paso

Paso 1: 
Buscamos las llamadas a modificarSaldo y encontramos que en ambos lados se hace la caca del if. 
El code smell  tambien es la existencia de los Movimientos.EXTRACION

Paso 2:  Clase cuenta. Hicimos un inline method en poner y sacar para eliminar el metodo modificarSaldo y para evitar el if por tipo-movimiento.

Paso 3: Ya sé que quiero delegar el comportamiento en otros objetos, como son comportamientos distintos, sé que voy a tener dos clases:  Extracción y Depósito, es decir, reifico el comportamiento de extraer, y el de depositar.

public class Extraccion extends Movimiento {
public Extraccion(Date fecha, BigDecimal monto) {
super(fecha, monto, Movimiento.EXTRACCION);
}
}

Y cambio la instanciación de la clase Movimiento por la clase correspondiente que va a ser responsable de ejecutar cada acción, en un caso Extracción, y en otro caso Deposito.
Luego de esto, el tipo de movimiento no se usa más desde la clase cuenta. Y podemos hacer abstracta a la clase Movimiento.
Y por otro lado, ya no necesito pasarle el tipo de movimiento en el constructor, gracias a la reificación, ahora ya no tiene que validar si es uno u otro, ahora, ese objeto YA ES uno y otro. Pero para hacer un cambio chiquito, lo que hago es que cada subclase, pase su tipo por parámetro, pero al menos, lo separé de la cuenta.

Paso 4: Buscamos otros usos. Encontramos:

public boolean fueDepositado(Date fecha) {
DateFormat dateFormat = DateFormat.getDateInstance();
return this.operacion.equals(DEPOSITO) && dateFormat.format(this.fecha).equals(dateFormat.format(fecha));
}

public boolean fueExtraido(Date fecha) {
DateFormat dateFormat = DateFormat.getDateInstance();
return this.operacion.equals(EXTRACCION) && dateFormat.format(this.fecha).equals(dateFormat.format(fecha));
}

Que se usan para getExtracciones y getDepositos (de una fecha)

Detengámonos un segundo en el método getDepositos, cuál es la diferencia entre la implemetación que hay ahora y la que estaba antes? es decir, la que se correspondia con este método? La diferencia es que antes, lo que representaba movimiento cambió, ahora un movimiento es o una extracción o un depósito, cambió la semántica del movimiento. Pero como acá los movimientos son polimórficos, cada movimiento va a tener que saber responder a esa pregunta. Así de movida, sabemos que el métodos, DEBE estar en la clase movimiento.

En el proceso de hacer esto, veo que el manejo de fechas es bastante discutible. Pero ojo, no me distraigo de lo que estoy haciendo!
Puedo agregar un TODO o levantar un issue, pero no debo iniciar otro refactor antes de terminar el primero.

a. Saco las constantes de los constructores, y dejo que cada subclase maneje la operación.

Veo el método fueDepositado(), y veo que hace bastantes cosas. Entre ellas, pregunta de que clase es, vamos a separar ese comportamiento, lo vamos a sacar de ahí para, primero separarlo, y después volarlo. 

b. Hago un extract method, y ahora lo hago abstracto, y dejo que las subclases me lo pidan.

Entonces me quedan:
protected abstract boolean isDeposito();
protected abstract boolean isExtraccion();

Ya me quedó todo dentro de la clase (por eso protected)
Y cada subclase tiene el comportamiento propio.

c. Voy a Deposito y Extraccion y le agrego las implementaciones y le quito las referencias a las constantes.

Si bien este paso podría haberse ahorrado, me permite elimnar las constantes.

Paso 5: Arreglemos las fechas que dejamos por ahí (ahora que tengo todo andando).

public boolean fueDepositado(Date fecha) {
return this.isDeposito() && this.esDeLaFecha(fecha);
}
public boolean fueExtraido(Date fecha) {
return this.isExtraccion() && this.esDeLaFecha(fecha);
}

/**
 * Uso un date format para elminar los segundos... no es muy feliz pero no se me ocurre nada mejor.
 */
protected boolean esDeLaFecha(Date fecha) {
DateFormat dateFormat = DateFormat.getDateInstance();
return dateFormat.format(this.fecha).equals(dateFormat.format(fecha));
}

La implementación es discutible, por eso le pusimos un comentario. 
Este es un ejemplo de más de realismo, el código / diseño perfecto no existen, tenemos restricciones de diferentes tipos y debemos encontrar una solución razonablemente buena.

Otras ideas que quedaron afuera

Acá se nos acabó el tiempo, pero podríamos continuar el refactor...

Paso 6: 
Delegar las validaciones en Deposito y Extracción.
En Extracción quedaría:

public void agregateA(Cuenta cuenta) {
this.validarMonto();
cuenta.validarExtraccion(this.getMonto());

if (cuenta.getSaldo().subtract(this.getMonto()).doubleValue() < 0) {
throw new UserException("No puede sacar más de " + cuenta.getSaldo() + " $");
}

cuenta.setProperty(Cuenta.SALDO, cuenta.getSaldo().subtract(this.getMonto()));
cuenta.agregarMovimiento(this);
}

Y en cuenta:
public void poner(BigDecimal cuanto) {
new Deposito(new Date(), cuanto).agregateA(this);
}

public void sacar(BigDecimal cuanto) {
new Extraccion(new Date(), cuanto).agregateA(this);
}

Paso 7: 
Mover las validaciones a la jerarquía de movimientos según corresponda.
Validar monto, va a movimiento, inclusive se puede hacer en el constructor.

public Movimiento(Date fecha, BigDecimal monto) {
this.fecha = fecha;
this.monto = monto;
this.validarMonto();
}

protected void validarMonto() {
if (this.getMonto().doubleValue() <= 0) {
throw new UserException(this.getMonto() + ": el monto a ingresar debe ser un valor positivo");
}
}

Nos quedan cosas que no sabemos bien qué hacer, nuevamente hay que lidiar con la imperfección.

Paso 8:
Hacer que los dos "agregar" sean iguales y luego moverlos a la superclase.
Las validaciones las hace la cuenta pero el movimiento sabe qué validar.

Refactors sobre los tests.

Otra cosa que nos quedó afuera fue revisar los tests. En realidad estos refactors son bastante más sencillos que los que hicimos, simplemente la idea de esto es agregar un ejemplo más; pero también insistir en que los tests son parte de nuestro código, que también deben ser mantenidos y por lo tanto conviene utilizar en su construcción los mismos criterios que utilizamos para el código productivo.

Ideas para mejorar el testDesposito:
  1. Separar las tres tareas que hay en el test, se puede hacer simplemente con un enter que divide el método en tres "bloques".
  2. Llevar la creacion del monto más abajo, acomodándolo en el segundo bloque, eso hace a los bloques más cohesivos.
  3. Una vez hecho eso podemos ver que ese test puede separarse en dos, entonces partimos en dos test de saldoInicialDebeCero.
  4. Renombramos los metodos de los test para que identifiquen el motivo del mismo, un nombre posible es "saldoDebeIncrementarseDespuesDeDepositar"
  5. Vemos que hay código repetido entre los tres asserts:
    • extraemos un método "assertSaldo"
    • agregamos el parámetro "saldo", para separar lo que no es repetido.
    • llevamos el método a una sección aparte (abajo de todo)
  6. También podemos marcar la sección con un separador
  7. Modificamos el resto de los asserts... (en realidad uno o 2)
  8. ¿Nos fuimos acordando de correr los tests en el medio de cada paso?
  9. Cambiar el nombre poner y sacar de la cuenta, para mantener la consistencia
Después de hacer todo esto, sería bueno comparar el método que modificamos con otro de los que están como arrancamos y ver las diferencias, claridad, expresividad.

Material de lectura

Comments