03 FebLos comentarios en el código son basura

Ayer estuve viendo una presentación (en inglés) de Robert C. Martin (Uncle Bob) en la Øredev 2008 en la que trataba alguno de los contenidos de su nuevo libro “Clean Code” (Código Limpio) y uno de sus comentarios me hizo recordar a mi buen amigo (y tantas veces maestro) Xavi Gost cuando decía que los comentarios en el código son basura.

Efectivamente, según Uncle Bob, los comentarios son excusas para escribir un método y mentiras (porque casi siempre están desactualizados). Por ello nos pide que, para ayudar a mantener limpio nuestro código, los borremos sin ni tan siquiera leerlos. Obviamente no se refiere al javadoc, aunque bien podría hacerlo. Martin afirma que si en un método dado tenemos varios bloques de código separado por comentarios, eso está pidiendo a gritos ser separado en varias llamadas a métodos donde cada comentario será el nombre de cada método.

He de decir que, a pesar de lo radical que pueda parecer la propuesta de Mr. Martin, no puede tener más razón. De hecho, Xavi me contaba que él ha llegado a configurar su sistema de control de versiones para que eliminara todos los comentarios no javadoc antes de hacer efectivo un commit, con lo que, en la práctica, estaba empujando a los desarrolladores a no incluir comentarios en el código.

Muchos de los que han trabajado conmigo me habrán oido discutir durante horas sobre el nombre de una clase o de un método. Por ahí van los tiros. Tanto Martin en “Clean Code” como Beck en el imprescindible “Implementation Patterns” insisten en la vital importancia de que elijamos bien los nombres de clases y métodos porque son el primer paso para escribir un buen código. Haced la prueba, intentad escribir vuestro código sin comentarios y, cuando tengáis la intención de hacerlo, cambiarlo por una llamada a un método…

27 NovRecursos en castellano

Carlos Ble ha tenido a bien incluirme en una lista de enlaces de recursos sobre TDD, refactoring y otras cosas ágiles. Muchas gracias, Carlos, sobre todo por compartir esa inquietud por las carencias de material en español.

18 NovRefactoring en Español (y 6)

Hoy, por fin, termino esta serie de artículos basados en el ejemplo del primer capítulo del libro de Fowler “Refactoring“.

Recapitulemos un poco antes de comenzar.

Desde la primera entrega hasta ésta no hemos cambiado el comportamiento del sistema: sólo hemos cambiado el diseño interno. A eso le llamamos refactorizar. (Por cierto, mi siguiente entrega de “Aprender inglés es fácil… si sabes cómo” irá justamente sobre esta palabra).

Y estamos seguros de que no ha cambiado el comportamiento porque tenemos una batería de pruebas unitarias que así nos lo han ido demostrando.

A lo largo de las cinco entregas anteriores hemos ido moviendo código dentro de las clases hacia métodos privados que, además, mejoraban la comprensión del código. También hemos movido código entre clases, delegando responsabilidades entre objetos. Y finalmente, en la última entrega dejamos unas condiciones múltiples (switch) en los métodos getCharge y getFrequentRenterPoints en la clase Movie. Vamos a ver en este final del ejemplo cómo eliminar esos ifs, lo cuál me sirve de entradilla para comentar la campaña anti-if (originalmente en italiano, sí, en italiano).

Comenzamos.

At last… Inheritance

Para no plagiar, creo que lo mejor es recomendar directamente la lectura de este capítulo 1 del libro. Sin embargo, y aunque la explicación es excelente, hay que reconocer que el original está en inglés… :-)

Aunque no me dedico a la traducción y mi inglés es mejorable, voy a tratar de aportar mi granito de arena a la causa hispana.

AL FIN… HERENCIA

Tenemos varios tipos de películas que tienen diferentes maneras de responder a la misma pregunta. Esto parece un trabajo para subclases. [N.T. El autor se refiere a Movie.getCharge]. Podemos tener 3 subclases de Movie, cada una de las cuáles teniendo su propia versión de getCharge. (Ver Fig. 1.14)

[N.T. La fig. 1.14 muestra un diagrama de clases UML con las subclases RegularMovie, ChildrensMovie y NewReleaseMovie, heredando de Movie. Todas tienen un método llamado getCharge.]

Esto nos permite sustituir la instrucción switch por el uso de polimorfismo. Desgraciadamente esto tiene un pequeño problema: que no funciona. Una película puede cambiar su clasificación a lo largo de su vida. Un objeto no puede cambiar su clase a lo largo de su vida. Sin embargo, hay una solución en el patrón Estado [GoF]. Usando el patrón Estado, nuestro diseño quedaría como en la fig. 1.15

[N.T. La fig. 1.15 muestra otro diagrama de clases UML con la clase Price y sus subclases RegularPrice, ChildrensPrice y NewReleasePrice. Además, también muestra la clase Movie usando una instancia de Price para el cálculo de getCharge.]

Añadiendo la indirección podemos hacer las subclases del objeto Price y cambiar el precio cada vez que lo necesitemos.

Si estáis familiarizados con los patrones del GoF, quizás os preguntéis: “¿Es esto un estado o una estrategia?”. ¿Representa la clase Price a un algoritmo para calcular el precio (en cuyo caso yo preferiría llamarla Pricer o PrincingStrategy) o representa al estado de una película (Star Trek X es una novedad)? En este punto, la elección del patrón (y del nombre) refleja cómo queremos pensar acerca de la estructura. De momento estamos pensando en este objeto como el estado de una película. Si más adelante decidimos que una estragia comunica mejor nuestras intenciones, refactorizaremos cambiando los nombres.

Hasta aquí la traducción. Espero que os ayude a entender el cambio que vamos a introducir en el diseño y el por qué lo hacemos.

Vamos a introducir el patrón Estado. Para ello nos aseguramos de que tenemos bien encapsulados los accesos al atributo Movie._priceCode. (Lo cambiamos incluso en el constructor).

TESTS

Para introducir Price y toda la jerarquía hacemos TDD y, para ello, creamos PriceTest como sigue y lo añadiremos a la suite TestAll.

package step6;

import org.junit.Test;

public class PriceTest {

  @Test
  public void testChildrensPrice() {
    Price aPrice = new ChildrensPrice();
    assertEquals(Movie.CHILDRENS,aPrice.getPriceCode());
  }

}

Lógicamente, como no compila, tendremos que crear las clases y métodos correspondientes. Al ejecutar los tests, salen en rojo (si hemos implementado getPriceCode con un valor por defecto), pero en cuanto hacemos que getPriceCode devuelva el valor esperado según la subclase que vayamos incorporando, volvemos al verde. Siempre primero el test y luego el código.

A continuación introducimos el uso de Price en Movie tal y como propone Fowler. De hecho, al copipegar su implementación de Movie.setPriceCode incorpora código para el que no teníamos pruebas. Esto lo descubrimos al ver el informe de cobertura de nuestras pruebas y comprobar que no tenemos una prueba para comprobar qué pasa cuando asignamos a una película un tipo de precio desconocido. (Sí, en sentido estricto no estaríamos refactorizando puesto que hemos cambiado los tests, e.d. la funcionalidad original, pero yo más bien diría que hemos encontrado un defecto en el código original puesto que no contemplaba una condición excepcional).

@Test(expected=IllegalArgumentException.class)
public void testMovieIllegalType() {
  new Movie(ANY_TITLE,UNKNOWN_PRICE_CODE);
}

Por lo demás, tras ejecutar nuestra batería de tests, el refactor ha ido bien. (Lo sabemos porque nuestros tests están en verde)

Siguiendo los pasos de Fowler, refactorizamos Movie.getCharge para que use un nuevo Price.getCharge (OJO, sin olvidar que estamos haciendo TDD) :-)

Implementamos Price.getCharge moviendo el código de Movie.getCharge a la clase abstracta (aunque este método no será abstracto).

No eliminamos el viejo test puesto que es lo que nos indicará que funciona o no nuestro refactor; pero implementamos los tests como el siguiente. (En realidad es fácil porque son idénticos a los de Movie)

@Test
public void testChargeForChildrens() {
  Price aPrice = new ChildrensPrice();
  double charges[] = { 1.5, 1.5, 1.5, 3.0, 4.5, 6.0, 7.5 };
  for (int daysRented = 1; daysRented <= charges.length; daysRented++) {
    assertEquals(charges[daysRented-1], aPrice.getCharge(daysRented),0);
  }
}

TESTS

Declaramos el método ahora como abstracto e implementamos el primero que nos venga en gana (p.ej. ChildrensPrice.getCharge). Ya tenemos los tests, por lo que iremos refactorizando con seguridad. Recordad el lema de TDD: “RED GREEN REFACTOR“.

TESTS

A continuación, vamos haciendo los cambios correspondientes al resto de tipos de precio. Siempre con pasos seguros gracias a nuestra batería de tests.

TESTS

Y finalmente hacemos lo mismo con getFrequentRenterPoints:

  • movemos el código de Movie.getFrequentRenterPoints a Price.getFrequentRenterPoints (incluyendo primero los nuevos tests porque hacemos TDD)
  • no olvidamos mover los javadocs
  • implementamos el caso particular de NewRelease.getFrequentRenterPoints (dejando el caso general en la clase abstracta Price)

Pues bien, llegados a este punto ya hemos terminado con el ejemplo de Fowler. Os recomiendo encarecidamente el libro (en él se detallan las técnicas de refactor y los “tufillos” (smells) que nos indican cuándo es posible que nuestro código requiera ser refactorizado) y un par de secuelas muy interesantes:

  • Refactoring Workbook, con el que podréis ejercitar todo lo que hayáis aprendido con el de Fowler.
  • Refactoring to Patterns, con el que podréis orientar vuestros refactorajes hacia patrones de diseño, mejorando así la calidad de vuestros diseños.

Finalmente, una última recomendación: http://sourcemaking.com/refactoring

Desgraciadamente todas estas referencias están en inglés. :-(
¿Cuándo nos decidiremos a crear contenidos en español ahora que estamos reivindicando la profesión aquí en España?

Tags: ,

11 NovRefactoring en Español (5)

Por fin he conseguido sacar tiempo para continuar con la serie basada en el ejemplo del primer capítulo del libro “Refactoring” de Martin Fowler.

En esta entrega vamos a ver cómo nos llevamos la lógica condicional que hay en los métodos Rental.getCharge y Rental.getFrequentRenterPoints a la clase Movie.

Replacing the Conditional Logic on Price Code with Polymorphism

Fowler afirma que es una mala idea basar un switch en un atributo que corresponde a otro objeto. Dado que se basa en un atributo de Movie, el método debería estar en Movie (aunque tengamos que pasar el número de días de alquiler como parámetro).

Los pasos a seguir son los siguientes (suponiendo que tenemos Eclipse):
1. Alt+Shift+V sobre el método Rental.getCharge

  • Seleccionamos Movie
  • Marcamos la opción de crear un método delegado (pero sin marcarlo como @deprecated)
  • Revisamos el javadoc porque ahora el método está en otro objeto y puede tener otras responsabilidades.

Con esto, habremos dejado el código como sigue:

 /**
  * @see Movie#getCharge(int)
  */
 double getCharge() {
  return _movie.getCharge(getDaysRented());
 }
 /**
  * Determine the corresponding amount depending on the type of movie
  * (the price code).
  *
  * @param rental TODO
  * @return
  */
 double getCharge(Rental rental) {
  double thisAmount = 0;
  switch (rental.getMovie().getPriceCode()) {
    case Movie.REGULAR:
      thisAmount += 2;
      if (rental.getDaysRented() > 2)
        thisAmount += (rental.getDaysRented() - 2) * 1.5;
      break;
    case Movie.NEW_RELEASE:
      thisAmount += rental.getDaysRented() * 3;
      break;
    case Movie.CHILDRENS:
      thisAmount += 1.5;
      if (rental.getDaysRented() > 3)
        thisAmount += (rental.getDaysRented() - 3) * 1.5;
      break;
   }
   return thisAmount;
 }

TESTS

2. El atributo sobre el que se hace el switch es equivalente a getPriceCode() puesto que se trata del mismo objeto Movie; así que esa linea pasa de:

  switch (rental.getMovie().getPriceCode()) {

a:

  switch (getPriceCode()) {

TESTS

3. Cambiamos el atributo que pasamos para que, en vez de pasar el objeto Rental, pasar el número de días que se alquila.

  • Hay que cambiar todas las rental.getDaysRented por el parámetro daysRented
  • Cambiamos el javadoc porque ahora el método recibe parámetros distintos.

El código de este método queda como sigue:

 /**
  * Determine the corresponding amount depending on the type of movie
  * (the price code) and the number or days rented.
  *
  * @param daysRented
  * @return
  */
 double getCharge(int daysRented) {
  double thisAmount = 0;
  switch (getPriceCode()) {
    case Movie.REGULAR:
      thisAmount += 2;
      if (daysRented > 2)
        thisAmount += (daysRented - 2) * 1.5;
      break;
    case Movie.NEW_RELEASE:
      thisAmount += daysRented * 3;
      break;
    case Movie.CHILDRENS:
      thisAmount += 1.5;
      if (daysRented > 3)
        thisAmount += (daysRented - 3) * 1.5;
      break;
    }
    return thisAmount;
 }

TESTS

Por simetría (un importante patrón, ver “Implementation Patterns” de Beck), implementamos el método Rental.getFrequentRenterPoints en Movie (pasando el número de días de alquiler como parámetro).

Lo dejo como ejercicio porque es simplemente aplicar la misma receta sobre el método Rental.getFrequentRenterPoints llevando el código al nuevo Rental.getFrequentRenterPoints(days:int).

TESTS

Llegados a este punto… me doy cuenta de que no estamos usando TDD. Es decir, que los tests que tengo no están probando el código que estoy escribiendo. Me refiero a que hemos movido código entre clases y no hemos escrito tests para estas clases.

He escrito MovieTest.

package step5;

import static org.junit.Assert.assertEquals;
import org.junit.Ignore;
import org.junit.Test;

public class MovieTest {
 private static final String ANY_TITLE = "Any Movie Title";
 private static final int UNKNOWN_PRICE_CODE = 99;
 @Test
 public void testMovieRegular() {
  Movie aMovie = new Movie(ANY_TITLE,Movie.REGULAR);
  assertEquals(Movie.REGULAR, aMovie.getPriceCode());
 }

 @Test
 public void testMovieChildren() {
  Movie aMovie = new Movie(ANY_TITLE,Movie.CHILDRENS);
  assertEquals(Movie.CHILDRENS, aMovie.getPriceCode());
 }

 @Test
 public void testMovieNewRelease() {
  Movie aMovie = new Movie(ANY_TITLE,Movie.NEW_RELEASE);
  assertEquals(Movie.NEW_RELEASE, aMovie.getPriceCode());
 }

 @Test(expected=IllegalArgumentException.class)
 @Ignore
 public void testMovieIllegalType() {
  new Movie(ANY_TITLE,UNKNOWN_PRICE_CODE);
 }

 @Test
 public void testFrequentRenterPointsRegular() {
  int points[] = { 1, 1, 1 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.REGULAR);
  for (int daysRented = 1; daysRented <= points.length; daysRented++) {
   assertEquals(points[daysRented-1], aMovie.getFrequentRenterPoints(daysRented));
  }
 }

 @Test
 public void testFrequentRenterPointsChildrens() {
  int points[] = { 1, 1, 1 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.CHILDRENS);
  for (int daysRented = 1; daysRented <= points.length; daysRented++) {
   assertEquals(points[daysRented-1], aMovie.getFrequentRenterPoints(daysRented));
  }
 }

 @Test
 public void testFrequentRenterPointsNewRelease() {
  int points[] = { 1, 2, 2, 2 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.NEW_RELEASE);
  for (int daysRented = 1; daysRented <= points.length; daysRented++) {
   assertEquals(points[daysRented-1], aMovie.getFrequentRenterPoints(daysRented));
  }
 }

 @Test
 public void testChargeForRegular() {
  double charges[] = { 2.0, 2.0, 3.5, 5.0, 6.5, 8.0 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.REGULAR);
  for (int daysRented = 1; daysRented <= charges.length; daysRented++) {
   assertEquals(charges[daysRented-1], aMovie.getCharge(daysRented),0);
  }
 }

 @Test
 public void testChargeForChildrens() {
  double charges[] = { 1.5, 1.5, 1.5, 3.0, 4.5, 6.0, 7.5 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.CHILDRENS);
  for (int daysRented = 1; daysRented <= charges.length; daysRented++) {
   assertEquals(charges[daysRented-1], aMovie.getCharge(daysRented),0);
  }
 }

 @Test
 public void testChargeNewRelease() {
  double charges[] = { 3.0, 6.0, 9.0, 12.0 };
  Movie aMovie = new Movie(ANY_TITLE,Movie.NEW_RELEASE);
  for (int daysRented = 1; daysRented <= charges.length; daysRented++) {
   assertEquals(charges[daysRented-1], aMovie.getCharge(daysRented),0);
  }
 }
}

Añadimos este test a la suite y volvemos a lanzarlos todos.

En resumen, el cambio que hemos hecho ha consistido en crear los métodos Movie.getCharge(days:int) y Movie.getFrequentRenterPoints(days:int) para alojar ahí la lógica de los métodos homónimos en la clase Rental y que dependían de una propiedad de la clase Movie.

En la siguiente entrega (la última), donde haremos un cambio muy serio al diseño puesto que introduciremos nuevas clases (entre ellas Price), podremos ver cómo recuperaremos la inversión que acabamos de hacer en los nuevos tests.

Tags:

12 OctRefactoring en Español (4)

Hace ya un par de semanas que publiqué la última entrega de “Refactoring en Español”. Voy a seguir con el ejemplo de Fowler en el libro “Refactoring” por el punto donde lo dejamos.

Removing Temps

Antes de comenzar hacemos el siguiente refactor “sintáctico”.

Reordenamos en Customer.statement para poner juntas estas lineas y poder cambiar
lo siguiente:

  Enumeration<Rental> rentals = _rentals.elements();
  while (rentals.hasMoreElements()) {
   Rental each = (Rental) rentals.nextElement();
   ...
  }

por:

  for (Rental each: _rentals ) {
    ...
  }

TESTS. Sí, aunque sea un cambio trivial, refactorizar consiste en hacer pequeños cambios pero siempre muy seguros; por eso lanzamos las pruebas unitarias cada vez que realizamos un cambio en nuestro código.

A partir de aquí podemos ver más claramente la necesidad/posibilidad de
trabajar sobre las variables totalAmount y frequentRenterPoints.

Primero trabajamos con totalAmount y creamos el método getTotalCharge como sigue:

 /**
  * Rolls over the list of rentals calculating
  * the total amount to be charged.
  *
  * @return
  */
 private double getTotalCharge() {
  double totalAmount = 0;
  for (Rental each: _rentals ) {
   totalAmount += each.getCharge();
  }
  return totalAmount;
 }

Obsérvese que lo hacemos replicando el bucle que recorre la lista de Rentals y
moviendo el código que suma la cantidad correspondiente del bucle original al
del método (que se ejecuta al final del bucle original y fuera de éste).

Este cambio es conceptualmente difícil de concebir e implementar (este ejemplo es
un ejercicio de laboratorio y en la práctica nos podemos encontrar con casos mucho
más difíciles de ver).

El método statement queda como sigue después de este cambio:

 public String statement() {
  int frequentRenterPoints = 0;
  String result = "Rental Record for " + getName() + "\n";
  for (Rental each: _rentals ) {
   frequentRenterPoints += each.getFrequentRenterPoints();
   // show figures for this rental
   result += "\t" + each.getMovie().getTitle() + "\t"
     + String.valueOf(each.getCharge()) + "\n";
   totalAmount += each.getCharge();
  }
  // add footer lines
  result += "Amount owed is " + String.valueOf(getTotalCharge()) + "\n";
  result += "You earned " + String.valueOf(frequentRenterPoints)
    + " frequent renter points";
  return result;
 }

TESTS. En este caso está más justificado que nunca puesto que no se trata de un cambio nada trivial.

A continuación aplicamos la misma receta a frequentRenterPoints

El método statement queda finalmente como sigue:

 public String statement() {
  String result = "Rental Record for " + getName() + "\n";
  for (Rental each: _rentals ) {
   // show figures for this rental
   result += "\t" + each.getMovie().getTitle() + "\t"
     + String.valueOf(each.getCharge()) + "\n";
  }
  // add footer lines
  result += "Amount owed is " + String.valueOf(getTotalCharge()) + "\n";
  result += "You earned " + String.valueOf(getTotalFrequentRenterPoints())
    + " frequent renter points";
  return result;
 }

TESTS. No olvidemos ejecutarlos después de cada cambio.

Finalmente, incidir en el comentario que hace Fowler sobre el hecho de que, tras este
cambio, tenemos tres bucles que recorren tres veces la lista de Rentals. Esto lo
tendremos en cuenta cuando estemos mejorando el rendimiento, pero no ahora porque
lo que nos preocupa es mejorar el diseño (sin incumplir con los requisitos), y con
los cambios efectuados hemos mejorado el diseño y la legibilidad del código, lo
que nos llevará a reducir los errores y aumentará la reusabilidad. (El ejemplo de
Fowler es muy bueno: ahora podemos escribir un método htmlStatement que imprime
el recibo en formato HTML, pero reutilizando todo el código que no tiene que ver
con la estética del mismo).

Tags:

21 SepRefactoring en Español (3)

Seguimos con el ejemplo de refactor del ejemplo del “videoclub”.

En esta entrega vamos a hacer un cambio parecido al de la entrega anterior (donde transformamos el método Customer.amountFor a Rental.getCharge), pero en este caso moveremos crearemos el método Customer.getFrequentRenterPoints y luego lo moveremos a Rental.

Extracting Frequent Renter Points

Decíamos al final de la entrega anterior que Fowler proponía un último refactor, pero que lo posponíamos. Ahora sí lo hacemos porque veréis que, en mi modesta opinión, es más afín a los siguientes cambios.

1. En Customer.statement sustituimos las ocurrencias de thisAmount por each.getRental(). (En Eclipse, si hacemos doble-click sobre thisAmount veremos que se iluminan todos los puntos donde se usa la variable).
2. Eliminamos la declaración de thisAmount

El código queda como:

        while (rentals.hasMoreElements()) {
            Rental each = (Rental) rentals.nextElement();
            // add frequent renter points
            frequentRenterPoints++;
            // add bonus for a two day new release rental
            if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
                    && each.getDaysRented() > 1)
                frequentRenterPoints++;
            // show figures for this rental
            result += "\t" + each.getMovie().getTitle() + "\t"
                    + String.valueOf(each.getCharge()) + "\n";
            totalAmount += each.getCharge();
        }

Hemos hecho este cambio para así eliminar el uso de thisAmount y ver más claro
el uso que se está haciendo de las demás variables locales. ¡No olvidemos pasar los tests!

Ahora nos fijamos en frequentRenterPoints.

Extraemos el método getFrequentRenterPoints (marcando desde el comentario “add frequent renter points” hasta el comentario “show figures for this rental” y haciendo Alt+Shift+M), pero nos quedará “un poco raro” porque estaremos incrementando la variable y luego asignando su valor en Customer.statement. Así que seguiremos refactorizando hasta tener en Customer.statement la siguiente linea:

            frequentRenterPoints += getFrequentRenterPoints(each);

Obsérvese que:
a) quitamos el parámetro frequentRenterPoints de la firma del método getFrequentRenterPoints que nos ha generado Eclipse. Hay que cambiarlo a mano, ahí ya no nos puede ayudar Eclipse. :-)
b) acumulamos el valor fuera del método (de ahí el “+=”); hay que cambiar el código del método para mantener la integridad semántica. Customer.getFrequentRenterPoints devuelve los valores 1 o 2 (dependiendo de si le corresponde bonus o no a la película que se está alquilando).
c) dado que, no sólo creamos un nuevo método sino que además cambiamos la semántica de las líneas de código originales, es muy importante que pongamos atención al javadoc del método

    /**
     * Returns the renter points corresponding to each rental. A two day new
     * release rental comes with a bonus.
     *
     * @param each
     * @return
     */
    private int getFrequentRenterPoints(Rental each) {
        // add bonus for a two day new release rental
        if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE)
                && each.getDaysRented() > 1) {
            return 2;
        }
        return 1;
    }

TESTS

Después de este cambio, vemos que Customer.getFrequentRenterPoints no tiene nada
que ver con Customer, por lo que hacemos lo mismo que hicimos con Rental.getCharge
y nos llevamos este método a Rental.

Para ello usamos Alt+Shift+V (pero esta vez desmarcamos la opción de crear el método
delegado), con lo que directamente Eclipse nos cambia la linea anterior a:

            frequentRenterPoints += each.getFrequentRenterPoints();

El resultado final de Rental.getFrequentRenterPoints es:

    /**
     * Returns the renter points corresponding to the rental.
     * A two day new release rental comes with a bonus.
     *
     * @param frequentRenterPoints
     * @return
     */
    int getFrequentRenterPoints() {
        if ((getMovie().getPriceCode() == Movie.NEW_RELEASE)
                && getDaysRented() > 1) {
            return 2;
        } else {
            return 1;
        }
    }

Observad el matiz del cambio en el javadoc.

TESTS y listo.

12 SepRefactoring en Español (2)

En esta nueva entrega sobre refactorizar, basada en el ejemplo del primer capítulo de “Refactoring“, vamos a ver el apartado “Moving the Amount Calculation”, donde Fowler nos enseña cómo cambiar el diseño y llevar código de una clase a otra.

Moving the Amount Calculation

Como el método Customer.amountFor (el que hemos creado como consecuencia del refactor de la entrega anterior) usa datos de Rental pero no de Customer, parece que es mejor moverlo a Rental.

Para ello seguimos los siguientes pasos:

1. Si usamos Eclipse, colocamos el cursor sobre el nombre del método y hacemos botón derecho + Refactor / Move. En el diálogo que nos muestra Eclipse:
* Seleccionamos la clase Rental
* Marcamos para que el método actual delegue en el nuevo y que quede marcado como @deprecated
* Podemos ver con Preview cómo van a quedar

2. Ejecutamos los tests para comprobar que los cambios no han estropeado nada.

3. Renombramos el método a Rental.getCharge (y ponemos el javadoc correspondiente). Para renombrar un método, Eclipse proporciona varios caminos: Alt+Shift+ R (el camino corto), Alt+Shift+T + Rename (el camino intermedio) y botón derecho + Refactor + Rename (el camino largo). La ventaja de usar Eclipse para renombrar métodos es que nos va a cambiar el método no sólo en la clase que estamos editando sino también en todas aquellas clases que usen el método (las tengamos abiertas para editar o no).

Hago hincapié en que debemos escribir el javadoc de Rental.getCharge porque cuando cambiamos un método de clase normalmente estamos cambiando la intención del mismo y debemos comunicar esto a los usuarios del método. Para comunicar el cambio usamos normalmente el nombre del método pero no podemos olvidar el comentario javadoc porque puede introducir errores de interpretación si nos dejamos llevar y no los actualizamos (no sólo incluyendo los @deprecated, y cambiando los @param donde toque sino cambiando el texto que explica el comportamiento de los métodos).

4. Volvemos a ejecutar los tests.

A continuación comprobamos que podemos eliminar el método “viejo” (que hemos marcado como @deprecated). (Fowler comenta que en ocasiones podemos preferir dejar el método “viejo” como un delegado al “nuevo” porque, por ejemplo, forme parte de la interfaz pública de la clase).

5. Comprobamos que el método Rental.getCharge sólo es llamado desde Customer.amountFor
y éste sólo desde Customer.statement (con Ctr+Alt+H).

6. Sustituimos la llamada en Customer.statement directamente a Rental.getCharge
thisAmount = each.getCharge();

7. TESTS

8. Comprobamos que Customer.amountFor ya no se usa (con Ctr+Alt+H de nuevo) y lo eliminamos (sin miedo)

9. TESTS

Fowler llama “Move Method” a la refactorización de código que consiste en mover un método de una clase a otra y es el primero de los que propone en el capítulo 7 : “Moving Features Between Objects”. Ahí se explican tanto las motivaciones que nos deberían llevar a realizar este cambio como los pasos a seguir para realizarlo de manera mecánica.

Fowler propone “Replace Temp with Query” en statement para sustituir las ocurrencias
de thisAmount por each.getCharge. (Yo no lo voy a implementar aún).

Recordatorio:

Que no se me olvide, en la entrada anterior “Refactoring en Español (1)” tenéis los enlaces al código con los tests y el código “copi-pegado” del libro.

03 SepRefactoring en Español (1)

Éste es el primero de una serie de artículos con los que pretendo seguir, paso a paso, el ejemplo que Martin Fowler usa en el primer capítulo de su “Refactoring” para explicar los fundamentos de esta técnica de desarrollo que consiste en mejorar el diseño del código sin cambiar el comportamiento del mismo.

Lo primero que he hecho es copi-pegar el código del que parte el ejemplo y a continuación he escrito una batería de tests con los que pretendo capturar los requisitos del programa. Tengo todo el código, pero sólo voy a publicar los tests porque creo que lo interesante es que cada uno haga el ejercicio por su cuenta. De todos modos, si alguien está muy, muy interesado en cada uno de los pasos resueltos, que me los pida. :-)

El primer paso consiste en probar que funcionan los tests. Si alguno no pasa es porque algo tenemos mal configurado.

A continuación, he querido poner comentarios en las clases y el método Customer.statement para darle más claridad a lo que pretendemos hacer.

Seguidamente yo he decidido quitar los warnings en la compilación (estoy usando JDK 6) para lo que he tenido que usar generics en la clase Customer (para dejar bien claro que _rental es una colección de Rental).

Y por fin llegamos al primer refactor de verdad (“Decomposing and Redistributing the Statement Method”). Yo he seguido los siguientes pasos (usando Eclipse como IDE es más fácil porque tiene ciertas ayudas muy útiles):

Decomposing and Redistributing the Statement Method

1. Colocamos el cursor sobre thisAmount en la linea donde se declara esta variable, pulsamos Ctrl+1 y elegimos “Split variable declaration”. De esta manera separamos la declaración de la asignación.

2. Movemos la asignación (thisAmount = 0) hasta después del comentario que dice:
// determine amounts for each line

3. Extraemos el método amountFor desde el comentario hasta el final del switch. Para ello seleccionamos todo ese bloque de código (incluido el comentario), pulsamos Alt+Shift+M (también podemos hacer Ctrl+1 + “Extract method”) y elegimos el nombre amountFor para el nuevo método.

4. En el método amountFor hacemos Ctrl+1 + “Join split declaration” para thisAmount.

5. Idem en el método statement (también para thisAmount).

6. Movemos el comentario “inline” al javadoc del método amountFor.

7. Cambiamos el nombre del atributo each a aRental (usando Ctrl+2).

Lo correcto es ejecutar los tests después de cada paso (si movemos comentarios, no, claro). Nos puede parecer una tontería ejecutar los tests después de un cambio tan “tonto” como los que se hacen en 4 ó 5, pero se trata de una técnica que se basa en realizar avances pequeños pero siempre con seguridad. Esta técnica nos dice que, si en algún momento pasamos de “verde a rojo” (de pasar todos los tests a que alguno falle), deshagamos los cambios y volvamos a realizarlos. SIN CONTEMPLACIONES. El fundamento de esto es claro: si hacemos un cambio sobre un código que está “en rojo” corremos el riesgo de no arreglar lo que falla e introducir nuevos errores. Por ello, lo mejor es volver “a terreno seguro” y replantearse el cambio de nuevo.

Bueno, en esta entrega hemos visto el refactor más sencillo (Extract Method) y en la siguiente entrega veremos un paso más en el ejemplo de Fowler (“Moving the Amount Calculation”) donde veremos otro refactor más complicado (Move Method) y seguiremos comprobando cómo Eclipse nos ayuda en estas tareas que, para hacerlas bien, hay que ser muy meticuloso.

Corrección:
Estaba preparando la siguiente entrega de esta serie y me he dado cuenta de que no había incluido el enlace al código y a los tests. :-(