Revisiones de código efectivas: más allá del visto bueno

La revisión de código es la herramienta de calidad más poderosa que los equipos usan mal constantemente. En un extremo, las revisiones degeneran en discusiones de estilo por detalles insignificantes — discutiendo sobre la colocación de llaves mientras una condición de carrera llega a producción. En el otro extremo, los revisores aprueban PRs automáticamente con "LGTM" tras una mirada de 30 segundos, convirtiendo la revisión en un ritual vacío.

El objetivo de la revisión de código no es la perfección. Es detectar defectos que las herramientas automatizadas no pueden, compartir conocimiento en el equipo y mantener una base de código en la que todo el equipo pueda trabajar con confianza.

En qué fijarse

Una lista de verificación estructurada para revisiones evita los dos modos de fallo: pasar 20 minutos en formato mientras se pasa por alto un agujero de seguridad, y aprobar sin haber leído realmente el código.

interface ReviewChecklist {
  // High priority — these create production incidents
  correctness: [
    "Does the code do what the PR description says?",
    "Are edge cases handled (null, empty, max values)?",
    "Are error paths handled, not just the happy path?",
  ];
  security: [
    "Is user input validated and sanitized?",
    "Are authorization checks in place?",
    "Is sensitive data handled properly (no logging PII)?",
  ];
  // Medium priority — these create long-term pain
  design: [
    "Is this the right abstraction level?",
    "Will this be maintainable by someone who didn't write it?",
    "Does it follow existing patterns in the codebase?",
  ];
  // Low priority — automate these away
  style: [
    "Let the linter handle this.",
    "Seriously, configure the linter.",
  ];
}

El tiempo de revisión es finito. Gástalo primero en corrección y seguridad, segundo en diseño, y nunca en estilo — para eso existen prettier y ESLint.

Escribir comentarios de revisión útiles

La diferencia entre una revisión útil y una desmoralizante es el enfoque. Los comentarios deberían explicar la preocupación, no solo señalar lo que está mal.

// ❌ Unhelpful — what should they do instead?
// "This is wrong."
// "Don't do it this way."
// "Nit: use const here."
 
// ✅ Helpful — explains the concern and suggests an alternative
// "This query runs inside the loop, which will cause N+1
// queries at scale. Consider using a JOIN or batch query
// to load all related records in one call."
 
// ✅ Questions work better than commands for design decisions
// "What happens if this promise rejects? I don't see error
// handling — is it intentional to let it bubble up?"

Prefija los comentarios con su severidad:

**blocker**: This will cause data loss in production. The DELETE
query has no WHERE clause when `userId` is undefined.
 
**suggestion**: Consider extracting this into a utility function —
I've seen this pattern in three other files.
 
**question**: Is the timeout of 30s intentional? Our SLA is 5s
for this endpoint.
 
**nit**: Minor style preference, not blocking. Take it or leave it.

El prefijo le dice al autor qué se debe arreglar versus qué es opcional. Sin él, los autores tratan cada comentario como un bloqueo, generando frustración en ambas partes.

La responsabilidad del autor

Las buenas revisiones empiezan con buenos PRs. Un revisor trabajando con un PR de 2000 líneas y sin descripción está condenado al fracaso.

<!-- ❌ PR description that wastes reviewer time -->
## Changes
Updated the user service.
 
<!-- ✅ PR description that enables quality review -->
## Context
Users reported intermittent 500 errors during checkout.
Root cause: race condition in inventory reservation.
 
## Changes
- Added optimistic locking to inventory updates
- Added retry logic for concurrent modification errors
- Added integration test reproducing the race condition
 
## Testing
- [x] Reproduced the race condition with parallel requests
- [x] Verified fix under concurrent load (k6 script attached)
- [x] Existing tests pass
 
## Risks
- Retry logic adds ~50ms latency in the contention case
- Optimistic locking may surface errors in other flows
  that were silently succeeding with stale data

El tamaño del PR importa

const reviewEffectiveness = {
  "1-100 lines": { defectRate: "high", reviewTime: "15 min" },
  "100-400 lines": { defectRate: "medium", reviewTime: "30-60 min" },
  "400-1000 lines": { defectRate: "low", reviewTime: "60+ min" },
  "1000+ lines": { defectRate: "near-zero", reviewTime: "rubber stamp" },
};
// Research shows defect detection drops dramatically above 400 lines

Si tu PR tiene más de 400 líneas, divídelo. Apila PRs en ramas de funcionalidades si los cambios son secuenciales. Los revisores tienen un presupuesto de atención finito — los PRs grandes lo agotan antes de llegar al código crítico.

Antipatrones en la revisión

El guardián

// ❌ Gatekeeper review — imposes personal preferences as requirements
// "I would have done this differently. Please rewrite using
// the visitor pattern instead of the switch statement."
 
// ✅ Collaborative review — explains trade-offs
// "A switch statement works here. If we expect more than 5-6
// cases, a strategy pattern might be easier to extend. For now,
// this is fine — just flagging for future reference."

El guardián trata cada revisión como una oportunidad para reescribir el código a su manera. Esto crea un cuello de botella, desmoraliza a los autores y no mejora la calidad.

El perfeccionista

// ❌ Blocking on subjective preferences
// "Please rename `processData` to `transformUserRecords`."
// (4 rounds of review later, still debating the name)
 
// ✅ Approve and suggest
// "Approving — the logic is correct and well-tested.
// Optional: `transformUserRecords` might be more descriptive
// than `processData`, but not blocking on this."

Aprueba el PR cuando sea correcto y seguro, incluso si lo hubieras escrito de otra manera. Las preferencias pertenecen a guías de estilo y linters, no a la revisión de código.

Automatizando las partes aburridas

Cada comentario manual de revisión sobre formato, orden de imports o convenciones de nombres es un fallo de proceso. Automatiza el cumplimiento de estilo para que los humanos puedan centrarse en la lógica.

{
  "scripts": {
    "lint": "eslint . --max-warnings 0",
    "format:check": "prettier --check .",
    "typecheck": "tsc --noEmit"
  }
}

# .github/workflows/pr-checks.yml
name: PR Checks
on: [pull_request]
jobs:
  quality:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - run: npm ci
      - run: npm run lint
      - run: npm run format:check
      - run: npm run typecheck
      - run: npm test

Si estas comprobaciones pasan antes de que un humano vea el PR, el revisor nunca necesita comentar sobre punto y coma, imports sin usar o errores de tipos. Su revisión es puramente sobre lógica, diseño y corrección.

Construyendo una cultura de revisión

El proceso solo funciona si el equipo le da valor. Dos prácticas construyen una cultura de revisión saludable:

Revisiones oportunas: Establece una norma de equipo — los PRs deberían recibir la primera revisión dentro de las 4 horas durante el horario laboral. Los PRs estancados causan conflictos de merge, cambios de contexto y frustración. Si estás bloqueando a alguien, su PR es tu prioridad.

La revisión como aprendizaje: Que ingenieros junior revisen código de senior es tan valioso como lo contrario. El junior aprende patrones y contexto. El senior obtiene una perspectiva fresca y practica explicar decisiones. Hazlo bidireccional.

La mejor cultura de revisión es aquella en la que nadie teme abrir un PR o recibir feedback. Eso empieza por tratar las revisiones como resolución colaborativa de problemas, no como auditorías de código.

Conclusiones clave

1. Prioriza la corrección y la seguridad sobre el estilo — deja que los linters se encarguen del formato para que puedas centrarte en los bugs
2. Prefija los comentarios con severidad — blocker, suggestion, question, nit — para que los autores sepan qué se debe arreglar
3. Escribe descripciones de PR descriptivas — el contexto, los cambios, las pruebas realizadas y los riesgos permiten mejores revisiones
4. Mantén los PRs por debajo de 400 líneas — la detección de defectos cae dramáticamente para cambios más grandes
5. Aprueba cuando sea correcto, sugiere cuando sea opcional — bloquear por preferencias crea cuellos de botella sin mejorar la calidad
6. Revisa dentro de 4 horas — los PRs estancados se convierten en conflictos de merge y contexto perdido