Code smells: método largo, clase grande, duplicación, primitivos en vez de tipos, cadenas largas de if

Código en:JavaC#PythonJavaScript

En resumen: Un code smell es una señal de que el próximo cambio producirá un error: un método largo, una clase grande, duplicación, primitivos en vez de tipos y una cadena larga de if.

Problema

La caja dice 90 €, la factura dice 95 €

El total del pedido se calcula en dos sitios: en la caja y en la factura. Hace un mes el descuento VIP pasó a 10 %. El desarrollador corrigió la caja:

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Caja: ${checkoutTotal(100, true).toFixed(2)} €`);
console.log(`Factura: ${invoiceTotal(100, true).toFixed(2)} €`);

Salida

Caja: 90.00 €
Factura: 95.00 €

El cliente paga 90 € y la factura dice 95 €. Contabilidad encuentra una diferencia de 5 € en cada pedido VIP del último mes. Ninguna línea de aquí es «incorrecta» por sí sola: invoiceTotal hace exactamente lo que se escribió para hacer. El problema es que una regla vive en dos sitios.

Míralo

Cómo se separan dos copias

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Caja: ${checkoutTotal(100, true).toFixed(2)} €`);
console.log(`Factura: ${invoiceTotal(100, true).toFixed(2)} €`);

Paso a paso, en palabras

1. invoiceTotal nació como un copy-paste de checkoutTotal: las mismas tres líneas, las dos con 5 % de descuento VIP.
2. El descuento VIP pasa a 10 %. El error se reportó en la caja, así que la corrección entra en checkoutTotal, línea 2.
3. Nadie recuerda que la factura es una copia. La línea 8 se queda con 0.05: las dos copias ahora dicen cosas distintas.
4. Un pedido de 100 € de un cliente VIP pasa por la caja: 100 − 10 + 0 = 90 €.
5. El mismo pedido entra en la factura: 100 − 5 + 0 = 95 €.
6. El cliente paga 90 € y la factura dice 95 €. Una regla en dos sitios: corriges una y la otra se queda atrás.

El día del copy-paste las dos funciones estaban bien. El error nació después, con una corrección hecha con buena fe, pero en un solo sitio. El código que hoy está bien pero causará un error con el próximo cambio tiene un code smell (literalmente, «mal olor del código»). El término viene de Kent Beck y Martin Fowler: un smell no es un error, sino una señal de que algo cerca de él pronto saldrá mal.

Esta lección trata de los cinco smells más comunes y de cómo notarlos antes de que te cuesten 5 € por pedido.

La idea

Duplicación: una regla, un lugar

La regla del total pasa a ser una función, y la caja y la factura la llaman las dos:

function orderTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Caja: ${orderTotal(100, true).toFixed(2)} €`);
console.log(`Factura: ${orderTotal(100, true).toFixed(2)} €`);

Salida

Caja: 90.00 €
Factura: 90.00 €

Ahora no puedes corregir una y olvidar la otra: no hay otra.

Cómo lo hueles. Corriges un error y te preguntas: «¿Esto se copió en algún sitio?». Buscas un número o una expresión llamativos (0.05) y los encuentras en dos sitios. Dos funciones con nombres casi iguales (checkout…, invoice…) y cuerpos casi iguales.

Cuidado. No todo lo que se ve igual es duplicación. El envío gratis desde 50 € y las devoluciones en 50 días son dos reglas con un número. Únelas en una constante, y cambiar una cambia la otra. La duplicación es una regla en dos sitios. Muchos equipos esperan a la tercera copia antes de extraer la parte común: dos pueden ser casualidad, tres rara vez.

Míralo

Método largo

La función recibe un pedido y lo hace todo:

function placeOrder(email, subtotal, city) {
  // comprobación
  if (!email.includes("@")) {
    console.log("Correo no válido: " + email);
    return;
  }
  if (subtotal <= 0) {
    console.log("Carrito vacío");
    return;
  }
  // total
  let delivery = city === "Sofía" ? 3.9 : 5.9;
  if (subtotal >= 50) {
    delivery = 0;
  }
  const total = subtotal + delivery;
  // aviso
  const text = `Tu pedido por ${total.toFixed(2)} € ha sido aceptado.`;
  console.log("Para " + email + ": " + text);
  // log
  console.log(`[log] pedido ${email} ${total.toFixed(2)}`);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Salida

Para ana@example.com: Tu pedido por 39.90 € ha sido aceptado.
[log] pedido ana@example.com 39.90
Correo no válido: ana.example.com

Cómo lo hueles: haces scroll para ver el final; los comentarios funcionan como títulos de partes («comprobación», «total»); no puedes decir qué hace sin decir «y»; una variable declarada arriba solo se usa abajo del todo. Cada comentario-título es el nombre de una función que está por nacer (mira la lección Comentarios que mienten).

Cada parte pasa a su propia función, con el nombre de su comentario:

function problemWith(email, subtotal) {
  if (!email.includes("@")) {
    return "Correo no válido: " + email;
  }
  if (subtotal <= 0) {
    return "Carrito vacío";
  }
  return null;
}

function deliveryFee(subtotal, city) {
  if (subtotal >= 50) {
    return 0;
  }
  return city === "Sofía" ? 3.9 : 5.9;
}

function notifyCustomer(email, total) {
  const text = `Tu pedido por ${total.toFixed(2)} € ha sido aceptado.`;
  console.log("Para " + email + ": " + text);
}

function log(email, total) {
  console.log(`[log] pedido ${email} ${total.toFixed(2)}`);
}

function placeOrder(email, subtotal, city) {
  const problem = problemWith(email, subtotal);
  if (problem !== null) {
    console.log(problem);
    return;
  }
  const total = subtotal + deliveryFee(subtotal, city);
  notifyCustomer(email, total);
  log(email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Salida

Para ana@example.com: Tu pedido por 39.90 € ha sido aceptado.
[log] pedido ana@example.com 39.90
Correo no válido: ana.example.com

La salida es la misma, letra por letra. La función principal ahora se lee como una lista de pasos, y la tarifa de envío se puede probar por separado. Este paso se llama Extract Method, y lo harás en pasos pequeños en la lección sobre refactorización.

Míralo

Clase grande

El pedido ha acumulado todo lo que alguien alguna vez necesitó:

class Order {
  customerName = "";
  customerEmail = "";
  customerPhone = "";
  deliveryCity = "";
  deliveryStreet = "";
  deliveryPostcode = "";
  cardNumber = "";
  cardExpiry = "";
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.deliveryCity = "Varna";
console.log("Envío a " + order.deliveryCity);

Salida

Envío a Varna

Cómo lo hueles: grupos de campos con un prefijo común (customer…, delivery…, card…); métodos que usan un solo grupo; un nombre como OrderManager o Utils que no dice nada; cada tarea del equipo toca este archivo, y los conflictos de merge en git siempre están en él.

Cada grupo con prefijo común es un tipo que espera su nombre:

class Order {
  customer = null;
  delivery = null;
  card = null;
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.delivery = { city: "Varna", street: "Calle del Mar 5", postcode: "9000" };
console.log("Envío a " + order.delivery.city);

Salida

Envío a Varna

En JavaScript un grupo suele ser un objeto simple { city, street, postcode }. Una clase vale la pena cuando el grupo tiene reglas propias, como el teléfono más abajo.

El resto (smsSent, emailSent) es el siguiente grupo. No tienes que arreglarlo todo de una vez.

Pruébalo

Predice la salida

Tres productos a 0,10 € cada uno. ¿Qué imprime el programa?

let total = 0;
for (let i = 0; i < 3; i++) {
  total += 0.1;
}
console.log(total === 0.3);
console.log(total);

Respuesta

false / 0.30000000000000004

true / 0.30

true / 0.3

Comprobar

0.10 no tiene una forma binaria exacta, igual que 1/3 no tiene una forma decimal exacta. Un double guarda el número binario más cercano, y los pequeños errores se acumulan.

Míralo

Primitivos en vez de tipos: el dinero

El dinero en un double es la forma más común del smell obsesión por los primitivos (primitive obsession): un valor con reglas propias vive en un tipo que no conoce esas reglas. Hasta ahora, en el curso los importes eran number para abreviar. En código real:

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += 10;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Salida

true
0.30

En JavaScript todo número es un double, y no hay un tipo decimal. Por eso el dinero se guarda en céntimos enteros: 10 + 10 + 10 es exactamente 30. Se convierte en euros solo al imprimir.

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += ;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Comprobar

Más precisamente, para curiosos

Aún mejor es un tipo Money que guarde juntos el importe y la moneda y no te deje sumar euros a dólares por error. Es la misma idea que el teléfono de la siguiente tarjeta.

Míralo

Primitivos en vez de tipos: el teléfono

Un número de teléfono como cadena es otra forma del mismo smell. «0888 123 456», «+359 888 123 456» y «+359888123456» son un número, pero tres cadenas distintas. Cada lugar que compara teléfonos tiene que recordar por su cuenta cómo limpiarlos. El tipo Phone limpia y comprueba una vez, cuando se crea:

class Phone {
  #value;

  constructor(raw) {
    let digits = raw.replace(/[ ()-]/g, "");
    if (digits.startsWith("0")) {
      digits = "+359" + digits.slice(1);
    }
    if (!/^\+359\d{9}$/.test(digits)) {
      throw new Error("Teléfono no válido: " + raw);
    }
    this.#value = digits;
  }

  get value() {
    return this.#value;
  }

  equals(other) {
    return other instanceof Phone && other.#value === this.#value;
  }
}

const a = new Phone("0888 123 456");
const b = new Phone("+359 888 123 456");
console.log(a.value);
console.log(a.equals(b) ? "mismo número" : "números distintos");
try {
  new Phone("0888 12");
} catch (e) {
  console.log(e.message);
}

Salida

+359888123456
mismo número
Teléfono no válido: 0888 12

La expresión regular [ ()-] quita espacios, paréntesis y guiones, y \+359\d{9} comprueba que después de +359 haya exactamente nueve dígitos.

Un teléfono no válido no puede existir: el constructor no lo deja pasar. Cada Phone del programa ya está limpio.

En JavaScript === entre dos objetos comprueba si son el mismo objeto, no si son iguales. Por eso Phone tiene su propio equals, y #value es un campo privado que nadie desde fuera puede cambiar.

Pon las líneas del constructor en orden para que "0888 123 456" se convierta en "+359888123456":

1. this.#value = digits;↑↓
2. digits = "+359" + digits.slice(1);↑↓
3. if (digits.startsWith("0")) {↑↓
4. if (!/^\+359\d{9}$/.test(digits)) {↑↓
5. let digits = raw.replace(/[ ()-]/g, "");↑↓
6. }↑↓
7. }↑↓
8. throw new Error("Teléfono no válido: " + raw);↑↓

Comprobar

Pruébalo

Encuentra el error

El cliente se dio de baja de los mensajes SMS, y la tienda le sigue escribiendo por cada paquete:

const optedOut = ["+359888123456"];
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

Salida

SMS a 0888 123 456: Tu pedido está en camino.
SMS a 0877 555 010: Tu pedido está en camino.

¿En qué línea está el error?

Elige la línea con el error.

1function notifyShipped(phone, optedOut) {2 if (optedOut.includes(phone)) {3 return;4 }5 console.log("SMS a " + phone + ": Tu pedido está en camino.");6}

Comprobar

Míralo

Cadena larga de if sobre un tipo

El precio y el plazo de entrega dependen del tipo de envío. La cadena está en las dos funciones:

function price(kind) {
  if (kind === "mensajero") {
    return 5.9;
  } else if (kind === "oficina") {
    return 3.9;
  } else if (kind === "locker") {
    return 2.9;
  }
  throw new Error("Tipo de envío desconocido: " + kind);
}

function days(kind) {
  if (kind === "mensajero") {
    return 1;
  } else if (kind === "oficina") {
    return 2;
  } else if (kind === "locker") {
    return 3;
  }
  throw new Error("Tipo de envío desconocido: " + kind);
}

for (const kind of ["mensajero", "oficina", "locker"]) {
  console.log(`${kind}: ${price(kind).toFixed(2)} €, días: ${days(kind)}`);
}

Salida

mensajero: 5.90 €, días: 1
oficina: 3.90 €, días: 2
locker: 2.90 €, días: 3

Cómo lo hueles: la misma cadena sobre el mismo campo (kind, type, status) en varios sitios; un tipo nuevo significa encontrar cada cadena; una rama olvidada aparece solo cuando un cliente la elige.

Aquí los tipos se distinguen solo por números. Entonces la cadena pasa a ser una tabla: tipo → datos:

const DELIVERY = new Map([
  ["mensajero", { price: 5.9, days: 1 }],
  ["oficina", { price: 3.9, days: 2 }],
  ["locker", { price: 2.9, days: 3 }],
]);

for (const kind of ["mensajero", "oficina", "locker"]) {
  const d = DELIVERY.get(kind);
  console.log(`${kind}: ${d.price.toFixed(2)} €, días: ${d.days}`);
}

Salida

mensajero: 5.90 €, días: 1
oficina: 3.90 €, días: 2
locker: 2.90 €, días: 3

Un tipo nuevo es una fila nueva en la tabla. Cuando los tipos se distinguen por comportamiento, no solo por números, cada tipo pasa a ser su propia clase o función: eso es Strategy. Si el tipo cambia con el tiempo (el pedido es nuevo, luego pagado, luego enviado), eso es State. Cómo una cadena se convierte en clases paso a paso (Replace Conditional with Polymorphism) está en la lección sobre refactorización.

La idea

Un smell no es un error: cuándo dejarlo

Los cinco smells en breve:

• duplicación: una regla en varios sitios → una función o constante;
• método largo: haces scroll y lees comentarios-título → Extract Method;
• clase grande: grupos de campos con prefijo común → una clase para cada grupo;
• primitivos en vez de tipos: dinero en un double, un teléfono en una cadena → un tipo que guarda sus propias reglas;
• cadena larga de if sobre un tipo: la misma cadena en muchos sitios → una tabla, Strategy o State.

Un smell no es un error. Te dice cuánto costará el próximo cambio. Así que lo dejas cuando:

• el código no cambia. El informe que nadie ha tocado en tres años no cuesta nada hasta que lo tocas;
• no hay tests. Sin ellos, arreglar el smell puede romper algo que funcionaba. Primero los tests (la próxima lección), luego los pasos pequeños;
• dos cosas solo se parecen. Dos reglas con un número no son duplicación;
• el código es de un solo uso: un script de migración que borrarás mañana.

La regla del explorador: cuando de todos modos estás tocando un archivo, déjalo un poco más limpio de como lo encontraste. No reescribas módulos enteros «por si acaso».

Ejercicio

Huélelo y arréglalo

Resuélvelo en tu propio editor.

El recibo. La función envía un recibo por SMS, correo o Viber:

function sendReceipt(channel, contact, amount) {
  if (channel === "sms") {
    const text = `Pagaste ${amount.toFixed(2)} €. ¡Gracias!`;
    console.log("SMS a " + contact.replaceAll(" ", "") + ": " + text);
  } else if (channel === "email") {
    const text = `Pagaste ${amount.toFixed(2)} €. ¡Gracias!`;
    console.log("Correo a " + contact.trim().toLowerCase() + ": " + text);
  } else if (channel === "viber") {
    const text = `Pagaste ${amount.toFixed(2)} €. ¡Gracias!`;
    console.log("Viber a " + contact.replaceAll(" ", "") + ": " + text);
  }
}

sendReceipt("sms", "0888 123 456", 24.5);
sendReceipt("email", " Ana@Example.com ", 24.5);
sendReceipt("viber", "0877 555 010", 9.9);

Salida

SMS a 0888123456: Pagaste 24.50 €. ¡Gracias!
Correo a ana@example.com: Pagaste 24.50 €. ¡Gracias!
Viber a 0877555010: Pagaste 9.90 €. ¡Gracias!

Encuentra los smells que tiene (al menos tres) y ponles nombre. Luego arréglalos uno por uno, ejecutando el programa después de cada paso: la salida debe quedar igual, letra por letra.

Más tareas

• Un canal nuevo. Añade Telegram, con un contacto como el del SMS. ¿En cuántos sitios haces cambios antes de la corrección, y en cuántos después?
• Dinero en céntimos. Cambia el importe del recibo a céntimos enteros (2450), y conviértelo en euros solo al imprimir.
• La clase grande. Termina Order de la tarjeta «Clase grande»: mueve smsSent y emailSent a su propio tipo y piensa cómo llamarlo.

Test

Ponte a prueba

1. El código tiene un smell. ¿Significa que tiene un error?

No. El código puede funcionar hoy; el smell dice que el próximo cambio es arriesgadoSí, un smell es un error con otro nombreSí, si el compilador da una advertencia

2. El envío gratis empieza en 50 € y las devoluciones se admiten hasta 50 días. ¿Deberías hacer de ellos una sola constante para evitar la duplicación?

No. Son dos reglas distintas que casualmente tienen el mismo númeroSí, pero llámala FIFTYSí, un número debe vivir en un solo lugar

3. Los totales se suman en un double, y 0.10 tres veces no es 0.30. ¿Qué ayuda?

Redondear cada vez que imprimesCéntimos enteros en vez de eurosUn tipo más grande: float en vez de double

4. La misma cadena de if sobre el tipo de envío está en el precio, en el plazo y en la etiqueta. Llega un tipo nuevo. ¿Qué ayuda?

Una tabla tipo → datos, o una clase por tipo si el comportamiento es distintoUn comentario sobre cada cadena que enumere los tiposUn else if más en los tres sitios

5. El módulo del informe anual: 300 líneas en un método, nadie lo ha tocado en tres años, sin tests. Mañana tienes que entregar otra cosa. ¿Qué haces?

Lo divido en funciones hoyLo borroLo dejo. Lo arreglaré cuando tenga que cambiarlo, y antes escribiré tests

Comprobar

Empezar la lección de nuevo

Lecciones relacionadas

• Se basa en: State · Nombres en el código · Funciones: una tarea, pocos parámetros, sin efectos secundarios ocultos · Comentarios que mienten y comentarios que ayudan
• Continúa en: Las pruebas como red de seguridad · Refactorización en pasos pequeños · SOLID sin dogma · Ejercicio: una clase desordenada, limpiada paso a paso
• Ejercicios sobre el tema: Código limpio y refactorización