Cómo llevar a cabo una code review y qué puntos tener en cuenta

Hemos estado mirando como hacer codes reviews del código que subimos a producción, para mí es algo bastante nuevo y voy a intentar plasmar algunas de las cosas que he encontrado sobre todo en este artículo (Code Review Best Practices) para aprender como llevar a cabo una code review y que puntos tener en cuenta.

Brevemente, una code review es una discusión entre dos o más programadores sobre cambios en el código para entregar/reparar una funcionalidad. Existen multitud de artículos que hablan sobre los beneficios del code review, incluyendo el compartir conocimiento, la calidad de código y el desarrollo profesional de los programadores al ver como otros desarrolladores solucionan problema. Por el contrario he encontrado muy pocos artículos sobre como hacer revisiones, qué buscar en una revisión y que puntos discutir a la hora de comentar con el desarrollador que ha hecho el código.

¿Qué mirar durante una revisión?

Arquitectura / Diseño

• Principio de única responsabilidad: La idea es que una clase solo debería tener una razón para existir. Es más difícil de lo que parece. Normalmente también habría que aplicar este principio a los métodos. Una pequeña regla sería que si tenemos que usar «y» para describir lo que hace una clase, seguramente no estemos aplicando bien el principio de única responsabilidad
• Principio Abierto/Cerrado: Si estamos trabajando con un lenguaje orientado a objetos, ¿están los objetos abiertos para extensión y cerrados para modificación? ¿Qué pasa si necesitamos añadir otro «loquesea»?
• Duplicación de código: Debemos aplicar la regla de los 3 strikes (la 1º vez escribes el código, la 2 vez lo copiasypegas y la 3º vez haces algo para evitar la duplicidad). Básicamente refactorizar cuando sea necesario.
• Examinar el código buscando patrones/antipatrón: Si revisamos el código podemos identificar algún patrón o la posibilidad de evitar algún antipatrón. Este punto lo irá dando la experiencia, pero si ya conocemos nuestro proyecto podemos intentar no tropezar 2 veces en la misma piedra.
• Boyscout: Si tocamos partes del código horripilantes, lo mejor es intentar dejar el código mejor de lo que lo hemos encontrado, aunque solo sea tabularlo, poco a poco iremos mejorándolo.
• Potenciales bugs: ¿Revisando el código detectas un caso que el desarrollador no contempló? ¿Terminan los bucles de la manera que esperas?
• Manejo de errores: ¿Se manejan correctamente las excepciones? ¿Hemos añadido errores personalizados para esa parte del código? Si es así, ¿son útiles?

Estilo

• Nombres de los métodos: ya hemos hablado de esto antes, el naming es uno de los problemas de la informática, si el método se llama ‘get_final_price’ y no sabemos si el precio será formateado, sin formato, con puntos, sin puntos, si además del precio hace otro cálculos…
• Nombre de las variables: Estamos en el mismo caso anterior, tener variables con nombres como t,varB,varA… hace que el código se vuelva irrevisable e ininteligible. Ser verboso a la hora de dar nombre a variables no cuesta nada y después ayuda muchísimo.
• Longitud de las funciones: En el artículo de Object Calisthenic ya hablamos de esto, tener funciones de más de 20 lineas (en PHP) hacen que un programador no pueda entender de un vistazo todo el código y se pase más tiempo haciendo scroll que trabajando.
• Longitud delas clases: Es el mismo caso anterior, tener muchos métodos hace que el código se vuelva difícil de segur, lo más difícil de estas reglas es llegar a un punto de equilibrio para tener «manga ancha» cuando es necesario y ser severo cuando toca.
• PHPDoc: Saber que devuelve un método y tener un PHPDoc o similar es la cosa más sencilla del mundo y después IDE como PHPStorm hacen el resto, podemos ver la notación de una función de manera sencilla y ágil.
• Comentarios de código: Hay escuelas en esto, desde lo que te dicen, no comentes nada hasta los que comentan el incremento de una variable. En la revisión somos libres de borrar comentarios fuera de lugar y añadir comentarios de código que no entendamos
• Número de argumentos en los métodos: Creo que 3 es más que suficiente, un método que necesite 5 o 6 argumentos, creo que no es útil y será difícil de testear.
• Legibilidad: El código es fácil de seguir ¿hemos tenido que hacer una pausa en la review para descifrarlo?

Testing

• Cobertura de código: Sobre todo en nuevas funcionalidades ¿Los test cubren todo el código? ¿Se cubren las condiciones de fallo? ¿Son los tests fáciles de leer? ¿Son muy grandes? ¿Son lentos?
• Testing con mesura: Testear todo al 100% es muy complicado, seamos realistas tener test unitarios, de integración y funcionales de cada una de las funcionalidades en producción, incluso del código Legacy es muy muy complicado. La idea aquí es cuanto necesitas testear para estar seguro de que la funcionalidad funciona correctamente? Tener un balance entre test unitarios y test de integración es complejo, bajo mi punto de vista lo ideal es tener muchos tests unitarios y solo unos pocos de integración. Sobre todo para que a la hora de ejecutar los tests todo sea rápido y no acabemos con: «tengo test, pero no los ejecuto porque son lentos».
• Numero de Mocks: Los mocks son geniales, Pero Mockearlo todo no es genial. En mi opinión una buena regla podría ser si tenemos más de 3 mocks en un test, debemos revisar el test y el método que prueba ese test. No obstante las code review son para discutir y esto es discutible.
• Conociendo los requisitos: Hacer un documento que indique como probar la funcionalidad, aunque sea de manera manual es algo necesario aunque tedioso, necesitamos tener documentadas las funcionalidades sobre todo de cara a facilitar las cosas al equipo de QA.

Revisar nuestro propio código primero

Una de las funcionalidades que me encantan de PHPStorm es que antes de commitear los cambios puedo ver las diferencias de código de una manera sencilla. Revisar el código con esta guía antes de commitear es fundamental. No podemos hacer una code review a un compañero sin haber practicado antes con nuestro propio código. Además tenemos que estar bien seguros de que nuestro código pasará la revisión antes de enviarlo a un compañero.

Como manejar las code review

Creo que las code review tienen un fuerte componente humano, es una manera de aprender tanto al revisar como al ser revisado. Eso sí hay que tener en cuenta que el código es fruto del esfuerzo de una persona y todas las personas tenemos corazoncito.

• Si tenemos dudas, preguntamos: ¿qué hace este método? Si este requisito cambia, ¿como afrontarías el cambio? ¿como sería más mantenible este método?
• Reforzar las buenas prácticas: La idea detrás de los code review es mejorar como programadores. Que alaben nuestro código siempre es bueno, nos hace sentir bien y nos refuerza los hábitos positivos.
• Discutir en persona los detalles: En ocasiones, recomendar un cambio en la arquitectura o en la manera de resolver el problema puede acarrear una discusión, lo mejor es intentar siempre que sea posible) comentar estos cambios en persona, ya que el desarrollador ha hecho un esfuerzo en sacar adelante una funcionalidad y por ello escribir un email diciendo podrías cambiar esto porque bla bla bla… es frío. Siempre es mejor intentar hablar en persona y preguntar el porqué de las decisiones tomadas.
• Explicaciones razonables: Todo tiene un porqué y la mejor manera de averiguarlo es preguntándole. Este punto va en consonancia con el anterior, necesitamos saber el contexto para ver la solución.
• Revisamos el código, solo el código: Es muy fácil comentar sobre un código, pero cuando lo estamos escribiendo, no es tan fácil. No se si me explico, es sencillo criticar o comentar un código, por eso hay que tener mesura a la hora de las reviews. El código resuelve una funcionalidad, para eso se hizo, por ello no debemos hacer sobre ingeniería ni arquitecturas complicadas, el código debe resolver el asunto para el que se creó, por lo que llegar más allá es sobre pasarse.
• Sugerencias las justas: cada desarrollador tiene una manera de escribir código, declarar variables… en definitiva una forma de resolver un issue. Las sugerencias son buenas, pero debemos ser concisos y claros a la hora de hacer una revisión. comentar los puntos clave y hacer que el resultado de la revisión sea claro y «accionable», es decir, que el desarrollador sepa pros y contras que hacer cuando le han devuelto una revisión.

En mente

Como desarrolladores somos responsables de hacer código mantenible. Esto es fácil de decir pero algo más difícil de cumplir ya que tenemos plazos y presiones. El refactoring no debe cambiar la funcionalidad solo hacer más mantenible el código. Mejorar el código haciéndolo más mantenible es tan importante como arreglar la linea que causó el bug.

Además, lo más importante es mantener la mente abierta durante las revisiones de código. Esto debería y digo debería ser nuestra lucha constante. Ir a una revisión a la defensiva o hacer una revisión defensiva no es bueno para nadie, ni para el código, ni para los desarrolladores, ni para el equipo, ni para la compañía ni para nosotros.

Si el revisor hace una sugerencia y no hay una respuesta clara de porqué no aplicar ese cambio, lo mejor es cambiarlo. Si el revisor hace una pregunta en una linea, esto podría significar que esa linea podría causar dudas en el futuro.

Referencias

Como ya he comentado hay una gran cantidad de post justificando los code review, pero poco sobre como hacerlos, creo que lo mejor es empezar con literatura top: – Clean Code – Refactoring – Pragmatic programmer

Conclusiones

Bajo mi punto de vista estos sería los aspectos a destacar al ahora de hacer revisiones de código, en otros artículos ya hemos tratado temas como la cobertura de código, los principios de diseño y demás. Esto es más que una especie de guía sobre que mirar y en que fijarse a la hora de hacer code reviews. Esto no pretende ser un texto de referencia, sino solo unas simples notas sobre que aspectos no debemos olvidar a la hora de comentar con otro(s) programador(es) cual ha sido la solución a un problema.

Para finalizar, ¿Cómo hacéis las revisiones? ¿añadirías/eliminarías algún punto?