Llevamos ya un tiempo hablando de como refactorizar una aplicación legacy en PHP. Desde que empezamos hemos ido aprendiendo poco a poco a ir reescribiendo código sin perder la funcionalidad de la aplicación. Llegados a este punto, nuestra aplicación legacy ya tiene algunos tests unitarios, utilizamos mocks en algunas partes y lo más importante: Estamos aprendiendo a conocer todo el sistema, hemos leído el código unas cuantas veces y ahora sí podemos atacar a los métodos más complejos.
En mi cuenta de github está todo el proceso que hemos llevado a cabo, además mediante los tags podemos ir viendo como hemos ido evolucionando esta aplicación legacy en PHP para ir convirtiéndola en una aplicación PHP con tests, fácil de entender y con funciones pequeñas.
Entendiendo el método roll()
Cuando leemos el método roll nos damos cuenta de que no devuelve nada por lo que puede que este método sea algo difícil de probar. Así que lo mejor que podemos ir haciendo son pequeñas refactorizaciones automáticas con nuestro IDE y algún “rename” de variables que veamos clarísimo. Si vemos que la refactorización se pone un poco cuesta arriba siempre podemos utilizar el test master comparando ficheros de texto.
Una de las primeras cosas que podemos ir haciendo es ver que pasa con el parámetro de entrada $roll. ¿que es un “roll”? ¿Un número?¿una acción? Si leemos todas vamos analizando donde se utiliza el parámetro “$roll” podemos ver que por ejemplo en echoln("They have rolled a " . $roll); se utiliza como un número. Así que quizás podríamos renombrar “$roll” como “$rolledNumber”. Quizás te preguntes si esto es importante y la respuesta es que estamos intentando hacer más entendible un sistema legacy así que cada pasito que demos, por pequeño que parezca, cuenta.
Ahora vamos a dar otro pequeño pasito refactorizando el else. Analizando dentro del bloque se chequea si se debería empezar una nueva vuelta, se imprimen unas cuantas cosas y se realiza una pregunta.
} else {
$this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] + $rolledNumber;
if ($this->playerShouldStartANewLap()) {
$this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - $boardSize;
}
echoln(
$this->players[$this->currentPlayer]
. "'s new location is "
. $this->places[$this->currentPlayer]
);
echoln("The category is " . $this->currentCategory());
$this->askQuestion();
}
Lo primero será intentar escribir un test que pruebe la 2º parte del if. Una de las cosas más complicadas cuando testeamos legacy code es saber como preparar/llegar a un estado para poder verificarlo. En Game ya sabemos crear un una nueva instancia del mismo, ahora solo tenemos que setear las variables para “entrar” en el else.
public function testSecondPartOfRollMethod()
{
$this->_game->currentPlayer = 0;
$this->_game->players[$this->_game->currentPlayer] = 'Jeff';
$this->_game->inPenaltyBox[$this->_game->currentPlayer] = false;
$this->_game->roll(1);
}
De momento esto solo es un test para chequear que entramos en el else, la salida puede ayudarnos un poco para intentar hacer algún assert con los que chequear el estado del jugador. Por ejemplo si un jugador está en la posición 2 y saca un 1 (RolledNumber) entonces irá a la posición 3.
public function testSecondPartOfRollMethod()
{
$currentPlace = 2;
$rolledNumber = 1;
$this->_game->currentPlayer = 0;
$this->_game->players[$this->_game->currentPlayer] = 'Jeff';
$this->_game->inPenaltyBox[$this->_game->currentPlayer] = false;
$this->_game->places[$this->_game->currentPlayer] = $currentPlace;
$this->_game->roll($rolledNumber);
$this->assertEquals($currentPlace + $rolledNumber, $this->_game->places[$this->_game->currentPlayer], 'The player was expected at position '.$currentPlace+$rolledNumber);
}
Ya tenemos un test, pero podemos ver que es un poco feo. Aunque más que feo quizás sea un test un poco difícil de entender, así que antes de empezar a refactorizar el método roll debemos darle un retoque al test. Además podemos testear que la categoría de la pregunta numero 3 es “Rock”. Así que después de extraer a métodos todo el “seteo” del estado tenemos un test como este:
public function testSecondPartOfRollMethod()
{
$currentPlace = 2;
$rolledNumber = 1;
$this->setAPlayerNotInPenaltyBox();
$this->setCurrentPlayerPosition($currentPlace);
$this->_game->roll($rolledNumber);
$this->assertEquals($currentPlace + $rolledNumber, $this->_game->places[$this->_game->currentPlayer], 'The player was expected at position '.$currentPlace+$rolledNumber);
$this->assertEquals('Rock', $this->_game->currentCategory());
}
Pero con este test no probamos toda la lógica. Como medida para saber si nuestros test cubren todos los casos podemos utilizar “CodeCoverage”, pero ojo, esto no significa que si una parte de nuestro código está en verde signifique que la estamos probando. El code coverage o al menos de la manera como lo utilizo yo, es para saber si hemos pasado por todas las ramas de los if. En nuestro caso necesitamos entrar por otra parte del código.
Este es nuestro tests:
public function testAPlayerWillStarANewLapWhenNeeded()
{
$currentPlace = 11;
$rolledNumber = 2;
$this->setAPlayerNotInPenaltyBox();
$this->setCurrentPlayerPosition($currentPlace);
$this->_game->roll($rolledNumber);
$this->assertEquals(
1,
$this->_game->places[$this->_game->currentPlayer],
'The player was expected at position 1');
$this->assertEquals('Rock', $this->_game->currentCategory());
}
Pero tenemos un problema, el test falla. Básicamente hemos copiado y pegado el código, pero ahora tenemos el inconveniente de que la categoría del test no concuerda. Una de las cosas que podemos hacer es modificar el test anterior para que solo tenga un assert (el de la posición) y crear test específicos para las categorías.
public function testAPlayersNextPositionIsCorrectlyDeterminedWhenNoNewLapIsInvolved()
{
$currentPlace = 2;
$rolledNumber = 1;
$this->setAPlayerNotInPenaltyBox();
$this->setCurrentPlayerPosition($currentPlace);
$this->_game->roll($rolledNumber);
$this->assertEquals(
$currentPlace + $rolledNumber,
$this->_game->places[$this->_game->currentPlayer],
'The player was expected at position ' . $currentPlace + $rolledNumber
);
}
public function testAPlayerWillStarANewLapWhenNeeded()
{
$currentPlace = 11;
$rolledNumber = 2;
$this->setAPlayerNotInPenaltyBox();
$this->setCurrentPlayerPosition($currentPlace);
$this->_game->roll($rolledNumber);
$this->assertEquals(
1,
$this->_game->places[$this->_game->currentPlayer],
'The player was expected at position 1');
}
Todavía no tenemos una “constante”, ni nada parecido para el tamaño del tablero así que no en el 2ºtest no podemos tener una fórmula como la del primer test. Solo nos queda testear las categorías, para ello los pasos serán:
- Crear un test para chequear que la categoría “Rock” aparezca en la posición 3
- Crear un test para chequear que la categoría “Science” aparezca en la posición 1
- Ver que el código es idéntico, refatorizar creando un método (customAssert) en el que dada la posición (currentPlaces) y la categoría esperada (ExpectedCategory) nos devuelva true o false.
public function testRockCategoryCanBeDetermined()
{
$currentPlaces = [3];
$expectedCategory = 'Rock';
$this->assertCorrectCategoryForGivenPlaces($currentPlaces, $expectedCategory);
}
protected function assertCorrectCategoryForGivenPlaces($currentPlaces, $expectedCategory)
{
foreach ($currentPlaces as $currentPlace) {
$this->setAPlayerNotInPenaltyBox();
$this->setCurrentPlayersPosition($currentPlace);
$foundCategory = $this->_game->currentCategory();
$this->assertEquals(
$expectedCategory,
$foundCategory,
'Expected' . $expectedCategory . 'category for position ' . $currentPlace .
' but got ' . $foundCategory
);
}
}
Algo así para el resto de categorías. Si bien es cierto, todavía no debemos preocuparnos por el método de las categorías, más adelante lo testearemos. Ahora estamos con roll y ya lo tenemos suficientemente cubierto con test como para empezar a refactorizarlo. Lo primero será seguir el mismo procedimiento. Extraer todo lo que no sea presentación a un método así:
} else {
$this->movePlayer($rolledNumber, $boardSize);
echoln(
$this->players[$this->currentPlayer]
. "'s new location is "
. $this->places[$this->currentPlayer]
);
echoln("The category is " . $this->currentCategory());
$this->askQuestion();
}
Nuestro método necesita 2 parámetros, el tamaño del tablero y el numero de movimientos. Más adelante veremos como cambiar lo del tamaño del tablero.
Aunque podemos segur refactorizando para separar e intentar aislar la responsabilidad de parar la capa de presentación.
} else {
$this->movePlayer($rolledNumber, $boardSize);
$this->displayPlayerNewLocation();
$this->displayCurrentCategory();
$this->askQuestion();
}
Ahora tenemos una parte del método entendible, con nombre de métodos explicativos y responsabilidades separadas. ¿Qué hacemos con el resto del método?
Hasta ahora hemos ido afrontando pequeños retos, funciones de pocas lineas que podíamos analizar linea a linea y entender a la perfección. Ahora nos enfrentamos a un reto mayor, no es solo extraer un par de lineas a un método, es nuestro mayor reto hasta ahora.
Llegados a este punto debemos evaluar la complejidad, el diseño del código y analizar como queremos seguir haciendo evolución nuestra aplicación legacy.
Si nos fijamos en el código antes de refactorizarlo, vemos que hay una gran cantidad de código duplicado en cada una de las ramas del if.
Refactorizando roll: todo esta testeado
Ahora viene la parte más compleja, ver partes de código que estén duplicadas y extraer esa funcionalidad a una función. Esto hay que hacerlo en pequeños pasos y de forma secuencial. Teniendo en cuenta que antes debemos tener test que cubran el método. En este momento no debemos centrarnos en el código sino alejarnos un poco y ver que funcionalidades están repetidas para poder agruparlas.
Llegar a entender el código desde “vista de pajaro” no es una tarea sencilla, es necesario que nos tomemos un tiempo y vayamos viendo y haciendo “debug mental” para llegar a las partes de código repetido dentro de nuestra aplicación legacy.
Seguramente en esta parte del camino veamos muy claro los cambios a realizar. No los haremos. No tenemos test que nos aseguren que la integridad de la aplicación no se ve comprometida. Debemos testearlo todo, así que vamos a por el otro else
} else {
echoln($this->players[$this->currentPlayer] . " is not getting out of the penalty box");
$this->isGettingOutOfPenaltyBox = false;
}
Estos podrían ser los tests de lo que queda del método roll.
public function testAPlayerWhoIsPenalizedAndRollsAnEvenNumberWillStayInThePenaltyBox()
{
$rolledNumber = 2;
$this->setAPlayerInPenaltyBox();
$this->_game->roll($rolledNumber);
$this->assertFalse($this->_game->isGettingOutOfPenaltyBox);
}
public function testPlayerGettingOutOfPenaltyNextPositionWithNewLap()
{
$currentPlace = 11;
$numberRequiredToGetOutOfPenaltyBox = 3;
$this->setAPlayerInPenaltyBox();
$this->setCurrentPlayersPosition($currentPlace);
$this->_game->roll($numberRequiredToGetOutOfPenaltyBox);
$this->assertEquals('2', $this->getCurrentPlayersPosition(), 'Player was expected at position 3');
}
Con estos tests y los que hicimos antes, ya tenemos cubierto todo el método roll y podemos ir analizando como afrontar la refactorización completa del método.
Estamos en unos pasos críticos y debemos estar muy concentrados y concienciados con los cambios que realizamos a nuestra aplicación legacy. Como vemos en esta función hay cantidad de código duplicado, por lo que debemos testar bien toda la función roll para más adelante hacer un buen refactor.
Una buena manera es hacer refactor por parejas, ya que teniendo distintos puntos de vista podemos ir codificando de manera que nuestro código sea más entendible. Una típica discusión a la hora de abordar una refactorización como esta del método roll es primero eliminar toda la duplicidad y luego mejorar el naming de las variables o como lo hemos hecho nosotros primero haciendo pasos pequeños hasta llegar a tener refactorizada una parte de la función y ahora continuar con el resto de la función.
Con un poco de vista de pájaro y sobre todo con los tests que llevamos podemos llegar a una función rollcomo esta.
function roll($rolledNumber)
{
$this->displayCurrentPlayer();
$this->displayRolledNumber($rolledNumber);
$boardSize = 12;
if ($this->inPenaltyBox[$this->currentPlayer]) {
if ($this->isOdd($rolledNumber)) {
$this->isGettingOutOfPenaltyBox = true;
echoln($this->players[$this->currentPlayer] . " is getting out of the penalty box");
$this->movePlayer($rolledNumber, $boardSize);
$this->displayPlayerNewLocation();
$this->displayCurrentCategory();
$this->askQuestion();
} else {
echoln($this->players[$this->currentPlayer] . " is not getting out of the penalty box");
$this->isGettingOutOfPenaltyBox = false;
}
} else {
$this->movePlayer($rolledNumber, $boardSize);
$this->displayPlayerNewLocation();
$this->displayCurrentCategory();
$this->askQuestion();
}
}
Hemos mejorado ¿no? Tenemos métodos pequeños, las responsabilidades separadas en funciones con un namming aceptable y un code style que hace que nuestro código sea legible. Pero todavía podemos ir más allá nuestro código no está completamente refactorizado, el método es muy grande como para comprender de un vistazo todo lo que realiza. Es necesario que lleguemos un poco más lejos para dar por terminada la refactorización del método, lo que si es cierto es que vamos por buen camino.
Los pasos que hemos seguido son más o menos estos:
- Agrupamos las 2 primeras llamadas a métodos justo al principio de roll en un método llamado displayStatusAfterRoll
- Integrar la variable $boarSize dentro del metodo movePlayer
- Integrar en un método llamado playNextMove las funciones movePlayer($rolledNumber), displayPlayerNewLocation(), displayCurrentCategory(), askQuestion().
- Agrupar el if (isOdd) completo, con su else y todo en una función llamada playNextMoveForPlayerInPenaltyBox
Cambinando estos pequeños pasos llegaremos a:
function roll($rolledNumber)
{
$this->displayStatusAfterRoll($rolledNumber);
if ($this->inPenaltyBox[$this->currentPlayer]) {
$this->playNextMoveForPlayerInPenaltyBox($rolledNumber);
} else {
$this->playNextMove($rolledNumber);
}
}
FInalmente llegamos a una función de unas 7 lineas con un simple if a las que hemos podido llegar después de recorrer un camino. Utilizando el IDE y “Extract Method” hemos ido dando pequeños pasos y realizando hasta llegar a esto.
Conclusiones
Hemos aprendido mucho en este paso, llegamos con un método larguísimo y bastante enrevesado y llegamos a tener un método de 7 lineas. Utilizar un buen IDE es crucial para la tarea de refactorizar aplicaciones legacy. De la misma manera hay veces en las que es mejor parar y analizar el código con vista de pájaro para ver si tenemos mucho código repetido para poder atajarlo.
Aunque parezca un camino de rosas, para llegar aquí hemos tenido que esforzarnos y dar pasos adelante y atrás hasta encontrar el camino correcto. Lo importante a la hora de refactorizar es tener test en los que basarte para cuando estemos refactorizando poder comprobar que nuestros pasos son correctos. Muchas veces refactorizando aplicaciones llegaremos a callejones sin salida en los que tendremos que volver al ultimo commit o 2 commit atrás. Eso no es un inconveniente, sino todo lo contrario, tenemos que aprender y para ello tenemos que equivocarnos.
Este post está basado en Refactoring Legacy Code: Part 6.
Jesús L.C. - Apuntes de un aprendiz 