He estado interesado en las preguntas de revisión de código durante mucho tiempo. Muchas veces surgió uno u otro problema, ya sea con la calidad del código o con el clima del equipo. De hecho, la revisión de código es, si no el único, uno de los lugares más importantes para conflictos en el equipo de desarrollo.
Y recientemente, en preparación para el próximo lanzamiento del podcast Zinc Prod , descubrí que Google ha publicado una guía de revisión de código llena de ideas valiosas. Todo el material es bastante voluminoso y no cabe en un artículo, por lo que intentaré resaltar los pensamientos más interesantes (para mí).
Así que vamos
Términos utilizados en el artículo original:
CL - lista de cambios. Pero generalmente se llama Solicitud de fusión o Solicitud de extracción, por lo que en el artículo usaré la abreviatura MR
LGTM : se ve bien para mí. En resumen, cuando presionan el botón "aprobar". Usaré el término "aprov", como más comprensible para la población.
Idealidad mr
En la práctica, uno puede encontrarse con varios extremos al considerar la RM. Alguien como un equipo completo comenta zadalivaet, hasta que todo se haya jugado al punto, se corrige, alguien mira la lógica y presiona "upruv".
Un pensamiento interesante está escrito en el documento de Google. MR no debería ser perfecto, pero debería mejorar la base del código. Es decir, con cada cambio introducido, el código debería mejorar cada vez más. Y si MR agrega mucho bien, entonces no tienes que encontrar fallas en las pequeñas cosas; es más rentable obtener esta mejora más rápido.
Ningún MR debería empeorar el código. La única excepción es si MR es una solución urgente para algo.
Libertad para comentar
A pesar de que no puedes deshacerte de las bagatelas, puedes escribirlas libremente al menos en cada línea. La contradicción con el párrafo anterior se resuelve simplemente: el revisor prefija las pequeñeces y la selección de liendres con el prefijo "nit:" del inglés. nitpick (nitpicking). No es necesario corregir tales comentarios, sin embargo, el autor de MR puede querer corregir algo o, incluso si no, tener en cuenta algunos puntos para el futuro.
Hechos sobre preferencias personales
Casi siempre, los principios de diseño de software estándar, basados en las mejores prácticas, son mejores que las bicicletas difíciles. Por lo tanto, se les debe dar preferencia.
Si puede aplicar varios enfoques estándar, esto queda a discreción del autor.
Presupuesto directo para una mejor comprensión:
Los aspectos del diseño de software casi nunca son una cuestión de estilo puro o simplemente una preferencia personal. Se basan en principios subyacentes y deben sopesarse en esos principios, no simplemente por opinión personal. A veces hay algunas opciones válidas. Si el autor puede demostrar (ya sea a través de datos o basado en principios de ingeniería sólidos) que varios enfoques son igualmente válidos, entonces el revisor debe aceptar la preferencia del autor. De lo contrario, la elección está dictada por los principios estándar del diseño de software.
Si el revisor y el autor de MR no están de acuerdo
Primero viene un intento de llegar a un consenso en los comentarios sobre MR. Si esto no funciona, entonces una discusión personal. Si aún no llega a un consenso, entonces atraiga a los miembros del equipo. Pero lo más importante, la MR no debe estar atascada por mucho tiempo debido al desacuerdo de dos personas.
Discutido en comentarios -> Discutido personalmente -> Discutido en un equipo -> Continuando
MR comprobar velocidad
La velocidad es extremadamente importante. Si hace un comentario cada pocos días, entonces el autor de MR se quejará de que le encuentran fallas y le impiden trabajar.
Si MR se verifica muy rápidamente, cualquier comentario no será un gran problema; después de todo, sus soluciones se verificarán aquí y la tarea continuará.
Google le aconseja que no se distraiga de concentrarse en su tarea, pero si está distraído, vea si hay algo que revisar. Por ejemplo, regresó del almuerzo, revisó. Llegó al trabajo - revisado. Etc.
Orden de visualización de MR
Primero debes mirar el MR como un todo. ¿Es necesario en absoluto? ¿Está el código en el lugar correcto (o debería estar en una biblioteca separada)? ¿Hay algún problema global?
Es decir no tiene sentido mirar algunos detalles de implementación si el código no está allí y no está en absoluto.
En general, el orden de visualización es importante para dar retroalimentación al autor lo antes posible (ver el párrafo anterior).
Cuando miró a MR en su conjunto, debe revisar los archivos principales, es decir marque lo más importante (si no está claro qué es lo más importante, puede preguntar al desarrollador).
Una vez más, cualquier problema debe informarse de inmediato, incluso si aún no ha inspeccionado el MR y ha decidido inspeccionarlo en general más tarde.
A continuación, debe seleccionar una secuencia lógica para ver los archivos restantes. No se debe omitir ningún archivo.
Qué buscar al ver
- El código está bien diseñado.
- La funcionalidad está bien hecha desde el punto de vista de los usuarios de este código, sin importar quiénes sean.
- La apariencia (si la hay) debe ser buena
- Se tienen en cuenta todos los matices de la programación paralela (si existe).
- Código no rediseñado
- El desarrollador no trabaja demasiado: no necesita escribir código que pueda ser necesario o que no sea necesario
- El código tiene pruebas
- Las pruebas están bien diseñadas
- Los nombres (para todo) están bien seleccionados
- Los comentarios sobre el código son comprensibles y necesarios. Deben explicar por qué se hace esto y no cómo se hace.
- Documentación agregada.
- El código corresponde a las guías de estilo.
Demasiado grande señor
Se debe solicitar que los MR demasiado grandes se rompan. Casi siempre es posible.
Cómo escribir comentarios sobre la revisión de código
- Debes ser cortés, no ser personal. Discuta el código, no el codificador.
- No solo emita directivas para las correcciones, sino que explique por qué necesita corregirlas.
- Mantenga un equilibrio: identifique el problema y empuje al desarrollador para que él mismo comprenda la mejor manera de resolverlo; o emita inmediatamente una solución preparada. El primero desarrolla al desarrollador (beneficio estratégico), el segundo mejora y acelera el MR (beneficio táctico).
- Si el revisor no entendió en algún momento del código y le pide al autor que explique qué es qué, entonces la mejor respuesta sería cambiar el código. Para que todo quedara claro en el código sin dudas.
Si no estás de acuerdo con tu opinión
Es necesario entender en detalle. Quizás no entiendas algo, solicita más información. Quizás lo contrario. En cualquier caso, a menudo la comprensión llega solo después de un par de rondas de explicaciones en ambos lados.
Miedo a molestar al autor MR
Sucede que el desarrollador está molesto si el revisor insiste en algunos cambios. Sin embargo, la mayoría de las veces los desarrolladores están molestos por la forma del comentario y no por el contenido. Sea cortés, explique con argumentos, y muy probablemente todo estará bien.
"Lo arreglaré más tarde".
Si el desarrollador acepta que hay un problema en el código, pero solicita MR, prometiendo que lo solucionará en otro momento, entonces, en opinión de los chicos de Google, este "posterior" casi nunca ocurre. Por lo tanto, si MR no es una corrección de errores urgente, entonces debe insistir en una solución.
Conclusiones
Realmente me gustó este documento de Google. Especialmente el truco de la vida con la palabra "nit", el énfasis en la velocidad de revisión de código, y también que MR no debe ser perfecto. Parece ser simple, pero mientras esto claramente no se considere, no llega al punto.
Si este artículo parece interesante para los lectores y recoge una serie de ventajas, escribiré la siguiente parte: el proceso de revisión del código por el autor MR.
Actualización: la segunda parte del artículo aquí
Además, en el próximo número de Zinc Sale, discutiremos en detalle el código de revisión desde diferentes ángulos. ¡Así que asegúrese de suscribirse a nuestro podcast de desarrollo!