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:

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Caja: {checkout_total(100, True):.2f} €")
print(f"Factura: {invoice_total(100, True):.2f} €")

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: invoice_total 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

def checkout_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


def invoice_total(subtotal, vip):
    discount = subtotal * 0.05 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery


print(f"Caja: {checkout_total(100, True):.2f} €")
print(f"Factura: {invoice_total(100, True):.2f} €")

Paso a paso, en palabras

1. invoice_total nació como un copy-paste de checkout_total: 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 checkout_total, 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:

def order_total(subtotal, vip):
    discount = subtotal * 0.10 if vip else 0
    delivery = 0 if subtotal >= 50 else 4.90
    return subtotal - discount + delivery

print(f"Caja: {order_total(100, True):.2f} €")
print(f"Factura: {order_total(100, True):.2f} €")

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:

def place_order(email, subtotal, city):
    # comprobación
    if "@" not in email:
        print("Correo no válido: " + email)
        return
    if subtotal <= 0:
        print("Carrito vacío")
        return
    # total
    delivery = 3.90 if city == "Sofía" else 5.90
    if subtotal >= 50:
        delivery = 0
    total = subtotal + delivery
    # aviso
    text = f"Tu pedido por {total:.2f} € ha sido aceptado."
    print("Para " + email + ": " + text)
    # log
    print(f"[log] pedido {email} {total:.2f}")

place_order("ana@example.com", 34, "Plovdiv")
place_order("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:

def problem_with(email, subtotal):
    if "@" not in email:
        return "Correo no válido: " + email
    if subtotal <= 0:
        return "Carrito vacío"
    return None


def delivery_fee(subtotal, city):
    if subtotal >= 50:
        return 0
    return 3.90 if city == "Sofía" else 5.90


def notify_customer(email, total):
    text = f"Tu pedido por {total:.2f} € ha sido aceptado."
    print("Para " + email + ": " + text)


def log(email, total):
    print(f"[log] pedido {email} {total:.2f}")

def place_order(email, subtotal, city):
    problem = problem_with(email, subtotal)
    if problem is not None:
        print(problem)
        return
    total = subtotal + delivery_fee(subtotal, city)
    notify_customer(email, total)
    log(email, total)

place_order("ana@example.com", 34, "Plovdiv")
place_order("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ó:

@dataclass
class Order:
    customer_name: str = ""
    customer_email: str = ""
    customer_phone: str = ""
    delivery_city: str = ""
    delivery_street: str = ""
    delivery_postcode: str = ""
    card_number: str = ""
    card_expiry: str = ""
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery_city = "Varna"
print("Envío a " + order.delivery_city)

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:

@dataclass
class Customer:
    name: str
    email: str
    phone: str


@dataclass
class Address:
    city: str
    street: str
    postcode: str


@dataclass
class Card:
    number: str
    expiry: str


@dataclass
class Order:
    customer: Customer | None = None
    delivery: Address | None = None
    card: Card | None = None
    subtotal: float = 0
    discount: float = 0
    sms_sent: bool = False
    email_sent: bool = False

order = Order()
order.delivery = Address("Varna", "Calle del Mar 5", "9000")
print("Envío a " + order.delivery.city)

Salida

Envío a Varna

@dataclass escribe __init__, __repr__ y __eq__ a partir de los campos. La dirección ahora es un valor: se pasa, se compara y se comprueba en un solo lugar. Address | None dice que puede no haber dirección en absoluto.

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?

total = 0
for _ in range(3):
    total += 0.10
print(total == 0.30)
print(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 float para abreviar. En código real:

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal("0.10")
print(total == Decimal("0.30"))
print(total)

Salida

True
0.30

Decimal, del módulo decimal, guarda los decimales exactamente. Se crea a partir de una cadena: Decimal(0.10) toma el float ya incorrecto y conserva el error.

from decimal import Decimal

total = Decimal("0")
for _ in range(3):
    total += Decimal()
print(total == Decimal("0.30"))
print(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:

import re
from dataclasses import dataclass


@dataclass(frozen=True)
class Phone:
    value: str

    def __post_init__(self):
        digits = re.sub(r"[ ()-]", "", self.value)
        if digits.startswith("0"):
            digits = "+359" + digits[1:]
        if not re.fullmatch(r"\+359\d{9}", digits):
            raise ValueError("Teléfono no válido: " + self.value)
        object.__setattr__(self, "value", digits)

a = Phone("0888 123 456")
b = Phone("+359 888 123 456")
print(a.value)
print("mismo número" if a == b else "números distintos")
try:
    Phone("0888 12")
except ValueError as e:
    print(e)

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.

frozen=True prohíbe los cambios después de crear el objeto. Por eso el número limpio se guarda con object.__setattr__ en __post_init__, el único lugar donde esto es aceptable. == viene de @dataclass.

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

1. digits = "+359" + digits[1:]↑↓
2. object.__setattr__(self, "value", digits)↑↓
3. if not re.fullmatch(r"\+359\d{9}", digits):↑↓
4. digits = re.sub(r"[ ()-]", "", self.value)↑↓
5. if digits.startswith("0"):↑↓
6. raise ValueError("Teléfono no válido: " + self.value)↑↓

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:

opted_out = ["+359888123456"]
notify_shipped("0888 123 456", opted_out)
notify_shipped("0877 555 010", opted_out)

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.

1def notify_shipped(phone, opted_out):2 if phone in opted_out:3 return4 print("SMS a " + phone + ": Tu pedido está en camino.")

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:

def price(kind):
    if kind == "mensajero":
        return 5.90
    elif kind == "oficina":
        return 3.90
    elif kind == "locker":
        return 2.90
    raise ValueError("Tipo de envío desconocido: " + kind)


def days(kind):
    if kind == "mensajero":
        return 1
    elif kind == "oficina":
        return 2
    elif kind == "locker":
        return 3
    raise ValueError("Tipo de envío desconocido: " + kind)

for kind in ["mensajero", "oficina", "locker"]:
    print(f"{kind}: {price(kind):.2f} €, 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:

@dataclass(frozen=True)
class Delivery:
    price: float
    days: int


DELIVERY = {
    "mensajero": Delivery(5.90, 1),
    "oficina": Delivery(3.90, 2),
    "locker": Delivery(2.90, 3),
}

for kind in ["mensajero", "oficina", "locker"]:
    d = DELIVERY[kind]
    print(f"{kind}: {d.price:.2f} €, 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:

def send_receipt(channel, contact, amount):
    if channel == "sms":
        text = f"Pagaste {amount:.2f} €. ¡Gracias!"
        print("SMS a " + contact.replace(" ", "") + ": " + text)
    elif channel == "email":
        text = f"Pagaste {amount:.2f} €. ¡Gracias!"
        print("Correo a " + contact.strip().lower() + ": " + text)
    elif channel == "viber":
        text = f"Pagaste {amount:.2f} €. ¡Gracias!"
        print("Viber a " + contact.replace(" ", "") + ": " + text)

send_receipt("sms", "0888 123 456", 24.5)
send_receipt("email", " Ana@Example.com ", 24.5)
send_receipt("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 imprimesDecimal, 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