Blog

El tema del código limpio está de moda, ¿no? Sí, pero también tiene sus fundamentos.

El código limpio es más comprensible y, por ende, mantenible en el tiempo.

Esto es especialmente importante cuando trabajas como parte de un equipo. Cuanto más claro sea el código, más fácil será para nuevos miembros hacerse productivos.

Todo el mundo está de acuerdo en esto pero lo cierto es que limpiar un código que lleva años acumulando capas de mugre puede ser una tarea titánica.

La respuesta más comúnmente usada ante estos escenarios es «¿para qué intentarlo cuando sé que no lo lograré?»

Cuando en realidad el escenario debería ser más parecido a:

Es decir: la opción de no limpiar no es realmente una opción… es cuestión de definir por dónde empezar.

Pero ahora, volviendo al código, hay algunas técnicas de limpieza más sencillas (¡y menos riesgosas!) que otras.

No es lo mismo cambiar el nombre de una variable que modificar la arquitectura.

Así que… vamos al punto. Mi enfoque para pasar de algo como esto:

<?php

declare(strict_types=1);

namespace GildedRose;

final class GildedRose
{
    /**
     * @param Item[] $items
     */
    public function __construct(
        private array $items
    ) {
    }

    public function updateQuality(): void
    {
        foreach ($this->items as $item) {
            if ($item->name != 'Aged Brie' and $item->name != 'Backstage passes to a TAFKAL80ETC concert') {
                if ($item->quality > 0) {
                    if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                        $item->quality = $item->quality - 1;
                    }
                }
            } else {
                if ($item->quality < 50) {
                    $item->quality = $item->quality + 1;
                    if ($item->name == 'Backstage passes to a TAFKAL80ETC concert') {
                        if ($item->sellIn < 11) {
                            if ($item->quality < 50) {
                                $item->quality = $item->quality + 1;
                            }
                        }
                        if ($item->sellIn < 6) {
                            if ($item->quality < 50) {
                                $item->quality = $item->quality + 1;
                            }
                        }
                    }
                }
            }

            if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                $item->sellIn = $item->sellIn - 1;
            }

            if ($item->sellIn < 0) {
                if ($item->name != 'Aged Brie') {
                    if ($item->name != 'Backstage passes to a TAFKAL80ETC concert') {
                        if ($item->quality > 0) {
                            if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                                $item->quality = $item->quality - 1;
                            }
                        }
                    } else {
                        $item->quality = $item->quality - $item->quality;
                    }
                } else {
                    if ($item->quality < 50) {
                        $item->quality = $item->quality + 1;
                    }
                }
            }
        }
    }
}

A algo como esto:

<?php

declare(strict_types=1);

namespace GildedRose;

final class GildedRose
{
    const BACKSTAGE_PASSES_TO_A_TAFKAL_80_ETC_CONCERT = 'Backstage passes to a TAFKAL80ETC concert';
    const SULFURAS_HAND_OF_RAGNAROS = 'Sulfuras, Hand of Ragnaros';
    const MAX_ITEM_QUALITY = 50;
    const FIRST_SELLIN_THRESHOLD = 11;
    const SECOND_SELLIN_THRESHOLD = 6;
    const AGED_BRIE = 'Aged Brie';

    /**
     * @param Item[] $items
     */
    public function __construct(
        private array $items
    )
    {
    }

    public function updateQuality(): void
    {
        foreach ($this->items as $item) {
            $this->updateItem($item);
        }
    }

    /**
     * @param Item $item
     * @return void
     */
    protected function updateItem(Item $item): void
    {
        if ($this->isSulfuras($item)) {

            exit;
        }

        $this->updateItemQuality($item);
        $this->updateItemSellIn($item);
    }

    /**
     * @param Item $item
     * @return void
     */
    protected function updateItemQuality(Item $item): void
    {
        if ($this->isBackStagePass($item)) {
            if ($this->isQualityBelowMaximum($item)) {
                $this->increaseQuality($item);
                if ($item->sellIn < self::FIRST_SELLIN_THRESHOLD && $this->isQualityBelowMaximum($item)) {
                    $this->increaseQuality($item);
                }
                if ($item->sellIn < self::SECOND_SELLIN_THRESHOLD && $this->isQualityBelowMaximum($item)) {
                    $this->increaseQuality($item);
                }
            }
        } elseif ($this->isAgedBrie($item)) {
            if ($this->isQualityBelowMaximum($item)) {
                $this->increaseQuality($item);
            }
        } else {
            if ($this->isQualityAboveMinimum($item)) {
                $this->reduceQuality($item);
            }
        }
    }

    /**
     * @param Item $item
     * @return void
     */
    protected function updateItemSellIn(Item $item): void
    {
        $this->decreaseSellIn($item);

        if ($this->isSellinExpired($item)) {
            if ($this->isAgedBrie($item)) {
                if ($this->isQualityBelowMaximum($item)) {
                    $this->increaseQuality($item);
                }
            } elseif ($this->isBackStagePass($item)) {
                $item->quality = 0;
            }
        } else {
            if ($this->isQualityAboveMinimum($item)) {
                $this->reduceQuality($item);
            }
        }
    }

    /**
     * @param Item $item
     * @return void
     */
    protected
    function reduceQuality(Item $item): void
    {
        $item->quality = $item->quality - 1;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isQualityAboveMinimum(Item $item): bool
    {
        return $item->quality > 0;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isQualityBelowMaximum(Item $item): bool
    {
        return $item->quality < self::MAX_ITEM_QUALITY;
    }

    /**
     * @param Item $item
     * @return void
     */
    protected
    function increaseQuality(Item $item): void
    {
        $item->quality = $item->quality + 1;
    }

    /**
     * @param Item $item
     * @return void
     */
    protected
    function decreaseSellIn(Item $item): void
    {
        $item->sellIn = $item->sellIn - 1;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isSellinExpired(Item $item): bool
    {
        return $item->sellIn < 0;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isSulfuras(Item $item): bool
    {
        return $item->name == self::SULFURAS_HAND_OF_RAGNAROS;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isAgedBrie(Item $item): bool
    {
        return $item->name == self::AGED_BRIE;
    }

    /**
     * @param Item $item
     * @return bool
     */
    protected
    function isBackStagePass(Item $item): bool
    {
        return $item->name == self::BACKSTAGE_PASSES_TO_A_TAFKAL_80_ETC_CONCERT;
    }
}

Consiste en ejecutar, casi mecánicamente, los siguientes pasos

1. Eliminar el hard-coding

Expresiones del tipo if ($item->quality < 50) { son bastante peligrosas. A priori surgen dos preguntas importantes:

1. ¿Por qué 50 es un número especial?
2. ¿Todos los 50 significan lo mismo?

En general, este tipo de números mágicos, tienen un sentido claro dentro del contexto de una aplicación. Probablemente cuando se escribió este código dicho contexto era claro pero, al no hacelo explícito, se hace muy complicado comprenderlo para alguien que lo ve por primera vez.

Una forma de evitar este problema es, simplemente, reemplazar este valor clavado por una constante de clase:

const MAX_ITEM_QUALITY = 50;

De esta forma se logra:

1. Dar significado a un número que a simple vista parece arbitrario
2. Permitir que si ese valor llega a cambiar en el futuro no sea necesario rastrear todos los lugares donde se usó el valor. Bastará con modificar la definición de la constante.

Lo mismo vale para valores tipo string como en el caso de $item->name != 'Backstage passes to a TAFKAL80ETC concert'

2. Extraer condicionales

El siguiente paso que suelo dar en este proceso es extrear las condiciones a métodos propios. Por ejemplo:

if ($item->name == 'Backstage passes to a TAFKAL80ETC concert') {

Se transformará en:

protected function isBackStagePass(Item $item): bool
{
return $item->name == self::BACKSTAGE_PASSES_TO_A_TAFKAL_80_ETC_CONCERT;
}

La idea es la misma. En lugar de, cada vez que se lea el código haya que concluir que el hecho de que name sea 'Backstage passes to a TAFKAL80ETC concert' significa que el ítem es un Backstage pass, dispongo de un método re-utilizable que me responde lo que realmente quiero saber.

Este ejemplo puede parecer trivial al comienzo, pero no lo es tanto.

¿Qué pasaría si, más adelante, la determinación del tipo de item viniese dada por alguna otra propiedad?

Este pequeño truco cobra todavía mayor importancia cuando las condiciones son complejas (cuando hay &&, || y/o muchos paréntesis)

3. Extraer cuerpo de loops

Del mismo modo, extraer los cuerpos de los bucles a métodos logra un resultado similar. Por un lado se reduce la complejidad del método y por otro se gana la posibilidad de re-utilizar la operación en diversos contextos.

En el ejemplo se hace muy visible el ciclo principal:

foreach ($this->items as $item) {
            if ($item->name != 'Aged Brie' and $item->name != 'Backstage passes to a TAFKAL80ETC concert') {
                if ($item->quality > 0) {
                    if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                        $item->quality = $item->quality - 1;
                    }
                }
            } else {
                if ($item->quality < 50) {
                    $item->quality = $item->quality + 1;
                    if ($item->name == 'Backstage passes to a TAFKAL80ETC concert') {
                        if ($item->sellIn < 11) {
                            if ($item->quality < 50) {
                                $item->quality = $item->quality + 1;
                            }
                        }
                        if ($item->sellIn < 6) {
                            if ($item->quality < 50) {
                                $item->quality = $item->quality + 1;
                            }
                        }
                    }
                }
            }

            if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                $item->sellIn = $item->sellIn - 1;
            }

            if ($item->sellIn < 0) {
                if ($item->name != 'Aged Brie') {
                    if ($item->name != 'Backstage passes to a TAFKAL80ETC concert') {
                        if ($item->quality > 0) {
                            if ($item->name != 'Sulfuras, Hand of Ragnaros') {
                                $item->quality = $item->quality - 1;
                            }
                        }
                    } else {
                        $item->quality = $item->quality - $item->quality;
                    }
                } else {
                    if ($item->quality < 50) {
                        $item->quality = $item->quality + 1;
                    }
                }
            }
        }

Que se reemplaza por:

public function updateQuality(): void
{
foreach ($this->items as $item) {
$this->updateItem($item);
}
}

4. Eliminar cláusulas else

Siempre que sea posible, deberías preferir prescindir las cláusulas else en tu código.

Una forma de lograrlo es utilizar early return.

Por ejemplo:

public function canSeeMovie(Person $person): bool
{
   if ($person->getAge() >= 18) {
      ...
   } else {
      return false;
   }
}

Bien podría ser reemplazado por:

public function canSeeMovie(Person $person): bool
{
   if ($person->getAge() < 18) {

      return false;
   }

   ...
}

Esto facilita mucho la lectura. Primero se ponen todas las validaciones o condiciones que podrían hacer que el método no pueda ejecutarse por completo y luego se codifica para el camino feliz.

Otro caso bastante parecido es el de usar valores por defecto:

public function getMaxSpeed(Car $car): int
{
   if (in_array($car->getName(), ["Ferrari", "Porsche"])) {
      $maxSpeed = 300;
   } else {
      $maxSpeed = 200;
   }

   return $maxSpeed;
}

Se transformaría en:

public function getMaxSpeed(Car $car): int
{
   $maxSpeed = 200;

   if (in_array($car->getName(), ["Ferrari", "Porsche"])) {
      $maxSpeed = 300;
   }

   return $maxSpeed;
}

5. Eliminar variables temporales

Las variables temporales suelen crearse para aclarar cosas pero muchas veces su efecto es precisamente el contrario:

public function showMovieTo(Movie $movie, Person $person): void 
{
   $canSeeTheMovie = $this->canSeeMovie($person);

   if ($canSeeTheMovie) {
       $this->playMovie($movie);
   }
}

En este pequeño ejemplo vemos que al llegar a la línea if ($canSeeTheMovie) { se necesita volver hacia atrás buscando la última asignación realizada a la variable $canSeeTheMovie para determinar qué hará este método.

Mucho más fácil de comprender es:

public function showMovieTo(Movie $movie, Person $person): void 
{
   if ($this->canSeeMovie($person)) {
       $this->playMovie($movie);
   }
}

Esta técnica no siempre es aplicable. En todo caso, lo segundo mejor por hacer es traer la definición y/o asignación de la variable lo más cerca de su uso que se pueda.

6. Renombrar, renombrar y renombrar

Este es un paso algo más sutil pero no menos importante.

El código que escribimos debe ser legible por humanos… muy probablemente por nosotros mismos en un futuro no muy lejano, de modo que es casi un deber moral escribirlo de un modo legible.

Encontrar buenos nombres para los elementos de nuestro código es una de las partes más complejas de programar, principalmente porque no hay recetas.

Algunos lineamientos que te puedo dar:

1. Escribir todo el código en inglés (No usar Spanglish)
2. No usar abreviaturas (Preferir $unusedTableNames a $utNm)
3. Poner nombres que revelen intención, no implementación (Prefereir $emptyAccounts sobre $emptyAccountsArray)
4. En el caso de clases o funciones, poner nombres que coincidan con el código.

Este último es, probablemente el punto más delicado. Durante la vida de un proyecto de software, es muy probable que un elemento cambie su definición conforme va pasando el tiempo.

Cuando eso sucede, es decir, cuando te toca modificar el código del cuerpo de un método (O de una clase en su totalidad), vale la pena preguntarte si el nombre que tenía antes de tu intervención sigue reflejando lo que el método hace (o lo que aporta mejor dicho) y, en caso de no ser así, es una buena idea darle un nuevo nombre más ligado a la realidad.

Y ahora… ¿qué?

Casi todos los cambios que nombré en este post se pueden realizar de forma bastante simple y segura utilizando un IDE, más adelante tocará encarar el refactor propiamente dicho:

1. Agregar tipado
2. Re-distribuir responsabilidades
3. Aplicar patrones de diseño

Pero… antes de intentar avanzar por este camino hay que asegurarse de contar con buena covertura de tests.

Si te interesa este tema te recomiendo leer el libro de Código Limpio de Robert C. Martin.

Por último te animo a que practiques tus habilidades de refactoring con la kata Gilded Rose, de la que saqué el código que usé para el ejemplo principal.