Hace unos días empezamos a refactorizar una aplicación legacy escrita en PHP, aquí tenéis los primeros pasos para empezar dejé el código en github. Cada post tendrá un tag en github.
Ahora vamos a seguir refactorizando y siguiendo las indicaciones de Paul M. Jones lo primero será intentar ir eliminando los includes para después limpiar un poco el código.
La mejor manera de ir entendiendo el código es eliminar “magic strings” y “magic constant” ya que de esta manera leeremos bastante código que luego podremos ir refactorizando.
Eliminando los include, require,…
Si vemos el código de GameRunner.php tenemos include_once __DIR__ . '/Game.php'; si la aplicación crece esto empezará a ser un problema. Quizás haya escépticos que digan que con includes la aplicación va más rápida porque solo se carga lo necesario. Si ese es vuestro único problema al refactorizar, olvidaos de refactorizar.
Como ya tenemos añadido composer para cargar PHPUnit solo tenemos que añadir estas lineas a composer
"autoload": {
"psr-4" : {
"GameLegacy\\": "GameLegacy/"
},
"classmap": ["GameLegacy/"]
},
modificamos en GameRunner esta linea include_once __DIR__ . '/Game.php'; por esta require __DIR__ . '/../vendor/autoload.php'; y solo tenemos que ejecutar $composer update
Quizás ahora no veamos ninguna mejora, pero más adelante nos alegraremos 🙂
Magic Strings
Vamos a empezar a leer el código de Game.php buscando string en el código tendremos conocimientos del código heredado y podremos utilizar dichos conocimientos para mejorarlo. Encontraremos muchos strings, unos que podremos refactorizar y otros que no, solo el tiempo y la experiencia nos dirán que hacer con cada pieza de código legacy.
for ($i = 0; $i < 50; $i++) {
array_push($this->popQuestions, "Pop Question " . $i);
array_push($this->scienceQuestions, ("Science Question " . $i));
array_push($this->sportsQuestions, ("Sports Question " . $i));
array_push($this->rockQuestions, $this->createRockQuestion($i));
}
}
function createRockQuestion($index)
{
return "Rock Question " . $index;
}
Analizando el código, este trozo es el mejor para empezar. A primera vista parece que no hay mucha coherencia, ya que depende del tipo de categoría de la pregunta se hace una concatenación con el string o se llama a una función en el caso de rock. Así que manos a la obra.
Actualizando los tests para comparar.
Antes de meter mano al código, lo mejor será crear un nuevo test para comparar la salida correcta del paso anterior (está añadida en el repositorio) con la salida que tendrá ahora.
function testOutputMatchWithMaster()
{
$masterOutput = __DIR__ . '/../MasterOutput.txt';
$times = 20000;
$actualPath = '/tmp/actual.txt';
$this->generateManyOutputs($times, $actualPath);
$fileContentMaster = sha1(file_get_contents($masterOutput));
$fileContentActualOutput = sha1(file_get_contents($actualPath));
$this->assertTrue($fileContentMaster == $fileContentActualOutput); }
Aquí tenemos el test que compara las salidas de una manera ágil, así que podemos empezar a modificar el código.
Marcaremos el testGenerateOutput() como Skipped para no ejecutarlo de momento, aunque también podríamos borrarlo.
Haciendo nuestros primeros cambios de código
Lo mejor para esto es tener un buen IDE que nos ayude a modificar los strings, a comprobar los usos de una función, que nos ayude con los renombramientos,… particularmente yo uso PHPStorm.
Lo primero que haremos será coger el código del método generateRockQuestion() y meteremos el código dentro del bucle. Este tipo de refactoring se le llama Inline Method.
“Put the method’s body into the body of its callers and remove the method.” ~ Martin Fowler
Estos son los pasos a seguir:
- Ver que el método no es poliformico -> como solo tenemos una clase no hay problema.
- Encontrar todas las llamadas al método -> con PHPStorm podemos encontrar todos los usos con un comando.
- Reemplazar cada llamada al método por el cuerpo del mismo-> Es la operación más delicada.
- Borrar la definición del método
Así es como queda el método después de refactorizar
for ($i = 0; $i < 50; $i++) {
array_push($this->popQuestions, "Pop Question " . $i);
array_push($this->scienceQuestions, "Science Question " . $i);
array_push($this->sportsQuestions, "Sports Question " . $i);
array_push($this->rockQuestions, "Rock Question " . $i);
}
Ahora ejecutamos los tests y vemos si se ha roto algo después de hacer los cambios. Si todo ha ido correctamente podemos hacer un commit y continuar con el proceso.
Otras cadenas a tener en cuenta
Existen strings en otros métodos pero no todos son “refactorizables”, como por ejemplo echoln. Un método que podemos refactorizar es currentCategory(), tan solo con un vistazo podemos ver maneras de mejorarlo. Para refactorizarlo introduciremos unas variable local. Los pasos a seguir son:
- Añadir una variable con el valor deseado.
- Encontrar todos los usos del valor
- Remplazar todos los usos.
function currentCategory()
{
$popCategory = "Pop";
$scienceCategory = "Science";
$sportsCategory = "Sports";
$rockCategory = "Rock";
if ($this->places[$this->currentPlayer] == 0) {
return $popCategory;
}
if ($this->places[$this->currentPlayer] == 4) {
return $popCategory;
}
if ($this->places[$this->currentPlayer] == 8) {
return $popCategory;
}
if ($this->places[$this->currentPlayer] == 1) {
return $scienceCategory;
}
if ($this->places[$this->currentPlayer] == 5) {
return $scienceCategory;
}
if ($this->places[$this->currentPlayer] == 9) {
return $scienceCategory;
}
if ($this->places[$this->currentPlayer] == 2) {
return $sportsCategory;
}
if ($this->places[$this->currentPlayer] == 6) {
return $sportsCategory;
}
if ($this->places[$this->currentPlayer] == 10) {
return $sportsCategory;
}
return $rockCategory;
}
Así creo que está algo mejor, ahora tenemos que ejecutar de nuevo los tests y ver si pasan para hacer un commit. Seguro que leyendo el código has encontrado muchas mejoras que podríamos hacer. ¡No las hagas! Empezar a hacer cambios sin pararnos a pensar puede llevarnos a callejones sin salida. Podemos ir anotando con TODO’s los lugares donde veamos posibles mejoras.
De momento no vamos a realizar más cambios en strings, más adelante cuando tengamos una capa de presentación más acotada, podremos realizar más mejoras.
Magic Constants
Las “Magic Constants” son parecidas a las “String Constants”. Los valores de estas constantes pueden ser boolean o números. Nos centraremos en los números utilizados en las guardas de un if o en los returns.
Echándole un vistazo al archivo GameRunner.php seguro que podemos refactorizar un poco el código.
Viendo fragmentos como este rand(0, 5) + 1 podemos extraerlo en una variable. Teniendo en mente que este código legacy es un juego en el que los jugadores van moviéndose por un tablero y que este fragmento devuelve un número entre 1 y 6. Así que podemos poner como nombre a la variable $dice.
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
¿A que mola? Utilizando PhpStorm para refactorizar extraer la funcionalidad a una variable. Ahora volveremos a ejecutar los tests para dar el siguiente paso refactorizar la guarda de este if.
if (rand(0, 9) == 7)
Analizando un poco el código, vemos que una pregunta es incorrecta si el valor aleatorio es 7. El valor aleatorio debe estar entre 0 y 9, por tanto podemos refactorizar el código para obtener algo como esto:
$minAnswerId = 0;
$maxAnswerId = 9;
$wrongAnswerId = 7;
if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId)
Volvemos a ejecutar los test y todos siguen en verde :). Viendo un poco el código podemos ver que va mejorando pero aún queda mucho trabajo por hacer. Refactorizar una aplicación PHP es un trabajo lento y hay que ser cuidadoso con los cambios a realizar.
Ahora vamos a abrir el archivo Game.phpen busca de nuevas magic constants como por ejemplo el 50 del bucle for de categorías (alrededor de la linea 35). Primero debemos entender que es el 50 para poder ponerle un nombre que sea descriptivo. Viendo el código parece el numero de preguntas de cada categoría.
$categorySize = 50;
for ($i = 0; $i < $categorySize; $i++) {
array_push($this->popQuestions, "Pop Question " . $i);
array_push($this->scienceQuestions, "Science Question " . $i);
array_push($this->sportsQuestions, "Sports Question " . $i);
array_push($this->rockQuestions, "Rock Question " . $i);
}
Con este proyecto de ejemplo podemos empezar a utilizar utilidades de nuestro IDE que habitualmente no utilizamos y que pueden sernos muy útiles, por ejemplo extrar variables.
Siempre después de cada cambio ejecutar los tests. Pareceré un poco pesado diciendo esto a cada rato, pero un paso en falso, una equivocación puede tirarnos atrás todo el trabajo realizado por no gastar unos segundos en ejecutar un test.
A por el siguiente cambio, ese parece que indica el mínimo numero de jugadores:
function isPlayable() {
return ($this->howManyPlayers() >= 2);
}
Así que nuestra función quedará así:
function isPlayable()
{
$minimalNumberOfPlayer = 2;
return ($this->howManyPlayers() >= $minimalNumberOfPlayer);
}
Ahora vamos a por el cambio más difícil, en un primer momento dan ganas de extraer estas variables $roll % 2 != 0 pero ¡No! Esa guarda del if merece la pena que sea refactorizada a una función (esto lo más adelante, no hay que perder el foco). Lo que sí extraeremos son los literales 11 y 12. Pero para poder extraerlos necesitamos conocer que significan.
Pensando un poco y con la pista de que este juego se parece un poco al Trivial podemos inducir que 12 es el tamaño del tablero y 11 es la ultima posición del tablero, por eso podemos extraer estos 2 valores así:
$lastPositionOnTheBoard = 11;
$boardSize = 12;
Después de los tests hacemos un commit y a seguir adelante. Cada vez que extraigamos una variable, lo mejor es ponerla cerca del comienzo de la función, así podremos ver si hay más variables “parecidas”. Debemos de tener en cuenta que el código puede ser confuso y cuando empezamos a extraer nos salgan un montón de variables. Por ello debemos pararnos a pensar, por ejemplo si hay variables con el mismo valor estas pueden estar repetidas o no,…
function didPlayerWin()
{
$winningScore = 6;
return !($this->purses[$this->currentPlayer] == $winningScore);
}
No debemos preocuparnos aún por el código repetido, tendremos una tarea para eso más adelante. Nuestro foco es claro: magic constants y magic strings.
Conclusiones
Estamos leyendo código legacy y seguro que nos entran muchas ganas de cambiarlo, pero no debemos perder el foco. De la misma manera después de cada cambio debemos estar seguro de que la integridad e la aplicación no se ha visto afectada, así que hay que ejecutar los tests. Poco a poco haremos una aplicación PHP mantenible.
Jesús L.C. - Apuntes de un aprendiz 