Guía para hacer Code Reviews en tu equipo

Las revisiones de código son una práctica que se ha extendido mucho en los últimos años, que consiste en que uno o varios desarrolladores revisen el nuevo código implementado por otro compañero, con el objetivo de detectar problemas de calidad del código, bugs, vulnerabilidades, malas prácticas, etc.. . Esto permite acortar los feedback loops, lo cual como sabemos es muy beneficioso ya que cuanto más tarde se detecte un problema, mayor será el coste de repararlo y el posible impacto en negocio.

Como el código es la materia esencial de la que se compone el software, y se crea y se mantiene diariamente por diferentes desarrolladores y equipos, es esencial contar con mecanismos que permitan asegurar su calidad. Hacer buenas code reviews, de forma consciente y siguiendo buenas prácticas, puede marcar la diferencia entre el éxito o el fracaso de una aplicación y en última instancia de un negocio. Por ello, uno de los primeros pasos recomendados para adoptar las revisiones de código, es contar con una guía de cómo hacerlas, que oriente y marque el camino para todo el equipo. El presente artículo pretende servir como una guía inicial, que los equipos puedan utilizar y adaptar a sus contextos.

Fundamentos del buen código

Antes de entrar en detalles, me parece interesante compartir este documento, que aunque se denomine Zen of Python, es una buena guía para cualquier lenguaje.

Lo bello es mejor que lo feo.
Lo explícito es mejor que lo implícito.
Lo simple es mejor que lo complejo.
Lo complejo es mejor que lo complicado.
Plano es mejor que anidado.
Repartido es mejor que denso.
La legibilidad es importante.
Los casos especiales no son tan especiales como para romper las reglas.
Aunque la practicidad gana a la pureza.
Los errores nunca deben pasar en silencio.
A menos que se silencien explícitamente.
Ante la ambigüedad, rechaza la tentación de adivinar.
Debe haber una -y preferiblemente sólo una- forma obvia de hacerlo.
Aunque esa manera puede no ser obvia al principio, a menos que seas holandés.
Ahora es mejor que nunca.
Aunque a menudo “nunca” es mejor que “ahora mismo”.
Si la implementación es difícil de explicar, es una mala idea.
Si la implementación es fácil de explicar, puede ser una buena idea.
Los espacios de nombres son una gran idea: ¡hagamos más!

Fuente: https://peps.python.org/pep-0020/

Objetivos de las Code Reviews

(Estilo) Detectar divergencias del estilo definido en las guías de estilo (es recomendable definir guías de estilo para el código de nuestra empresa) y en el código del proyecto.
(Arquitectura) Revisar si la implementación sigue la arquitectura del código.
(Modularidad) Revisar si el código nuevo está demasiado acoplado. Revisar que los componentes tiene cohesión, tienen un único propósito.
(Composición y herencia) Revisar si se usan correctamente la herencia y la composición. Y se da prioridad a la composición frente a la herencia.
(APIs) Si se crean o extienden APIs, revisar que son consistentes y limpias.
(Legibilidad) Comprobar que el código es fácil de entender, seguir y mantener
(Testeabilidad) Comprobar que el código tiene unit tests y tests automáticos que prueben la nueva funcionalidad.
(Prevenir defectos) Detectar posibles bugs y/o casos no cubiertos por el código
(Reparto del conocimiento) Familiarizar al resto de desarrolladores sobre los cambios que se van haciendo en el código. Permite que el conocimiento esté más repartido en el equipo.
(Formación) Transmitir el conocimiento de los programadores más seniors a los más nuevos (en la empresa) y los juniors.
(Seguridad) Comprobar que el código no expone credenciales de ningún tipo, y no tiene fallos de seguridad obvios

Definiendo las bases

El objetivo de la revisión no es criticar el trabajo de los compañeros, sino revisarlo para mejorarlo de forma conjunta, detectar posibles lapsus o fallos, y aprender, a través del feedback. Los comentarios deben ser siempre respetuosos y se deben recibir de forma abierta y agradeciendo el tiempo que ha dedicado nuestro compañero a revisar nuestro código. Nunca debe enfocarse la revisión como una competición entre adversarios, sino como un trabajo de equipo.

Da igual el seniority que tengamos como desarrolladores, todos somos humanos y cometemos fallos. Un buen desarrollador, independientemente de lo senior que sea, siempre está abierto al feedback aunque venga de un desarrollador más junior.

Pasos a seguir

Pre revisión

Abrir la Pull Request/Merge Request (o PR / MR) y leer la descripción. Si aún así, te falta contexto del cambio, abrir el ticket en el software de gestión de proyectos que uséis, y leerlo. Es importante leer los criterios de aceptación, para intentar detectar si el código puede cumplir con todos o no **(por supuesto, el objetivo de la revisión de código no es probarlo, pero en muchas ocasiones se pueden detectar criterios de aceptación no cubiertos simplemente mirando al código).
**
Verificar que la rama base es la correcta. A veces por error, se puede crear una PR contra main, cuando se quería integrar en otra rama. En el caso de que no, notificarlo y parar la revisión. Ya que si la rama base no es la correcta puede que estemos revisando cambios ya revisados.

Revisión

Empezar revisando la lista de ficheros cambiados/creados/eliminados. Revisar que la arquitectura de ficheros y módulos es consistente con lo que existe y las guías de buenas prácticas (si las hubiese). Revisar que los nombres siguen el naming convention.
Si el código tiene unit tests (lo recomendable), empezar por los unit tests puede ser una buena forma de entender si se cubren todos los criterios de aceptación o no. Además es una buena forma de entender el código desde fuera. Los unit tests deberían cubrir no solo el happy path, sino también corner cases y casos de error.
Revisar que el estilo del código sigue la guía de estilo (si la hubiese), si no es el caso, comprobar que es uniforme con el código ya existente. Revisar los nombres de las variables, clases, métodos, etc..
Revisar que la arquitectura del nuevo código respeta la arquitectura existente. Por ejemplo: las nuevas clases son coherentes con las clases existentes. La forma de organizar el código en ficheros sigue los patrones existentes.
Chequear que el código tenga bajo acoplamiento y tenga cohesión. Las clases y métodos deberían tener un único propósito. Un ejemplo de alto acoplamiento bastante común, es que el acceso al motor de base de datos para hacer queries esté repartido por todo el código. Esto causa un alto acoplamiento al motor específico de base de datos, complicando un posible reemplazo en el futuro, por ejemplo, entre otras cosas.
Revisar que no se ha duplicado código. DRY (Don’t Repeat Yourself)
Revisar que el código es sencillo de entender. Si vemos código complicado, deberíamos añadir un comentario a la PR, para que el desarrollador intenté hacerlo más sencillo y/o añadir comentarios para que se pueda entender.
1. Revisar que no se usen números mágicos o literales, lo cual hace el código menos entendible y menos mantenible. Sugerir el uso de constantes con un nombre autoexplicativo.
Si se ha modificado una API, revisar que los cambios son coherentes con el resto de la API, y revisar que se ha actualizado la documentación.
La idea es revisar el código nuevo, pero a veces para entenderlo bien necesitaremos leer un poco el contexto, el código ya existente. Hay veces que podemos ver que el cambio está incompleto o no es correcto, mirando el contexto.
Chequear que el código no incluye passwords, credenciales o información sensible de ningún tipo. Si fuera así, esta información ya está en el repositorio y podría verse comprometida, plantearse cambiar las credenciales.
Estar atento por si se ve algún problema de seguridad evidente a simple vista. Por ejemplo, inyecciones sql (utilizar un input de usuario directamente en una query de sql).
Revisar si los cambios deberían reflejar cambios en la documentación del repositorio (por ejemplo, en el README), y ver si se ha actualizado y se entiende correctamente. Por ejemplo, si se ha cambiado la forma de lanzar el proyecto en local, debería estar claramente actualizado en el README.

Post revisión

En el caso de que hayamos hecho comentarios, estar atentos a la posible respuesta y a los cambios en la PR, para volver a revisar.
Si no hemos hecho comentarios y lo vemos bien. Indicarlo en la PR y aceptarla. Aquí en función de las reglas que hayamos definido en nuestro equipo, podría ser necesario que la revisarán otras personas y seguir un proceso específico para mergear la PR.

Notas Finales

Esta guía sirve como referencia genérica para hacer code reviews de una forma uniforme a través de un equipo u organización. La idea es que cada equipo/organización la pueda customizar y extender a su caso particular, y trabajar de forma continua en mejorarla y adaptarla a sus circunstancias.

Además de todas estas comprobaciones manuales, es altamente recomendable incorporar al proceso herramientas de análisis de código estático, linters, y reglas de formateo en el IDE, entre otros. Estas nos permitirán detectar problemas, de forma rápida y barata, incluso antes de crear la PR para solicitar la revisión de código.

Y por supuesto, vuestros comentarios son siempre bienvenidos.