Las code reviews no sirven para nada.

...si no se realizan correctamente </clickbait>.

Esta semana en mi empresa hemos hablado bastante sobre revisiones de cdigo y me gustara 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 cdigo" 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:

I Am Devloper

@iamdevloper

10 lines of code = 10 issues.

500 lines of code = "looks fine."

Code reviews.

09:58 AM - 05 Nov 2013

El proceso ptimo

Creo que antes de establecer ciegamente un proceso de revisin de cdigo deberamos plantearnos cmo hacerlo y si el equipo y la metodologa de trabajo estn preparados para aprovecharse de todas las ventajas que nos ofrece.

Y cules son estas ventajas? La mayora de la gente de dar unas respuestas parecidas. Revisar el cdigo 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 preguntmonos Es la code-review el lugar ms ptimo para detectar que falta un ; o que length est mal escrito? Es discutir sobre nombres de variables y parntesis 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 podran solucionar fcilmente de otra forma.

Analizadores estticos

Por eso creo que deberamos utilizar Typescript para detectar errores de sintaxis antes de que lleguen a la revisin y una combinacin bastante rgida de EsLint y Prettier para reducir al mnimo los comentarios relacionados con estilo de cdigo (espacios, parntesis, llaves...).

Para temas ms arbitrarios los equipos deberan tener una gua de estilo de cdigo consensuada y actualizada frecuentemente a con las discusiones recurrentes de las code-reviews. De esta forma conseguiremos unificar el estilo de cdigo progresivamente y reducir todava mas las discusiones "irrelevantes" en las code-reviews.

Tests automticos

De la misma forma que utilizamos Typescript para detectar errores de sintaxis, deberamos utilizar tests automticos para detectar errores bsicos en la lgica. No tiene sentido que el revisor tenga que dedicar tiempo a recorrer y revisar mentalmente el cdigo para asegurarse de que funcionar bajo determinadas circunstancias. Al tener tests este tiempo se reduce drsticamente ya que los revisores simplemente tienen que centrarse en leer en los tests qu casos se estn 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 automticamente pero ciertas prcticas como pair programming y mob programming han demostrado ser bastante tiles en este sentido. Otra prctica menos conocida y desde mi punto de vista de las ms tiles es el "code-preview".

La mayora de artculos 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 asncronos es la nica opcin sin embargo un gesto tan pequeo como poner en comn las posibles soluciones a un ticket puede ayudar muchsimo ms que un commit de 30000 palabras.

En esta reunin se debera comentar y consensuar la solucin adecuada a un problema determinado de forma que todos los implicados tendrn un contexto suficiente para poder aportar valor durante la code-review. Por lo general al abrir el cdigo el revisor ya tendr una idea de lo que se va a encontrar y es ms probable que aparezcan comentarios del tipo "S aadimos una pequea cache en esta funcin podemos ahorrarnos un montn de peticiones de red porque xxx no cambia". Cmo regla, los comentarios tiles son cosas que deberan transcender una vez minificado el cdigo: 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 cdigo por ello conviene poner el foco en el lenguaje utilizado. Hablar en plural, o utilizar el condicional son pequeos 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.

Tamao asequible

Para terminar el consejo ms importante de todos: LAS PULL REQUESTS DEBEN SER PEQUEAS.

Cuando son grandes sufrimos mltiples inconvenientes: El revisor tiende a procrastinarlas y cuando por fin la revisa, el nmero de fallos por lnea de cdigo detectados es inversamente proporcional al tamao del pull request.

Adems, si un cambio de 100000 lineas est mal decir "por favor empieza de nuevo" es terriblemente frustrante para todas las partes y una prdida 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 pequeas, 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 pequeos y atmicos y utilizamos feature flags que nos permitan integrar cdigo continuamente sin necesidad de liberar nuevas caractersticas a los clientes.

En resumen.

Antes de obligar al equipo a hacer revisiones a lo loco crea un entorno de trabajo adecuado!

• Utiliza analizadores estticos para detectar errores de sintaxis y estandarizar el formato del cdigo.
• Utiliza una gua de estilo actualizada para evitar discusiones innecesarias.
• Utiliza tests automticos para detectar errores antes de la revisin.
• 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 cmo.
• Divide las tareas de forma que tengas PRs pequeas de no ms de 400 lineas.
• Utiliza feature-flags para poder integrar cdigo continuamente.
• Preocpate de que la gente utilice un lenguaje amable en las revisiones.

Referencias

• Gua de estilo de Hola
• Gua de estilo de Google
• Cmo escribir un Pull Request
• Libro sobre code review