...si no se realizan correctamente </clickbait>.
Esta semana en mi empresa hemos hablado bastante sobre revisiones de código y me gustaría compartir mis reflexiones y experiencias al respecto.
Las code reviews por sí solas no sirven de nada
He visto muchas empresas donde para "mejorar la calidad del código" obligan a que cada cambio sea revisado por una persona diferente al autor sin modificar absolutamente nada del proceso global de desarrollo. Al final el resultado siempre es el mismo:
El proceso óptimo
Creo que antes de establecer ciegamente un proceso de revisión de código deberíamos plantearnos cómo hacerlo y si el equipo y la metodología de trabajo están preparados para aprovecharse de todas las ventajas que nos ofrece.
¿Y cuáles son estas ventajas? La mayoría de la gente de dará unas respuestas parecidas. Revisar el código sirve principalmente para detectar errores, para mejorar su calidad, para compartir conocimiento entre los miembros del equipo y para ayudar a unificar el estilo.
Ahora preguntémonos ¿Es la code-review el lugar más óptimo para detectar que falta un ; o que length está mal escrito? ¿Es discutir sobre nombres de variables y paréntesis el mejor uso del tiempo de un developer? Desde mi punto de vista tener comentarios de este estilo en los pull requests de GitHub es un indicio de un proceso que no está preparado para las revisiones en el que se hace perder tiempo y paciencia a los revisores y a los autores buscando defectos que se podrían solucionar fácilmente de otra forma.
Analizadores estáticos
Por eso creo que deberíamos utilizar Typescript para detectar errores de sintaxis antes de que lleguen a la revisión y una combinación bastante rígida de EsLint y Prettier para reducir al mínimo los comentarios relacionados con estilo de código (espacios, paréntesis, llaves...).
Para temas más arbitrarios los equipos deberían tener una guía de estilo de código consensuada y actualizada frecuentemente a con las discusiones recurrentes de las code-reviews. De esta forma conseguiremos unificar el estilo de código progresivamente y reducir todavía mas las discusiones "irrelevantes" en las code-reviews.
Tests automáticos
De la misma forma que utilizamos Typescript para detectar errores de sintaxis, deberíamos utilizar tests automáticos para detectar errores básicos en la lógica. No tiene sentido que el revisor tenga que dedicar tiempo a recorrer y revisar mentalmente el código para asegurarse de que funcionará bajo determinadas circunstancias. Al tener tests este tiempo se reduce drásticamente ya que los revisores simplemente tienen que centrarse en leer en los tests qué casos se están probando y pensar sobre aquellos que se le pueden haber olvidado al autor.
Pair/mob programming y code-previews
Por desgracia no hay herramientas que nos permitan compartir conocimiento en el equipo automáticamente pero ciertas prácticas como pair programming y mob programming han demostrado ser bastante útiles en este sentido. Otra práctica menos conocida y desde mi punto de vista de las más útiles es el "code-preview".
La mayoría de artículos dedicados a mejorar el proceso de review ponen el foco en el texto de la propia pull request o de los mensajes de commit. Es cierto que en entornos asíncronos es la única opción sin embargo un gesto tan pequeño como poner en común las posibles soluciones a un ticket puede ayudar muchísimo más que un commit de 30000 palabras.
En esta reunión se debería comentar y consensuar la solución adecuada a un problema determinado de forma que todos los implicados tendrán un contexto suficiente para poder aportar valor durante la code-review. Por lo general al abrir el código el revisor ya tendrá una idea de lo que se va a encontrar y es más probable que aparezcan comentarios del tipo "Sí añadimos una pequeña cache en esta función podemos ahorrarnos un montón de peticiones de red porque xxx no cambia". Cómo regla, los comentarios útiles son cosas que deberían transcender una vez minificado el código: Temas de arquitectura, casos sin contemplar, posibles errores...
Lenguaje adecuado
Las reviews pueden ser origen de tensiones en el equipo y habitualmente los programadores son reticentes a revisar código por ello conviene poner el foco en el lenguaje utilizado. Hablar en plural, o utilizar el condicional son pequeños trucos que suavizan el tono y reducen la probabilidad de chispas.
Tirano:
You totally forgot the most simple case so your code is going to crash.
Menos tirano:
I think we should take xxx case into account because this might crash under some circumstances.
Tamaño asequible
Para terminar el consejo más importante de todos: LAS PULL REQUESTS DEBEN SER PEQUEÑAS.
Cuando son grandes sufrimos múltiples inconvenientes: El revisor tiende a procrastinarlas y cuando por fin la revisa, el número de fallos por línea de código detectados es inversamente proporcional al tamaño del pull request.
Además, si un cambio de 100000 lineas está mal decir "por favor empieza de nuevo" es terriblemente frustrante para todas las partes y una pérdida de tiempo absoluta. Así que la alternativa es no comentar absolutamente nada o hacer comentarios triviales que no aportan valor.
Por ello recordamos: Las pull requests deben ser pequeñas, de aproximadamente 400 lineas! Aunque pueda parecer una locura, no es complicado hacerlo si cambiamos el proceso de desarrollo, dividimos los tickets en grupos de cambios pequeños y atómicos y utilizamos feature flags que nos permitan integrar código continuamente sin necesidad de liberar nuevas características a los clientes.
En resumen.
¡Antes de obligar al equipo a hacer revisiones a lo loco crea un entorno de trabajo adecuado!
- Utiliza analizadores estáticos para detectar errores de sintaxis y estandarizar el formato del código.
- Utiliza una guía de estilo actualizada para evitar discusiones innecesarias.
- Utiliza tests automáticos para detectar errores antes de la revisión.
- Haz sesiones de pairing y mob-programming con frecuencia para que el conocimiento fluya de forma natural entre los miembros del equipo.
- Procura hacer una code-preview de los tickets complicados para que todos los implicados tengan un contexto suficiente de qué se va a hacer y cómo.
- Divide las tareas de forma que tengas PRs pequeñas de no más de 400 lineas.
- Utiliza feature-flags para poder integrar código continuamente.
- Preocúpate de que la gente utilice un lenguaje amable en las revisiones.
Top comments (0)