No siempre hay que implementar las buenas prácticas

En mi anterior trabajo, una vez se me ocurrió borrar un comentario del código y luego hice una pull request de todo el trabajo hecho, el comentario que borre era tipo esto:

// esto ejecuta la compra.
public function executePurchase(Order $order): void

Lo borré porque creí que el nombre de la función era lo bastante descriptivo y explicito de lo que hacía. Pero uno de los programadores más antiguos del departamento me abrió un thread cuestionando ese borrado. Yo era nuevo, así que ni lo discutí y lo restablecí. Ahora me arrepiento.

¿Qué sentido tiene mantener ese comentario? Ninguno. No explica ningún porqué y no aporta contexto. Yo creo que realmente ni se lo llegó a leer, simplemente en su cabeza existía la regla de: NO borrar comentarios, y ni se paró a pensar si este caso lo merecía o no.

Y las buenas prácticas funcionan igual. Quiero decir, vienen de problemas reales, constrastados y solucionados, pero que aplicadas por decreto y sin contexto, pueden llegar a ser una castaña.

Vamos a ver unos cuantos casos donde seguir la regla a rajatabla sale más caro que saltársela.

El código se explica solo (hasta que no)

Antes he explicado un ejemplo de comentarios inútiles que no aportan nada, esto es lo contrario. No escribir un comentario jamás porque el código bien escrito se documenta solo. Yo soy el primero que intenta documentar eligiendo bien los nombres y muchas veces puede no ser suficiente:

if ($transaction->getAmount() >= 999
    && $transaction->getStatus() === TransactionStatus::TAGGED) {
    $transaction->setStatus(TransactionStatus::REJECTED);
}

(Me he inventado la condición, no le busques el sentido.)

Miramos el código y pensamos: ¿Por qué 999? ¿Por qué TAGGED? El que escribió esto lo sabía. Se fue de la empresa y solo el sabía porque se hizo esto.

La próxima persona que llegue aquí se va a preguntar si esto hace falta y nadie va a saber contestarle, alguien lo borrará y resultará que sí hacía falta. Un comentario de una línea con el porqué y todos sabemos el porque de las cosas y no hay que ir yendo a preguntar a nadie.

El comentario tipo // esto ejecuta la compra. se va al carrer y el // límite exigido por el banco desde el incidente de marzo se queda porque aporta valor.

Y no, la solución tampoco es meter el porqué en el nombre y acabar con rejectTaggedTransactionsOverBankImposedLimit(). Un nombre es un nombre, no una frase.

DRY hasta hacerse daño

Dos trozos de código que se parecen no siempre son el mismo trozo de código. Imagina dos endpoints que validan email y password:

// Endpoint A: crear usuario normal
validateEmail($email);
validatePasswordStrength($password);

// Endpoint B: crear usuario admin
validateEmail($email);
validatePasswordStrength($password);

Al primero que pase por ahí le salta la alarma del DRY y lo saca todo a un único y precioso validateUserCredentials($email, $password).

Dos semanas después los admin y solo los admin necesitan una contraseña más larga. Le metes un flag. Un mes después también validan el dominio del email: otro flag. Y los usuarios externos no pueden llevar símbolos raros, otro más. Al final tienes un monstrenco de cinco booleanos que ninguno de los dos endpoints quiere tocar porque si lo cambio aquí, rompo el otro, horror.

Copia y pega primero y abstrae cuando el patrón esté probado.

Cobertura no es confianza

Tenemos un 100% de cobertura suena bien. Hasta que alguien cambia un comportamiento de verdad y se rompen quince tests de golpe. No porque el comportamiento nuevo esté mal, sino porque los tests estaban acoplados a la implementación:

// Test acoplado a la implementación (frágil)
public function test_calculateTotal_callsTaxServiceWithCorrectArgs(): void
{
    $taxService = $this->createMock(TaxService::class);
    $taxService->expects($this->once())
        ->method('calculate')
        ->with(100, 'ES');
    // ...
}

// Test acoplado al comportamiento (robusto)
public function test_calculateTotal_appliesSpanishVAT(): void
{
    $result = $this->calculator->calculateTotal(100, 'ES');
    $this->assertEquals(121, $result);
}

El primero se rompe el día que cambias TaxService por TaxCalculator. El segundo aguanta cualquier refactor mientras el resultado sea correcto. Si un test se rompe cada vez que tocas el interior de una función sin cambiar lo que hace, ese test no te está protegiendo de nada.

Capas y abstracciones para un futuro que no llega

Controllers llaman a Services, Services a Repositories, Repositories a Models. Para una aplicación grande tiene todo el sentido del mundo. Para la herramienta interna de admin con cinco funcionalidades, igual no:

UserController
  → UserService
    → UserRepository
      → UserModel

Cuatro ficheros que tocar por cada campo que cambies, veinte clases en total, para cinco features. Una función updateUserStatus($userId, $status) que tira directa a base de datos hace lo mismo, se lee más fácil y vive en un solo sitio. Si mañana la herramienta crece considerablemente, el Service lo metes ese mismo día.

Algo parecido a esto es diseñar extensible para requisitos imaginados, como la arquitectura de plugins que nunca tuvo un segundo plugin, la clase abstracta con una única hija, la interface con una sola implementación...

// El monstruo "extensible" que nunca tuvo segundo caso de uso
interface PaymentProcessor {
    public function process(Payment $p): Result;
}

class StripePaymentProcessor implements PaymentProcessor { /* única implementación */ }

// Lo que hacía falta el día 1:
function processStripePayment(Payment $p): Result {
    // ... 20 líneas y listo
}

Más adelante casi nunca llega y cuando llega, los requisitos nunca son los que te habías imaginado, así que la abstracción que montaste no vale y te toca rehacerla igual. La abstracción correcta aparece cuando tienes dos casos reales delante, así que hasta entonces es mejor no sobredimensionar las cosas.

Nota

La arquitectura de empresa es para problemas de empresa. Una herramienta interna de cinco botones no es un problema de empresa.

Excepciones como flujo de control

Las excepciones son para casos excepcionales. Todos estamos de acuerdo, ah! pero luego...

public function getUser(int $userId): User
{
    $user = $this->db->find($userId);
    if (!$user) {
        throw new UserNotFoundException("User not found");
    }
    return $user;
}

// Y en el caller
try {
    $user = $this->getUser(123);
    $this->process($user);
} catch (UserNotFoundException $e) {
    return response('Not found', 404);
}

Que un usuario no exista no es que algo haya ido mal, es un resultado completamente normal de buscar cuando hiciste una query y no había nada.

// Más limpio
public function getUser(int $userId): ?User
{
    return $this->db->find($userId);
}

// Caller
$user = $this->getUser(123);
if ($user === null) {
    return response('Not found', 404);
}
$this->process($user);

Devuelve null, devuelve un tipo Result o lo que sea. Las excepciones resérvalas para lo que de verdad no debería pasar nunca, que por algo se llaman así.

Cuarenta constantes con nombre

La regla de los números mágicos existe por esto:

if ($statusCode === 7) { // ¿qué es 7?
    // ...
}

Un significado misterioso que nadie va a adivinar, hasta ahí, estamos de acuerdo. El problema es el otro extremo:

const int MINIMUM_PASSWORD_LENGTH = 8;
const int MAX_LOGIN_ATTEMPTS = 5;
const int SESSION_TIMEOUT_MINUTES = 30;
const int DEFAULT_PAGE_SIZE = 20;
const int ANIMATION_DURATION_MS = 300;
const int BORDER_RADIUS_PX = 4;

Cuarenta constantes para valores obvios que no cambian nunca, y cada vez que tocas algo te toca ir al fichero de constantes a mirar qué mierdas vale cada cosa. retry(3) no es un número mágico: es un contador y el contexto lo deja clarísimo. Guarda los nombres de constante para los valores que nadie adivinaría.

Refactorizar ya que paso por aquí

Te toca arreglar un bug y al lado hay una función que te hace daño a la vista y como ya estás ahí, la refactorizas. El fix se va a la rama junto con un refactor que nadie ha pedido.

Dos días después aparece otro bug. ¿Es del fix? ¿Del refactor? ¿De los dos? Acabas de duplicar las posibilidades de petada con toda la buena intención del mundo.

La Regla del Boy Scout que dice: deja el código mejor de como lo encontraste, está bien como filosofía general. En una rama de release con clientes esperando, deja la función tranquilita y abre un ticket para refactorizarlo más adelante.

---

Bueno concluyendo, que las reglas están muy bien hasta que dejan de estarlo, y para saber cuándo es eso no hay libro, hace falta criterio y años de experiencia.

El comentario de la compra que comenté al principio del post, por cierto, que yo sepa sigue ahí. // esto ejecuta la compra., justo encima de executePurchase.

Y ahí seguirá.

EA! en los bares nos beeremos!