Atlasingeniería

Code review en la prácticaCambios de comportamiento que el diff no muestraTema 1

Borrar código que todavía le servía a alguien

El comentario más frecuente de una review es «¿esto no se usaba?». Detrás hay un caso real: una migración de reglas a base de datos que dejaba afuera a todas las marcas asociadas sin que el diff lo dijera, y una regla para revisar borrados sin depender de la memoria del equipo.

Borrar código se siente como higiene: menos líneas, menos ramas, menos deuda. En un repositorio joven lo es. En uno con años encima, borrar es la operación más riesgosa que existe, porque el código viejo no está vivo o muerto: está vivo para alguien que no aparece en tu ambiente de pruebas.

De todos los comentarios de review que recibí en un año, el grupo más grande —más de uno de cada diez— eran sospechas sobre algo que se había sacado.

El caso: una migración que se comió un segmento

La tarea era ordinaria: una aplicación mostraba unos avisos propios —un cartel, un recordatorio— cuya lógica de «a quién se le muestra» estaba desparramada en clases del backend. La migración movía esas condiciones a documentos de configuración en una base, uno por tipo de aviso. Menos código, reglas editables sin deploy. Un buen cambio.

La clase que se borraba habilitaba el aviso para el sitio principal y para varias marcas asociadas, cada una con su país y condicionada a un experimento. El documento nuevo tenía una sola condición: la marca principal.

Por qué nadie lo vio

  1. El diff se leía perfecto. Una clase menos, un archivo de configuración más, tests en verde.
  2. La descripción del pull request hablaba de otra cosa: mencionaba el caso puntual que había motivado el ticket, no el resto de las condiciones que se movían.
  3. En el ambiente de pruebas no existen las marcas asociadas. Nadie podía tropezarse con el faltante probando a mano.
  4. La condición perdida era la que casi nunca se lee: el experimento que la habilitaba estaba al 0 %, así que también podía ser una decisión deliberada de producto. El diff no distingue «lo apagué a propósito» de «me lo llevé puesto».

Lo encontró alguien que recordaba esa lista de marcas y preguntó, en dos líneas, si dejar afuera a las marcas asociadas era una decisión de producto o un olvido. Costó un comentario y una respuesta. Si no aparecía, el aviso simplemente dejaba de verse en varios sitios y nadie tenía una alerta que lo dijera: no hay excepción, no hay error, no hay métrica que caiga de golpe. Se nota semanas después, cuando alguien pregunta por qué bajó una conversión.

Qué mirar antes de borrar

La pregunta útil no es «¿esto se usa?» sino «¿para quién deja de pasar esto?». Cambia el método de búsqueda: en vez de buscar referencias en el editor, se busca el conjunto de casos que la condición cubría.

Lo que se borraLa pregunta que correspondeDónde se verifica
Una condición de habilitación¿Qué marcas, países o flujos cubría?La lista completa, no el caso del ticket
Una función sin usos visibles¿La consume otro repositorio o un template?Búsqueda por nombre en toda la organización
Una entrada de configuración¿Algún ambiente la sigue leyendo?Configuración de producción, no la local
Un registro o log¿Alguien alerta o mide sobre eso?Dashboards y alertas existentes
Un texto o traducción¿Queda algún idioma sin clave?Todos los archivos de idioma, no sólo el propio
Una rama de experimento¿El experimento terminó o está pausado?La plataforma de experimentos, no el código
Borrar sin responder la columna del medio es apostar a que nadie del otro lado estaba mirando.

Antes de seguir, predecí

Un experimento está al 0 % desde hace meses y su rama de código sigue ahí. ¿Qué es lo más seguro?

Declarar el borrado

La segunda mitad del problema no es técnica. Aunque el borrado sea correcto, si no queda escrito que el comportamiento cambia, el equipo se entera cuando alguien lo nota.

Lo que hace revisable un borrado

  1. Listar en la descripción a quién deja de pasarle. Una línea: «deja de mostrarse en las marcas asociadas; el experimento que lo habilitaba está terminado».
  2. Separar el borrado del cambio funcional. Si el ticket era arreglar un caso puntual, la limpieza va en otro pull request: así el revisor puede aprobar uno y discutir el otro.
  3. Dejar rastro cuando la condición se vuelve invisible. Si algo deja de mostrarse porque falta configuración, que haya un registro que lo diga. Pasó en el mismo cambio: al mover las reglas a la base se perdió la advertencia que avisaba que una colección de reglas había quedado vacía. Sin ella, un ambiente sin datos cargados y un ambiente donde nadie es elegible se ven idénticos.
  4. Borrar completo o no borrar. Media limpieza —la clase sí, la constante no, la traducción tampoco— deja un repositorio donde nadie sabe qué está vigente.

Un borrado que parece seguro

src/notices/notice-list.tsx+1−8

Hay 4 problemas en este cambio. Tocá la línea donde creas que está.

@@ -4,18 +4,8 @@ import { useNotices } from './use-notices';
44
55
66
7
8
9
10
7
118
12
13
14
15
169
1710

Diez líneas menos, tres consecuencias, y ningún test en rojo.

Cómo se borra sin romper

Cierre

Autoevaluación

¿Lo entendiste?

¿Por qué el borrado del caso no se detectó en pruebas?
Una función no tiene usos en el repositorio. ¿Alcanza para borrarla?
¿Qué se pierde al sacar la advertencia de «no hay reglas cargadas»?