(también conocido como "Claves para un code review exitoso" presentado en Ágiles 2017)
por Nahuel Garbezza (nahuel@10pines.com) Twitter/Github: @ngarbezza
Última revisión: Jun 2020
Elegir cuidadosamente (evitar palabras ofensivas, tener cuidado con brecha cultural, lo que a nosotres en una cultura nos suena bien, a otres en otra cultura podría ofender).
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, nada de utopías.
La idea no es complicarle la vida al 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).
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).
Si me piden code review, es fundamental no colgarme. Tratar de ser expeditivo. Dar solo pulgar para arriba sin ver el detalle tampoco sirve! Se pierde oportunidad de aprendizaje. Si es necesario puede ser útil bloquear tiempo de agenda para organizar mejor las reviews.
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.
- 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.
- review bloqueante: "esto no se debe poner en producción porque causaría X problema"
- review no bloqueante: "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.
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.
- agnóstica del negocio: sólo para validar cuestiones de diseño/buenas prácticas/técnicas puntuales ("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 ("qué problemas de performance podría causar la ejecución de este código a gran escala")
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 "por ejemplo" (viendo a los demás haciéndolo).
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?
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.
(ya sean bugs potenciales, problemas de performance, por ejemplo)
(no el qué sino el cómo) ojo! se puede hacer largo. importante entender trade-offs
está bueno revisar pero claramente esto no es bloqueante mejor si lo hace una herramienta automática (ejemplo, linters como RuboCop en Ruby) ojo con las cosas que no dan valor: como por ejemplo orden de los commits
¿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?
¿qué pasos debo seguir? ¿qué pasa si falla? ¿cómo revertimos este cambio? Útil cuando es muy crítico lo que estamos cambiando
- ¿Cómo se pide? (mail/chat/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 del review? ¿Cómo se piden y se realizan re-revisiones?
- ¿Hay deadline 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 mata code review? ¿En qué contextos?