Qué se comenta realmente en un code review
Cerca de mil comentarios de code review recibidos en un año, sobre una aplicación con varias marcas, países e idiomas. Casi ninguno habla de algoritmos: la mayoría son preguntas de dos líneas sobre un nombre, un valor suelto o algo que se borró.
Cuando alguien estudia para su primer trabajo imagina que en la revisión de código le van a discutir la complejidad de un algoritmo. Después llega la primera review real y son once comentarios que dicen «¿esto se usa?», «usá una constante», «¿por qué default a esto?».
Este post no es una opinión sobre cómo debería ser una review. Es el recuento de cerca de mil comentarios reales dejados a lo largo de un año en mis pull requests, sobre una aplicación con varias marcas, varios países y varios idiomas. Los agrupé por patrón y saqué todo lo demás: lo que se publica acá es la forma del comentario, nunca el código, el repositorio ni quién lo escribió.
La forma de una review, en números
De ese total, unos cuatro de cada diez los escribieron otras personas del equipo, dos de cada diez los dejó un revisor automático y el resto son respuestas mías dentro de las mismas conversaciones.
Lo primero que sorprende es el tamaño: el comentario humano promedio tiene 84 caracteres, y 7 de cada 10 no llegan a 80. Una review no es un ensayo; es una ráfaga de frases cortas.
Lo segundo es el modo: 1 de cada 3 comentarios humanos termina en signo de pregunta. No «esto está mal», sino «¿por qué acá sí y en la otra caja no?». La pregunta es la herramienta principal del revisor, porque quien revisa casi nunca tiene todo el contexto del cambio.
| Patrón | Peso | Cómo suena |
|---|---|---|
| Borrar, o sospechar de algo borrado | 13 % | «¿esto no se usaba?», «¿por qué se saca?» |
| Estilos y sistema de diseño | 9 % | «usá la paleta», «los estilos van en una clase» |
| Extraer a una función o helper | 8 % | «llevalo a un helper así lo usan las otras pantallas» |
| Banderas y experimentos | 7 % | «¿ese experimento no ganó ya?», «falta una condición» |
| Nombres | 6 % | «poco descriptivo», «no le pondría el nombre del experimento» |
| Marcas, socios y países | 6 % | «ojo, ese id no es lo mismo que esa marca» |
| Valores sueltos que deberían ser constantes | 5 % | «usar constantes», «¿esto hardcodeado?» |
| Nulos y respuestas que no llegan | 3 % | «si eso queda null puede romper el front» |
| Traducciones y encoding | 3 % | «faltan los otros idiomas», «se te rompió el acento» |
| Consultar con alguien más | 3 % | «validemos esto con diseño», «preguntemos en la daily» |
Las tres preguntas que se repiten
Detrás de casi todos los comentarios hay una de estas tres preguntas. Vale la pena hacérselas uno mismo antes de pedir la revisión: cada una que contestás en la descripción del pull request es un ciclo de ida y vuelta que no ocurre.
Lo que el revisor está tratando de averiguar
- ¿Qué cambia para el usuario que no está en el diff? Un archivo de configuración con una regla menos puede dejar afuera a un país entero. El código se lee, el comportamiento se deduce.
- ¿Esto ya existía? Casi siempre hay un helper, una constante o un módulo que hace lo mismo. Quien revisa conoce el repositorio mejor que quien llegó a tocar un archivo puntual.
- ¿Por qué acá sí y allá no? La inconsistencia es la señal más barata de detectar y la que más bugs anticipa: si una pantalla valida algo y la de al lado no, una de las dos está mal.
Antes de seguir, predecí
El revisor automático y el humano miran cosas distintas
Los comentarios automáticos no compiten con los humanos: casi no se superponen.
| Encuentra | El revisor automático | El humano del equipo |
|---|---|---|
| Nulos posibles y excepciones | Muy bien: rastrea el camino del valor | A veces, por memoria de un incidente |
| Typos y detalles de formato | Siempre | Casi nunca |
| Contradicción entre el título del PR y el código | Sí, lo marca explícitamente | Sí, pero lo pregunta |
| Que ese identificador no es el de esa marca | No: no conoce el dominio | Sí, es lo que más sabe |
| Que hay que hablar con otro equipo | Nunca | Sí: «consultemos esto en la daily» |
| Que ese experimento ya terminó | No | Sí |
Cómo se reduce la lista
Casi todos estos comentarios se pueden anticipar. Lo que más funcionó, en orden de impacto:
Antes de pedir la revisión
- Leer el diff completo como si fuera de otra persona. Ahí aparecen los cambios de comillas
del editor, el
console.log, la coma que quedó y el archivo que se tocó por accidente. - Escribir en la descripción qué cambia de comportamiento, no qué archivos se tocaron. «Deja de mostrarse en las marcas asociadas» es la línea que evita diez preguntas.
- Buscar antes de escribir. Un
grepdel concepto antes de crear un helper nuevo. - Separar lo que no es del ticket. Si hace falta un refactor, va en otro pull request: mezclado con el cambio real obliga a revisar dos cosas a la vez y esconde la importante.
- Dejar preguntadas las dudas propias. Un comentario propio que dice «acá defaulteé a esto, ¿es lo esperado?» convierte una discusión larga en una respuesta de una línea.
Más a fondo · nivel seniorPor qué las preguntas cortas son una buena señal
Un equipo que pregunta en vez de afirmar está asumiendo que quien escribió el cambio tiene contexto que el revisor no tiene. Eso mantiene la revisión barata: nadie tiene que reconstruir el ticket entero para opinar. El costo aparece cuando la pregunta corta reemplaza a una decisión que nadie toma —«¿y si lo vemos en la daily?» repetido tres reviews seguidas—; ahí el patrón deja de ser eficiencia y pasa a ser una decisión de producto sin dueño.
Encontrar los comentarios antes de leerlos
Hay 4 problemas en este cambio. Tocá la línea donde creas que está.
| 14 | 14 | ||
| 15 | 15 | ||
| 16 | |||
| 17 | |||
| 18 | |||
| 19 | |||
| 20 | |||
BloqueaEl 0.15 no dice de dónde sale Un porcentaje escrito a mano en el medio de la función. Dentro de seis meses nadie va a saber si es el descuento de este cupón, el máximo permitido o un valor de prueba que quedó. Y cuando cambie, va a haber que buscar 0.15 en todo el repositorio. El descuento tiene que venir del cupón, y si hay un tope, ser una constante con nombre. | |||
| 21 | |||
| 22 | |||
PreguntaUn TODO que pregunta algo que la review puede contestar Si la duda es real, éste es el momento de resolverla: quien revisa está leyendo justo eso. Un TODO con una pregunta abierta que entra a la rama principal casi nunca se contesta después, y queda como una marca de que algo no estaba resuelto. | |||
| 23 | |||
SugerenciaUn booleano negado, negado otra vez «si no no aplicable» obliga a traducir dos veces en cada lectura. Con hasItems la condición se lee sola. Es el segundo comentario más repetido después del de las constantes, y el más barato de arreglar: es cambiar un nombre. | |||
| 24 | |||
| 25 | |||
| 26 | |||
| 27 | |||
| 16 | 28 | ||
| 17 | 29 | ||
Cuatro comentarios, ninguno sobre algoritmos. Así es la mayoría de las reviews reales.
Qué se lleva una review
Cierre
Autoevaluación
¿Lo entendiste?
Práctica
BloqueaSe usa el resultado sin comprobar que exista
findByCode devuelve el cupón o nada, y acá se lee expiresAt de una. Con un código inexistente —o mal tipeado por el usuario— esto explota con un error que no dice nada sobre cupones. Es el comentario más frecuente de todos: el resultado de una búsqueda se usa antes de preguntarse si hubo resultado.