Refactoring 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:

 /**
  *  Movie#getCharge(int)
  */
 double getCharge() {
  return _movie.getCharge(getDaysRented());
 }
 /**
  * Determine the corresponding amount depending on the type of movie
  * (the price code).
  * 
  *  rental TODO
  *  
  */
 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.
  * 
  *  daysRented
  *  
  */
 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;
  
 public void testMovieRegular() {
  Movie aMovie = new Movie(ANY_TITLE,Movie.REGULAR);
  assertEquals(Movie.REGULAR, aMovie.getPriceCode());
 }

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

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

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

  
 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));
  }
 }

  
 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));
  }
 }

  
 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));
  }
 }

  
 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);
  }
 }

  
 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);
  }
 }

  
 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.

Tagged: