Mejores prácticas de revisión de código

Internet ofrece una gran cantidad de material sobre las revisiones de código: sobre el efecto de las revisiones de código en la cultura de la empresa, sobre las revisiones de seguridad formales, guías más cortas, listas de comprobación más largas, revisiones humanizadas, razones para hacer revisiones de código en primer lugar, mejores prácticas, más mejores prácticas, estadísticas sobre la eficacia de la revisión de código para la captura de errores, y ejemplos de revisiones de código que han salido mal. Ah, y por supuesto también hay libros. En resumen, esta entrada de blog presenta la opinión de Palantir sobre las revisiones de código. Las organizaciones con una profunda reticencia cultural a las revisiones por pares pueden querer consultar el excelente ensayo de Karl E. Wiegers sobre Humanizar las revisiones por pares antes de intentar seguir esta guía.

Este post está copiado de las guías de mejores prácticas de nuestra cadena de herramientas de Calidad de Código Java, Baseline, y cubre los siguientes temas:

• Por qué, qué y cuándo hacer revisiones de código
• Preparación del código para su revisión
• Realización de revisiones de código
• Ejemplos de revisiones de código

Realizamos revisiones de código (RC) para mejorar la calidad del código y beneficiarnos de los efectos positivos en la cultura del equipo y de la empresa. Por ejemplo:

• Los committers están motivados por la noción de un conjunto de revisores que revisarán la solicitud de cambio: el committer tiende a limpiar los cabos sueltos, consolidar los TODOs, y en general mejorar el commit. El reconocimiento de la experiencia de codificación a través de los compañeros es una fuente de orgullo para muchos programadores.
• Compartir el conocimiento ayuda a los equipos de desarrollo de varias maneras:
  – Un CR comunica explícitamente la funcionalidad añadida/alterada/eliminada a los miembros del equipo que posteriormente pueden construir sobre el trabajo realizado.
  – El committer puede utilizar una técnica o algoritmo del que los revisores pueden aprender. En términos más generales, las revisiones de código ayudan a elevar el nivel de calidad en toda la organización.
  – Los revisores pueden poseer conocimientos sobre técnicas de programación o la base de código que pueden ayudar a mejorar o consolidar el cambio; por ejemplo, otra persona puede estar trabajando simultáneamente en una característica o corrección similar.
  – La interacción y la comunicación positivas fortalecen los vínculos sociales entre los miembros del equipo.
• La consistencia en una base de código hace que el código sea más fácil de leer y entender, ayuda a prevenir errores y facilita la colaboración entre las especies de desarrolladores regulares y migratorios.
• La legibilidad de los fragmentos de código es difícil de juzgar para el autor del que es hijo, y fácil de juzgar para un revisor que no tiene el contexto completo. El código legible es más reutilizable, libre de errores y a prueba de futuro.
• Los errores accidentales (por ejemplo, las erratas), así como los errores estructurales (por ejemplo, el código muerto, los fallos de lógica o de algoritmo, los problemas de rendimiento o de arquitectura) suelen ser mucho más fáciles de detectar para los revisores críticos con una perspectiva externa. Los estudios han descubierto que incluso las revisiones de código cortas e informales tienen un impacto significativo en la calidad del código y en la frecuencia de los errores.
• Los entornos de cumplimiento y regulación suelen exigir revisiones. Las RC son una gran manera de evitar las trampas de seguridad comunes. Si su característica o entorno tiene requisitos de seguridad significativos, se beneficiará de (y probablemente requerirá) la revisión por parte de sus cascarrabias de seguridad locales (la guía de OWASP es un buen ejemplo del proceso).

Qué revisar

No hay una respuesta eternamente verdadera a esta pregunta y cada equipo de desarrollo debe acordar su propio enfoque. Algunos equipos prefieren revisar cada cambio fusionado en la rama principal, mientras que otros tendrán un umbral de «trivialidad» por debajo del cual no se requiere una revisión. El equilibrio está entre el uso eficaz del tiempo de los ingenieros (tanto de los autores como de los revisores) y el mantenimiento de la calidad del código. En ciertos entornos normativos, la revisión del código puede ser necesaria incluso para cambios triviales.

Las revisiones de código no tienen clase: ser la persona más veterana del equipo no implica que su código no necesite revisión. Incluso si, en el raro caso, el código es impecable, la revisión proporciona una oportunidad para la tutoría y la colaboración, y, mínimamente, diversifica la comprensión del código en la base de código.

Cuándo revisar

Las revisiones de código deben ocurrir después de que las comprobaciones automatizadas (pruebas, estilo, otros CI) se hayan completado con éxito, pero antes de que el código se fusione con la rama principal del repositorio. Por lo general, no realizamos una revisión formal del código de los cambios agregados desde la última versión.

Para los cambios complejos que deben fusionarse en la rama principal como una sola unidad, pero que son demasiado grandes para encajar en un CR razonable, considere un modelo de CR apilado: Crear una rama principal feature/big-feature y una serie de ramas secundarias (por ejemplo, feature/big-feature-apifeature/big-feature-testing, etc.) que encapsulen cada una un subconjunto de la funcionalidad y que se revisen individualmente contra la rama feature/big-feature. Una vez que todas las ramas secundarias se fusionan en feature/big-feature, crear un CR para la fusión de este último en la rama principal.

Preparación del código para la revisión

Es responsabilidad del autor presentar CRs que sean fáciles de revisar para no perder el tiempo y la motivación de los revisores:

• Alcance y tamaño. Los cambios deben tener un alcance estrecho, bien definido y autocontenido que cubra de forma exhaustiva. Por ejemplo, un cambio puede implementar una nueva característica o corregir un error. Se prefieren los cambios más cortos a los más largos. Si una CR realiza cambios sustanciales en más de ~5 archivos, o ha tardado más de 1-2 días en escribirse, o tardaría más de 20 minutos en revisarse, considere la posibilidad de dividirla en varias CRs autocontenidas. Por ejemplo, un desarrollador puede enviar un cambio que defina la API para una nueva característica en términos de interfaces y documentación, y un segundo cambio que añada implementaciones para esas interfaces.
• Envíe sólo CRs completos, auto-revisados (por diff), y auto-probados. Para ahorrar tiempo a los revisores, pruebe los cambios enviados (es decir, ejecute el conjunto de pruebas) y asegúrese de que pasan todas las compilaciones, así como todas las pruebas y comprobaciones de calidad del código, tanto a nivel local como en los servidores de CI, antes de asignar a los revisores.
• Los cambios de refactorización no deben alterar el comportamiento; a la inversa, los cambios que cambian el comportamiento deben evitar la refactorización y los cambios de formato del código. Hay múltiples buenas razones para esto:
  – Los cambios de refactorización a menudo tocan muchas líneas y archivos y, en consecuencia, serán revisados con menos atención. Los cambios de comportamiento no deseados pueden filtrarse en la base de código sin que nadie se dé cuenta.
  – Los grandes cambios de refactorización rompen el cherry-picking, el rebasing y otras magias de control de código. Es muy oneroso deshacer un cambio de comportamiento que fue introducido como parte de un commit de refactorización en todo el repositorio.
  – El costoso tiempo de revisión humana debe ser gastado en la lógica del programa en lugar de debates de estilo, sintaxis o formato. Preferimos resolverlos con herramientas automatizadas como Checkstyle, TSLint, Baseline, Prettier, etc.

Mensajes de commit

El siguiente es un ejemplo de un buen mensaje de commit siguiendo un estándar ampliamente citado:

Capitalized, short (80 chars or less) summaryMore detailed explanatory text, if necessary. Wrap it to about 120 characters or so. In some contexts, the first
line is treated as the subject of an email and the rest of the text as the body. The blank line separating the
summary from the body is critical (unless you omit the body entirely); tools like rebase can get confused if you run
the two together.Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches
up with commit messages generated by commands like git merge and git revert.Further paragraphs come after blank lines.- Bullet points are okay, too

Trate de describir tanto lo que el commit cambia como la forma en que lo hace:

> BAD. Don't do this.
Make compile again> Good.
Add jcsv dependency to fix IntelliJ compilation

Encontrando revisores

Es habitual que el committer proponga uno o dos revisores que estén familiarizados con el código base. A menudo, uno de los revisores es el líder del proyecto o un ingeniero senior. Los propietarios de proyectos deberían considerar la posibilidad de suscribirse a sus proyectos para recibir notificaciones de nuevos CR. Las revisiones de código entre más de tres partes suelen ser improductivas o incluso contraproducentes, ya que diferentes revisores pueden proponer cambios contradictorios. Esto puede indicar un desacuerdo fundamental sobre la implementación correcta y debería resolverse fuera de una revisión de código en un foro de mayor ancho de banda, por ejemplo en persona o en una videoconferencia con todas las partes implicadas.

Realización de revisiones de código

Una revisión de código es un punto de sincronización entre diferentes miembros del equipo y, por tanto, tiene el potencial de bloquear el progreso. En consecuencia, las revisiones de código deben ser rápidas (del orden de horas, no días), y los miembros del equipo y los líderes deben ser conscientes del compromiso de tiempo y priorizar el tiempo de revisión en consecuencia. Si crees que no puedes completar una revisión a tiempo, házselo saber al committer de inmediato para que pueda encontrar a otra persona.

Una revisión debe ser lo suficientemente exhaustiva como para que el revisor pueda explicar el cambio con un nivel de detalle razonable a otro desarrollador. Esto asegura que los detalles de la base de código son conocidos por más de una sola persona.

Como revisor, es tu responsabilidad hacer cumplir los estándares de codificación y mantener la barra de calidad. Revisar el código es más un arte que una ciencia. La única manera de aprenderlo es haciéndolo; un revisor experimentado debería considerar poner a otros revisores menos experimentados en sus cambios y hacer que ellos hagan una revisión primero. Asumiendo que el autor ha seguido las directrices anteriores (especialmente con respecto a la auto-revisión y asegurar que el código se ejecuta), aquí hay una lista de cosas a las que un revisor debe prestar atención en una revisión de código:

Propósito

• ¿Cumple este código el propósito del autor? Cada cambio debe tener una razón específica (nueva característica, refactorización, corrección de errores, etc). ¿Cumple el código presentado realmente este propósito?
• Haga preguntas. Las funciones y las clases deben existir por una razón. Cuando la razón no está clara para el revisor, esto puede ser una indicación de que el código necesita ser reescrito o apoyado con comentarios o pruebas.

Implementación

• Piensa en cómo habrías resuelto el problema. Si es diferente, ¿a qué se debe? Su código maneja más casos (de borde)? Es más corto/fácil/limpio/rápido/seguro pero funcionalmente equivalente? Hay algún patrón subyacente que haya detectado y que no esté capturado por el código actual?
• ¿Ve potencial para abstracciones útiles? El código parcialmente duplicado a menudo indica que una pieza más abstracta o general de la funcionalidad puede ser extraída y luego reutilizada en diferentes contextos.
• Piense como un adversario, pero sea amable al respecto. Intente «pillar» a los autores que toman atajos o que se saltan los casos presentando configuraciones/datos de entrada problemáticos que rompen su código.
• Piense en las bibliotecas o en el código de los productos existentes. Cuando alguien reimplementa una funcionalidad existente, la mayoría de las veces es simplemente porque no sabe que ya existe. A veces, el código o la funcionalidad se duplican a propósito, por ejemplo, para evitar dependencias. En estos casos, un comentario del código puede aclarar la intención. La funcionalidad introducida ya la proporciona una biblioteca existente?
• ¿Sigue el cambio patrones estándar? Las bases de código establecidas suelen presentar patrones en torno a las convenciones de nomenclatura, la descomposición lógica del programa, las definiciones de tipos de datos, etc. Normalmente es deseable que los cambios se implementen de acuerdo con los patrones existentes.
• ¿Añade el cambio dependencias en tiempo de compilación o de ejecución (especialmente entre subproyectos)? Queremos mantener nuestros productos débilmente acoplados, con el menor número de dependencias posible. Los cambios en las dependencias y en el sistema de compilación deben ser analizados en profundidad.

Legibilidad y estilo

• Piensa en tu experiencia de lectura. ¿Comprendiste los conceptos en un tiempo razonable? ¿Fue el flujo cuerdo y fueron los nombres de las variables y los métodos fáciles de seguir? ¿Fue capaz de seguir la pista a través de múltiples archivos o funciones? ¿Se ha sentido desanimado por la inconsistencia de los nombres?
• ¿El código se adhiere a las directrices de codificación y al estilo del código? Es el código coherente con el proyecto en términos de estilo, convenciones de la API, etc.? Como se mencionó anteriormente, preferimos resolver los debates de estilo con herramientas automatizadas.
• ¿Tiene este código TODOs? Los TODOs sólo se acumulan en el código, y se vuelven rancios con el tiempo. Haz que el autor envíe un ticket en GitHub Issues o JIRA y adjunta el número de incidencia al TODO. El cambio de código propuesto no debe contener código comentado.

Mantenibilidad

• Lee las pruebas. Si no hay pruebas y debería haberlas, pida al autor que escriba algunas. Las características verdaderamente no comprobables son raras, mientras que las implementaciones de características no comprobadas son, por desgracia, comunes. Compruebe las pruebas en sí mismas: ¿cubren casos interesantes? ¿Son legibles? ¿La RC disminuye la cobertura general de las pruebas? Piensa en las formas en que este código podría romperse. Las normas de estilo para las pruebas suelen ser diferentes a las del código del núcleo, pero siguen siendo importantes.
• ¿Introduce esta CR el riesgo de romper el código de pruebas, las pilas de montaje o las pruebas de integración? Estos a menudo no se comprueban como parte de las comprobaciones previas al commit/merge, pero que se caigan es doloroso para todos. Las cosas específicas que hay que buscar son: la eliminación de las utilidades o modos de prueba, los cambios en la configuración y los cambios en la disposición/estructura de los artefactos.
• ¿Este cambio rompe la compatibilidad con versiones anteriores? Si es así, ¿está bien fusionar el cambio en este momento o debe ser empujado a una versión posterior? Las rupturas pueden incluir cambios en la base de datos o en el esquema, cambios en la API pública, cambios en el flujo de trabajo del usuario, etc.
• ¿Este código necesita pruebas de integración? A veces, el código no puede probarse adecuadamente sólo con pruebas unitarias, especialmente si el código interactúa con sistemas o configuraciones externas.
• Deje su opinión sobre la documentación a nivel de código, los comentarios y los mensajes de confirmación. Los comentarios redundantes desordenan el código, y los mensajes de confirmación escuetos desconciertan a los futuros contribuyentes. Esto no siempre es aplicable, pero los comentarios y los mensajes de confirmación de calidad se pagarán por sí mismos en el futuro. (Piensa en una ocasión en la que hayas visto un mensaje de confirmación o un comentario excelente, o realmente terrible). Si su proyecto mantiene un README, CHANGELOG, u otra documentación, ¿se actualizó para reflejar los cambios? La documentación desactualizada puede ser más confusa que ninguna, y será más costoso arreglarla en el futuro que actualizarla ahora.

No olvide elogiar el código conciso/leíble/eficiente/elegante. A la inversa, rechazar o desaprobar un CR no es de mala educación. Si el cambio es redundante o irrelevante, recházalo con una explicación. Si lo consideras inaceptable debido a uno o más defectos fatales, desaprueba, de nuevo con una explicación. A veces el resultado correcto de un CR es «hagamos esto de una manera totalmente diferente» o incluso «no hagamos esto en absoluto».

Sea respetuoso con los revisados. Aunque el pensamiento adversario es útil, no es tu característica y no puedes tomar todas las decisiones. Si no puedes llegar a un acuerdo con tu revisado con el código tal y como está, cambia a la comunicación en tiempo real o busca una tercera opinión.

Seguridad

Verifica que los puntos finales de la API realizan una autorización y autenticación adecuadas y coherentes con el resto de la base de código. Compruebe otras debilidades comunes, por ejemplo, una configuración débil, entradas de usuario maliciosas, eventos de registro que faltan, etc. En caso de duda, remite el RC a un experto en seguridad de aplicaciones.

Comentarios: concisos, amables, procesables

Las críticas deben ser concisas y estar escritas en un lenguaje neutral. Critique el código, no al autor. Cuando algo no esté claro, pida una aclaración en lugar de asumir su ignorancia. Evite los pronombres posesivos, en particular junto con las evaluaciones: «mi código funcionaba antes de tu cambio», «tu método tiene un error», etc. Evita los juicios absolutos: «esto nunca puede funcionar», «el resultado siempre es erróneo».

Intente diferenciar entre sugerencias (por ejemplo, «Sugerencia: extraiga el método para mejorar la legibilidad»), cambios requeridos (por ejemplo, «Añada @Override»), y puntos que necesitan discusión o aclaración (por ejemplo, «¿Es este realmente el comportamiento correcto? Si es así, añade un comentario explicando la lógica»). Considere la posibilidad de proporcionar enlaces o punteros a explicaciones en profundidad de un problema.

Cuando haya terminado con una revisión de código, indique hasta qué punto espera que el autor responda a sus comentarios y si le gustaría volver a revisar el RC después de que se hayan implementado los cambios (por ejemplo, «Siéntase libre de fusionar después de responder a las pocas sugerencias menores» frente a «Por favor, considere mis sugerencias y hágame saber cuándo puedo echar otro vistazo»).

Responder a las revisiones

Parte del propósito de la revisión de código es mejorar la solicitud de cambio del autor; en consecuencia, no se ofenda por las sugerencias de su revisor y tómelas en serio aunque no esté de acuerdo. Responde a todos los comentarios, aunque sea con un simple «ACK» o «done». Explica por qué has tomado ciertas decisiones, por qué existe alguna función, etc. Si no puedes llegar a un acuerdo con el revisor, cambia a la comunicación en tiempo real o busca una opinión externa.

Las correcciones deben ser empujadas a la misma rama, pero en un commit separado. Aplastar los commits durante el proceso de revisión dificulta el seguimiento de los cambios por parte del revisor.

Diferentes equipos tienen diferentes políticas de merge: algunos equipos sólo permiten a los propietarios del proyecto hacer merge, mientras que otros equipos permiten al colaborador hacer merge después de una revisión de código positiva.

Revisiones de código en persona

Para la mayoría de las revisiones de código, las herramientas asíncronas basadas en diff como Reviewable, Gerrit o, GitHub son una gran opción. Los cambios complejos, o las revisiones entre partes con conocimientos o experiencia muy diferentes pueden ser más eficientes cuando se realizan en persona, ya sea frente a la misma pantalla o proyector, o de forma remota a través de VTC o herramientas de pantalla compartida.

Ejemplos

En los siguientes ejemplos, los comentarios de revisión sugeridos se indican mediante //R: ... comentarios en los bloques de código.

Nombres inconsistentes

class MyClass {
private int countTotalPageVisits; //R: name variables consistently
private int uniqueUsersCount;
}

Firmas de métodos inconsistentes

interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<String> extractString(String s); /** Returns null if {@code s} cannot be rewritten. */
//R: should harmonize return values: use Optional<> here, too
public String rewriteString(String s);
}

Uso de bibliotecas

//R: remove and replace by Guava's MapJoiner
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

Gusto personal

int dayCount; //R: nit: I usually prefer numFoo over fooCount; up to you, but we should keep it consistent in this project

Bugs

//R: This performs numIterations+1 iterations, is that intentional?
// If it is, consider changing the numIterations semantics?
for (int i = 0; i <= numIterations; ++i) {
...
}

Preocupaciones arquitectónicas

otherService.call(); //R: I think we should avoid the dependency on OtherService. Can we discuss this in person?