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:

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Caja: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Factura: %.2f €%n", invoiceTotal(100, true));

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

static double checkoutTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

static double invoiceTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.05 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Caja: %.2f €%n", checkoutTotal(100, true));
System.out.printf("Factura: %.2f €%n", invoiceTotal(100, true));

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:

static double orderTotal(double subtotal, boolean vip) {
    double discount = vip ? subtotal * 0.10 : 0;
    double delivery = subtotal >= 50 ? 0 : 4.90;
    return subtotal - discount + delivery;
}

System.out.printf("Caja: %.2f €%n", orderTotal(100, true));
System.out.printf("Factura: %.2f €%n", orderTotal(100, true));

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

El método recibe un pedido y lo hace todo:

static void placeOrder(String email, double subtotal, String city) {
    // comprobación
    if (!email.contains("@")) {
        System.out.println("Correo no válido: " + email);
        return;
    }
    if (subtotal <= 0) {
        System.out.println("Carrito vacío");
        return;
    }
    // total
    double delivery = city.equals("Sofía") ? 3.90 : 5.90;
    if (subtotal >= 50) {
        delivery = 0;
    }
    double total = subtotal + delivery;
    // aviso
    String text = String.format("Tu pedido por %.2f € ha sido aceptado.", total);
    System.out.println("Para " + email + ": " + text);
    // log
    System.out.printf("[log] pedido %s %.2f%n", 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

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:

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

static double deliveryFee(double subtotal, String city) {
    if (subtotal >= 50) {
        return 0;
    }
    return city.equals("Sofía") ? 3.90 : 5.90;
}

static void notifyCustomer(String email, double total) {
    String text = String.format("Tu pedido por %.2f € ha sido aceptado.", total);
    System.out.println("Para " + email + ": " + text);
}

static void log(String email, double total) {
    System.out.printf("[log] pedido %s %.2f%n", email, total);
}

static void placeOrder(String email, double subtotal, String city) {
    String problem = problemWith(email, subtotal);
    if (problem != null) {
        System.out.println(problem);
        return;
    }
    double 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. El método 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 {
    String customerName;
    String customerEmail;
    String customerPhone;
    String deliveryCity;
    String deliveryStreet;
    String deliveryPostcode;
    String cardNumber;
    String cardExpiry;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.deliveryCity = "Varna";
System.out.println("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:

record Customer(String name, String email, String phone) {}

record Address(String city, String street, String postcode) {}

record Card(String number, String expiry) {}

class Order {
    Customer customer;
    Address delivery;
    Card card;
    double subtotal;
    double discount;
    boolean smsSent;
    boolean emailSent;
}

Order order = new Order();
order.delivery = new Address("Varna", "Calle del Mar 5", "9000");
System.out.println("Envío a " + order.delivery.city());

Salida

Envío a Varna

Un record escribe los campos, el constructor y los accesores. La dirección ahora es un valor: se pasa, se compara y se comprueba en un solo lugar.

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?

double total = 0;
for (int i = 0; i < 3; i++) {
    total += 0.10;
}
System.out.println(total == 0.30);
System.out.println(total);

Respuesta

true / 0.3

false / 0.30000000000000004

true / 0.30

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 double para abreviar. En código real:

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal("0.10"));
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

Salida

true
0.30

BigDecimal guarda los decimales exactamente. Se crea a partir de una cadena: new BigDecimal(0.10) toma el double ya incorrecto y conserva el error. Se compara con compareTo, porque equals trata 0.30 y 0.3 como distintos.

BigDecimal total = BigDecimal.ZERO;
for (int i = 0; i < 3; i++) {
    total = total.add(new BigDecimal());
}
System.out.println(total.compareTo(new BigDecimal("0.30")) == 0);
System.out.println(total);

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:

record Phone(String value) {
    Phone {
        String digits = value.replaceAll("[ ()-]", "");
        if (digits.startsWith("0")) {
            digits = "+359" + digits.substring(1);
        }
        if (!digits.matches("\\+359\\d{9}")) {
            throw new IllegalArgumentException("Teléfono no válido: " + value);
        }
        value = digits;
    }
}

Phone a = new Phone("0888 123 456");
Phone b = new Phone("+359 888 123 456");
System.out.println(a.value());
System.out.println(a.equals(b) ? "mismo número" : "números distintos");
try {
    new Phone("0888 12");
} catch (IllegalArgumentException e) {
    System.out.println(e.getMessage());
}

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.

El constructor compacto de un record recibe value antes de guardarlo y puede cambiarlo. equals viene del record y compara los valores limpios.

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

1. }↑↓
2. String digits = value.replaceAll("[ ()-]", "");↑↓
3. throw new IllegalArgumentException("Teléfono no válido: " + value);↑↓
4. if (!digits.matches("\\+359\\d{9}")) {↑↓
5. if (digits.startsWith("0")) {↑↓
6. value = digits;↑↓
7. }↑↓
8. digits = "+359" + digits.substring(1);↑↓

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:

List<String> optedOut = List.of("+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.

1static void notifyShipped(String phone, List<String> optedOut) {2 if (optedOut.contains(phone)) {3 return;4 }5 System.out.println("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:

static double price(String kind) {
    if (kind.equals("mensajero")) {
        return 5.90;
    } else if (kind.equals("oficina")) {
        return 3.90;
    } else if (kind.equals("locker")) {
        return 2.90;
    }
    throw new IllegalArgumentException("Tipo de envío desconocido: " + kind);
}

static int days(String kind) {
    if (kind.equals("mensajero")) {
        return 1;
    } else if (kind.equals("oficina")) {
        return 2;
    } else if (kind.equals("locker")) {
        return 3;
    }
    throw new IllegalArgumentException("Tipo de envío desconocido: " + kind);
}

for (String kind : List.of("mensajero", "oficina", "locker")) {
    System.out.printf("%s: %.2f €, días: %d%n", kind, price(kind), 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:

record Delivery(double price, int days) {}

static final Map<String, Delivery> DELIVERY = Map.of(
    "mensajero", new Delivery(5.90, 1),
    "oficina", new Delivery(3.90, 2),
    "locker", new Delivery(2.90, 3));

for (String kind : List.of("mensajero", "oficina", "locker")) {
    Delivery d = DELIVERY.get(kind);
    System.out.printf("%s: %.2f €, días: %d%n", kind, d.price(), 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: 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:

static void sendReceipt(String channel, String contact, double amount) {
    if (channel.equals("sms")) {
        String text = String.format("Pagaste %.2f €. ¡Gracias!", amount);
        System.out.println("SMS a " + contact.replace(" ", "") + ": " + text);
    } else if (channel.equals("email")) {
        String text = String.format("Pagaste %.2f €. ¡Gracias!", amount);
        System.out.println("Correo a " + contact.trim().toLowerCase() + ": " + text);
    } else if (channel.equals("viber")) {
        String text = String.format("Pagaste %.2f €. ¡Gracias!", amount);
        System.out.println("Viber a " + contact.replace(" ", "") + ": " + 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 imprimesBigDecimal, creado a partir de una cadenaUn 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