Mapa conceptual del code review

Introducción

La revisión de código es un aspecto muy importante en cualquier equipo de desarrollo. Si bien el trabajo sincrónico / pairing reduce drásticamente la necesidad de hacer revisiones asincrónicas y tardías (porque lo mismo que resuelve la revisión sucede en la sesión de trabajo con feedback rápido), existen contextos donde no es posible trabajar de esta manera y tenemos una instancia de revisión post-desarrollo.

El siguiente artículo muestra algunos aspectos a tener en cuenta al momento de hacer una revisión o definir / iterar un proceso de revisiones.

Aspectos Humanos

Lenguaje/Vocabulario

Elegir cuidadosamente nuestras palabras (evitar palabras ofensivas, tener cuidado con la brecha cultural, lo que a nosotres en una cultura nos suena bien, a otres en otra cultura podría ofender). Algo simple es volver a leer aquello que escribimos para sensar el tono acorde a la audiencia.

Críticas constructivas

Apuntar al feedback constructivo (más allá del "estaría bueno que..."). Si no sé, no opino (evitar cosas del estilo "no estoy seguro, pero esto creo que está mal"). Sugerir alternativas. Tener en cuenta el seniority de la persona y el tiempo que lleva trabajando en el proyecto. Las alternativas también deben ser realizables con un esfuerzo razonable, no aportan demasiado las posibles soluciones utópicas en el contexto actual del proyecto.

Solidaridad

La idea no es complicarle la vida a quien escribió el código, sino ayudarle a lograr el objetivo (tanto quien revisa como la persona que realizó el código tienen un objetivo en común, que es salir a producción con la mejor calidad posible en el menor tiempo posible).

Objetividad

Los prejuicios no suman para nada ("esto no me gusta"). Medir con la misma vara el código de diferentes personas (unificar criterios; que el review del código no dependa de factores como el estado de ánimo o la longitud del PR).

Responsabilidad

Si me piden code review, es fundamental responder a tiempo: cada equipo deberá acordar e iterar tiempos esperables de respuesta. Tratar de ser expeditivo, priorizar cerrar una revisión vs. tenerla por un largo tiempo "en preparación". Indicar aprobación sin ver los detalles del código, ¡tampoco sirve! se pierde oportunidad de aprendizaje. Si es necesario puede ser útil reservar tiempopara organizar mejor las revisiones.

Construcción de cultura

Comprender que el review no es algo que hacemos por inercia u obligación. Que sea un hábito no quiere decir que no lo estemos inspeccionando y adaptando como sea necesario. La idea es ir pudiendo lograr acuerdos colectivos y criterios unificados. Que todo el equipo de desarrollo aprenda y que el proceso contribuya positivamente en la calidad del resultado.

Aspectos de gestión

¿sincrónica o asincrónica?

• sincrónica o "en vivo": frente a la PC, o remotes por algún soft de videoconferencias. Tiene la ventaja de explicar lo que hicimos a quien revisa, y no entorpecer la comunicación ya que podemos aclarar cualquier cuestión en el momento. Para cambios grandes / complejos / arquitecturales es fundamental.
• asincrónica: por ejemplo, comentarios en GitHub. Ideal para cambios chicos, de impacto bajo, y que tienen baja probabilidad de generar discusiones largas o malentendidos.

¿bloqueante o no bloqueante?

• revisión bloqueante: cuando lo que queremos expresar es "esto no se debe poner en producción porque causaría X problema"
• revisión no bloqueante: cuando lo que queremos expresar es "opino que hay que corregir esto, pero dejo a tu criterio la decisión de arreglarlo o seguir como está; charlemos cómo trabajar esta situación"

Importante: ¡usar con criterio! Si bloqueamos todo, la cosa deja de fluir.

Acá recomiendo tomar como inspiración el artículo "Ship, Show, Ask" de Martin Fowler que indica 3 maneras de proceder con un cambio acorde a su criticidad y a lo que el equipo tenga acordado.

Estrategia para las correcciones

Una vez que recibimos alguna sugerencia, tenemos que saber qué hacer con ellas. No es bueno que las ideas queden sin resolverse ya que es una oportunidad para el aprendizaje.

A veces hacer alguna corrección en el momento es difícil y una idea para no bloquear tanto es crear nuevas tareas para las sugerencias, que podrán ser priorizadas y realizadas cuando se crea necesario. Obviamente, es importante darle seguimiento a esto. Y otras veces también se disparan discusiones interesantes (quizás de cambios más grandes), en cuyo caso hay que plantearse si no es necesario llevar la discusión a todo el equipo y reunirse para definir cómo accionar ante esto.

Sub-reviews, o reviews más específicas

• Agnóstica del negocio: sólo para validar cuestiones de diseño / buenas prácticas / técnicas puntuales. Ejemplo: "llamemos a nuestro gurú de AWS para revisar esta parte del PR"
• Meramente de negocio: viendo el código, ¿parece que lo que debería cumplirse se está cumpliendo?
• De alguna cuestión especial técnica: para algunos casos es útil traer un experto en el tema, sobre todo si nuestros cambios son críticos. Ejemplo: "¿qué problemas de performance podría causar la ejecución de este código a gran escala?"

Mejora continua

Como toda práctica ágil es necesario asegurar la mejora continua. Una idea es tenerlo como tema recurrente en las retrospectivas y ser sincero tanto cuando sale bien como cuando no. Es importante que todo el equipo vaya aprendiendo y la mejor manera es haciéndolo, aprendiendo de cómo lo hacen los demás, equivocándonos y volviendo a intentar.

Resumen de la revisión

No está bueno dejar varios comentarios sueltos sin una conclusión. ¿Me parece que puede seguir su camino o quiero bloquearlo? ¿Qué parte del review es la más importante para que priorice la persona?

Aspectos Técnicos

Contexto para quien revisa

La(s) persona(s) que va(n) a revisar nuestro código tiene(n) que tener un entendimiento acerca de ello. ¿En qué consiste este cambio? ¿Por qué lo estamos haciendo ahora? ¿Que efectos esperamos al ponerlo en producción?

Por eso el desarrollador debe poder responder a estas preguntas. 

mínimamente link a un ticket con una descripción, explicar qués y por qués.

Los templates de GitHub sirven como buen punto de partida. Iterar lo que al equipo le resulte más cómodo.

Objetivo: demostrar que sabemos lo que estamos haciendo más allá de tener el código; y que entendemos el contexto en el cual nuestros cambios se van a aplicar.

Detectar complejidad oculta/problemas conocidos

Muchas veces el código no nos cuenta toda la historia. Algunas cosas las detectamos ejecutándolo, viendo sus logs, o recordando errores comunes relacionados más a lo no funcional que a lo funcional (por ejemplo, performance, robustez, seguridad).

En ese caso la experiencia de la persona que revisa es vital para remarcar posibles bugs / problemas de performance o cualquier cuestión que no sea evidente en el conjunto de cambios que se está revisando.

Cuestionamientos sobre el diseño de la solución

Es posible que al revisar el código nos encontremos con alguna mejora al diseño (organización del código, división de responsabilidades, nombres, decisiones de tipos de datos, etc). No es el momento ideal para trabajar estos cambios pues requiere un ida y vuelta costoso, que podría haberse evitado con una sesión sincrónica de trabajo. No obstante, hay casos donde se puede conversar acerca de estos temas, planteándolos como futuros cambios, o simplemente para explicitar un poco más los _trade-offs_ de la solución elegida.

Buenas prácticas, estilo y consistencia

Estas son cuestiones que está bueno revisar pero no deberían ser bloqueantes para avanzar con la integración. También son cosas que una herramienta automática bien configurada puede hacerlo por nosotros, y hacerlo mejor. Tal es el caso de los analizadores estáticos de código o linters (ESLint, Rubocop, KTlint, Sonar, por mencionar algunos).

Después, es importante tener cuidado con señalar cosas que no aporten un valor real al resultado final: por ejemplo, el orden que elegimos para los commits.

Testing

Este es un apartado vital que en el caso de programadores con poca experiencia tiene aún más sentido poner el foco.

Debemos preguntarnos: ¿Cómo es el testing de lo que vamos a poner en producción? Tiene que haber un plan... ¿testeamos de manera manual o automática? ¿a qué nivel? ¿integración / unitario/ end-to-end?

Acá también es importante apoyarnos en herramientas que nos ayuden a encontrar mejoras, por ejemplo, el reporte de cobertura de tests: no nos va a decir nada sobre la calidad de los tests, pero nos va a señalar si hay partes del código sin tests asociados, que es información muy válida para discutir como parte de una revisión.

Plan para el release

Muchas veces la aprobación de una revisión de código habilita un merge y un posterior despliegue. Esto va a depender del esquema de releases que estemos utilizando.

En cualquier caso, debemos detenernos a pensar qué esperamos que suceda cuando este código pase por los ambientes de despliegue (sobre todo producción).

¿Hay algún paso previo a realizar antes de desplegar? ¿Qué pasa si falla algo relacionado a este código? ¿Cómo revertimos este cambio? ¿Cómo podemos observar/monitorear estos cambios?

Esto se vuelve mucho más útil cuando es muy crítico lo que estamos cambiando, es un sistema 24x7, tiene muchos usuarios, debe mantener estricta retrocompatibilidad, entre otros contextos posibles.

Preguntas que nos pueden ayudar a definir nuestro propio proceso de revisión

Para finalizar, quería dejar una serie de preguntas para tener a mano para definir / iterar nuestro proceso de revisión.

• ¿Cómo se pide? (mail / chat / en persona)
• ¿Representamos este estadío en nuestro issue tracker? Es bueno para medir lead time y potenciales cuellos de botella.
• ¿Cuántos revisores? ¿y quiénes? Si revisó una persona, ¿ya termina el proceso?tip: a criterio del desarrollador, basado en la confianza.
• ¿En qué momento lo pido? ¿puedo pedir review aun si no está listo del todo? ¿es necesario que sea un paso "formal"?
• ¿Cómo se hace el seguimiento de la revisión? ¿Cómo se piden y se realizan re-revisiones?
• ¿Hay un tiempo límite para revisar el código? ¿Explícito o implícito?
• ¿Necesito aprobación explícita? ¿De cualquier persona o de alguien en particular?
• ¿Pair programming anula la necesidad de revisión? ¿En qué contextos?