Revisar antes o después de confirmar el código, ¿cuál es mejor?

Me gusta Simon Whitehead menciona en su comentario , depende de tu estrategia de ramificación.

Si los desarrolladores tienen su propia sucursal privada para el desarrollo (que de todos modos recomendaría en la mayoría de las situaciones), realizaría la revisión del código antes de fusionarme con el tronco o el repositorio principal. Esto permitirá a los desarrolladores tener la libertad de registrarse con la frecuencia que quieran durante su ciclo de desarrollo / prueba, pero cada vez que el código entra en la rama que contiene el código entregado, se revisa.

En general, sus malas experiencias con las revisiones de códigos parecen más bien un problema con el proceso de revisión que tiene soluciones. Al revisar el código en fragmentos más pequeños e individuales, puede asegurarse de que no demore demasiado. Un buen número es que se pueden revisar 150 líneas de código en una hora, pero la velocidad será más lenta para las personas que no están familiarizadas con el lenguaje de programación, el sistema en desarrollo o la criticidad del sistema (una seguridad crítica requiere más tiempo) esta información puede ser útil para mejorar la eficiencia y decidir quién participa en las revisiones de códigos.

Hay un mantra que nadie parece haber citado todavía: Regístrese temprano y con frecuencia :

  

Los desarrolladores que trabajan por largos períodos de tiempo, y por mucho tiempo me refiero a más de un día, sin controlar el control de la fuente, se están preparando para algunos dolores de cabeza de integración graves en el futuro. Damon Poole concurs :

     

    

Los desarrolladores a menudo postergan el proceso de registro. Lo postergan porque no quieren afectar a otras personas demasiado pronto y no quieren que se les culpe por romper la construcción. Pero esto lleva a otros problemas, como perder el trabajo o no poder volver a las versiones anteriores.

         

Mi regla de oro es el "check-in temprano y con frecuencia", pero con la advertencia de que tiene acceso a las versiones privadas. Si un check-in es visible inmediatamente para otros usuarios, entonces corre el riesgo de introducir cambios inmaduros y / o romper la construcción.

  

     

Prefiero que se revisen los pequeños fragmentos periódicamente que pasar largos periodos sin tener idea de lo que escriben mis compañeros de trabajo. En lo que a mí respecta, si el código no está incluido en el control de código fuente, no existe . Supongo que esta es otra forma de Don't Go Dark ; el código es invisible hasta que existe en el repositorio de alguna forma.

     

... Si aprende a registrarse temprano y a menudo, tendrá tiempo suficiente para comentarios, integración y revisiones en el camino ...

He trabajado para un par de compañías que tenían diferentes enfoques hacia esto. Uno lo permitió, siempre y cuando no impidiera la compilación. El otro se enloquecería si verificara algún error. El primero es mucho más preferido. Debería estar desarrollándose en un tipo de entorno que le permita colaborar con otras personas en cosas que aún están en progreso, con el entendimiento de que todo se probará más adelante.

También hay una declaración de Jeff Atwood: No seas miedo de romper cosas :

  

La forma más directa de mejorar como desarrollador de software es ser absolutamente intrépido cuando se trata de cambiar tu código. Los desarrolladores que temen a los códigos rotos son desarrolladores que nunca se convertirán en profesionales.

También agregaría que para las revisiones de pares, si alguien quiere que cambies algo, tener la historia de tu versión original en origen es una herramienta de aprendizaje muy valiosa.

Recientemente comencé a realizar revisiones previas a un compromiso en un proyecto en el que estoy y debo decir que estoy gratamente sorprendido de lo problemático que es.

El mayor inconveniente de las revisiones post-commit que veo es que a menudo es un asunto solo para una sola persona: Alguien revisa el código para ver si está correcto, pero el autor no suele estar involucrado a menos que haya un grave error. Los pequeños problemas, sugerencias o sugerencias generalmente no llegan al autor en absoluto.

Esto también cambia el resultado percibido de las revisiones del código: se ve como algo que solo produce trabajo adicional, en lugar de algo en el que todos (el revisor y el autor del código) pueden aprender cosas nuevas cada vez.

Las revisiones de código de pre-confirmación parecen tan naturales, nunca se me ocurrió que las revisiones pudieran realizarse deliberadamente después de la confirmación. Desde una perspectiva de integración continua, desea confirmar su código una vez que esté terminado, no en un estado de trabajo pero posiblemente mal escrito, ¿no?

Tal vez se deba a que la forma en que siempre lo hemos hecho en mis equipos es mediante el diálogo en vivo iniciado por el desarrollador original, sin embargo, las revisiones basadas en documentos asincrónicas y controladas por el control.

La mayoría de los repositorios de hoy en día admiten un compromiso de dos fases o un conjunto de líneas (sucursal privada, solicitud de extracción, envío de parches o como quiera llamarlo), que le permitirá inspeccionar / revisar el trabajo antes de llegar a la línea principal. Diría que aprovechar estas herramientas le permitiría realizar siempre revisiones previas.

También, podría considerar la codificación de pares (pares senior con junior) como otra forma de proporcionar una revisión de código incorporada. Considérelo como una inspección de calidad en la línea de ensamblaje en lugar de después de que el automóvil haya rodado.

Haz ambas cosas:

• compromiso previo: realice este tipo de revisiones cuando sea algo muy importante, como una pieza de código muy reutilizable o una decisión de diseño importante
• enviar confirmación: haga este tipo de revisiones cuando desee obtener una opinión sobre un código que podría mejorarse

Cualquier revisión formal debe realizarse en los archivos de origen que están bajo el control de la configuración, y los registros de la revisión indican claramente la revisión del archivo.

Esto evita los argumentos de tipo "no tienes el archivo más reciente" y garantiza que todos estén revisando la misma copia del código fuente.

También significa que, si se requieren correcciones posteriores a la revisión, el historial se puede anotar con ese hecho.

Para la revisión del código en sí, mi voto es para 'durante' el compromiso.

Un sistema como gerrit o clover (creo) puede realizar un cambio y luego hacer que el revisor lo asigne al control de fuente (push git) si es bueno. Eso es lo mejor de ambos mundos.

Si eso no es práctico, creo que después de comprometerse es el mejor compromiso. Si el diseño es bueno, entonces solo los desarrolladores más jóvenes deberían tener las cosas lo suficientemente malas como para que no se comprometan nunca. (Haga una revisión previa a la confirmación para ellos).

Lo que lleva a la revisión del diseño, aunque puede hacerlo en el momento de la revisión del código (o, en ese caso, en el momento de la implementación del cliente), los problemas de diseño deben realizarse antes de eso, antes de que se escriba el código.

Con la revisión por pares, existe un riesgo mínimo de perder el control. Todo el tiempo dos personas tienen conocimiento sobre el mismo código. Tienen que cambiar de vez en cuando, por lo que deben estar atentos todo el tiempo para realizar un seguimiento del código.

Tiene sentido tener un desarrollador hábil y un novato trabajando juntos. A primera vista, esto parece ser ineficiente, pero no lo es. De hecho, hay menos errores y se necesita menos tiempo para solucionarlos. Además, los novatos aprenderán mucho más rápido.

En lo que se refiere a prevenir un mal diseño, esto debe hacerse antes de codificar. Si hay algún cambio / mejora / pieza de diseño considerable, debe revisarse antes de que comience la codificación. Cuando el diseño está completamente desarrollado, no queda mucho por hacer. Revisar el código será más fácil y tomará menos tiempo.

Estoy de acuerdo en que no es esencial revisar el código antes de comprometerse solo si el código es producido por un desarrollador experimentado, que ya ha demostrado sus habilidades. Pero si hay un novato, el código debe revisarse antes de comprometerse: el revisor debe sentarse junto al desarrollador y explicar cada cambio o mejora realizada por ellos.

Las revisiones se benefician de los compromisos previos y posteriores.

Confirmación previa a la revisión

• Da a los revisores la confianza de que están revisando la última revisión del autor.
• Ayuda a asegurar que todos revisen el mismo código.
• Proporciona una referencia para comparación una vez que se completan las revisiones realizadas desde los elementos de revisión.

Sin compromisos en ejecución durante la revisión

He utilizado las herramientas de Atlassian y he visto que se ejecutan confirmaciones durante la revisión. Esto es confuso para los revisores, por lo que lo recomiendo.

Revisiones posteriores a la revisión

Después de que los revisores completen sus comentarios, verbalmente o por escrito, el moderador debe asegurarse de que el autor realice los cambios solicitados. A veces, los revisores o el autor pueden estar en desacuerdo sobre si designar un artículo de revisión como un fallo, una sugerencia o una investigación. Para resolver los desacuerdos y garantizar que los elementos de la investigación se borran correctamente, el equipo de revisión depende del criterio del moderador.

Mi experiencia con alrededor de 100 inspecciones de código es que cuando los revisores pueden hacer referencia a un estándar de codificación no ambiguo, y para la mayoría de los tipos de lógica y otras fallas de programación, los resultados de la revisión son generalmente claros. De vez en cuando hay un debate sobre la selección de liendres o un punto de estilo puede degenerar en discusión. Sin embargo, dar poder de decisión al moderador evita el estancamiento.

Compromiso posterior a la revisión

• Le da al moderador y a otros revisores un punto de datos para comparar con el compromiso de revisión previa.
• Proporciona métricas para juzgar tanto el valor como el éxito de la revisión en la eliminación de defectos y la mejora del código.

Depende de tu equipo de maquillaje. Para un equipo relativamente experimentado que sea bueno con las confirmaciones pequeñas y frecuentes, luego la revisión posterior a la confirmación solo para obtener un segundo par de ojos en el código está bien. Para compromisos más grandes y complejos y / o para desarrolladores menos experimentados, las revisiones previas a la confirmación para solucionar problemas antes de que aparezcan parecen más prudentes.

En ese sentido, tener un buen proceso de IC y / o registros de entrada cerrados reduce la necesidad de revisiones antes de cometer (y, posiblemente, publicar cometer para muchos de ellos).

Mis colegas y yo hicimos una investigación científica sobre este tema recientemente, así que me gustaría agregar algunas de nuestras ideas a pesar de que esta es una pregunta bastante antigua. Construimos un modelo de simulación de un proceso / equipo de desarrollo de Kanban ágil y comparamos la revisión previa y posterior a la confirmación de un gran número de situaciones diferentes (número diferente de miembros del equipo, diferentes niveles de habilidades, ...). Observamos los resultados después de 3 años de tiempo de desarrollo (simulado), y buscamos las diferencias con respecto a la eficiencia (puntos de historia terminados), la calidad (errores encontrados por los clientes) y el tiempo de ciclo (tiempo desde el inicio hasta la entrega de una historia de usuario) . Nuestros resultados son los siguientes:

• Las diferencias en términos de eficiencia y calidad son insignificantes en muchos casos. Cuando no lo son, la revisión posterior a la confirmación tiene algunos beneficios con respecto a la calidad (otros desarrolladores actúan como "beta-testers" de alguna manera). Para mayor eficiencia, post commit tiene algunos beneficios en equipos pequeños y pre commit tiene algunos beneficios en equipos grandes o no calificados.
• La revisión previa de confirmación puede llevar a tiempos de ciclo más largos, cuando se presenta una situación en la que la revisión demora el inicio de las tareas dependientes.

De estos, derivamos las siguientes reglas heurísticas:

• Cuando tiene un proceso de revisión de código establecido, no se moleste en cambiar, probablemente no valga la pena el esfuerzo
  • a menos que tenga problemas con el tiempo de ciclo = > Cambiar a Publicar
   
  • O los problemas ocultos interrumpen a los desarrolladores con demasiada frecuencia = > Cambiar a Pre
• Si aún no estás haciendo comentarios
  • Utilice la confirmación previa si alguno de estos beneficios es aplicable para usted
    • La revisión previa a la confirmación permite que personas externas sin derechos de confirmación contribuyan a proyectos de código abierto
    • Si se basa en la herramienta, la revisión previa a la ejecución de compromisos impone cierta disciplina de revisión en equipos con una adhesión por lo demás poco estricta del proceso
    • La revisión previa a la confirmación evita que se entreguen los cambios no revisados, lo cual es bueno para una implementación continua / ciclos de lanzamiento muy cortos
  • Use pre commit si su equipo es grande y puede vivir con o eludir los problemas en el tiempo de ciclo
  • De lo contrario (por ejemplo, un equipo industrial pequeño y capacitado) use post commit
• Busque combinaciones que le brinden los beneficios de ambos mundos (no los hemos estudiado formalmente)

El documento de investigación completo está disponible aquí: enlace o en mi sitio web: enlace

En mi opinión, la revisión por pares de código funciona mejor si se realiza después de la confirmación.

Recomendaría ajustar su estrategia de bifurcación. El uso de una rama de desarrollador o una rama de funciones tiene una serie de beneficios ... de los cuales no menos importante es facilitar las revisiones de códigos posteriores a la confirmación.

Una herramienta como Crucible suavizará y automatizará el proceso de revisión. Puede seleccionar uno o más conjuntos de cambios confirmados para incluir en la revisión. Crucible mostrará qué archivos se tocaron en los conjuntos de cambios seleccionados, realizar un seguimiento de qué archivos ya ha leído cada revisor (mostrando un% completo en general) y permitir que los revisores hagan comentarios fácilmente.

enlace

Algunos otros beneficios de las ramas de usuario / características:

• Los desarrolladores obtienen los beneficios del control de versiones (hacer una copia de seguridad de los cambios, restaurar desde el historial, cambiar los cambios) con menos preocupación de romper el sistema para todos los demás.
• Los cambios que causan defectos o que no se terminan a tiempo se pueden revertir, priorizar o anular si es necesario.

Para desarrolladores sin experiencia, una consulta regular con un mentor y / o programación en pareja es una buena idea, pero no lo consideraría una "revisión de código".

Ambos. (Tipo de.)

Debe revisar su propio código de manera resumida antes de cometerlo. En Git, creo que el área de preparación es excelente. Después de haber configurado mis cambios, ejecuto git diff --cached para ver todo lo que está en escena. Utilizo esto como una oportunidad para asegurarme de que no estoy revisando ningún archivo que no pertenezca (compile artefactos, registros, etc.) y asegurándome de que no tenga ningún código de depuración allí o ningún código importante comentado afuera. (Si estoy haciendo algo que sé que no quiero registrar, generalmente dejo un comentario en mayúsculas para que lo reconozca durante la puesta en escena).

Habiendo dicho eso, su revisión del código de pares se debe realizar generalmente después de la confirmación, suponiendo que está trabajando en una rama temática. Esta es la forma más fácil de asegurarse de que todos los demás estén revisando lo correcto, y si hay problemas importantes, entonces no es gran cosa arreglarlos en su sucursal o eliminarlos y comenzar de nuevo. Si realiza revisiones de código de forma asíncrona (es decir, utiliza Google Code o Atlassian Crucible), puede cambiar fácilmente de sucursal y trabajar en otra cosa sin tener que realizar un seguimiento por separado de todos los diferentes parches / diferencias que se encuentran actualmente bajo revisión durante unos días.

Si no estás trabajando en una rama temática, deberías . Reduce el estrés y la molestia y hace que la planificación de lanzamientos sea mucho menos estresante y complicada.

Editar: También debería agregar que debería hacer una revisión de código después de la prueba, que es otro argumento a favor de cometer código primero. No querrás que tu grupo de prueba juegue con docenas de parches / diferencias de todos los programadores, luego archiva errores simplemente porque aplicaron el parche incorrecto en el lugar equivocado.

Programación emparejada al 100% (sin importar qué tan avanzado creas) con muchos compromisos pequeños y un sistema de CI que se basa en CADA compromiso (con pruebas automatizadas que incluyen unidades, integración y funcional siempre que sea posible). Revisiones post-commit para cambios grandes o riesgosos. Si debe tener algún tipo de revisión cerrada / previa al compromiso, Gerrit funciona.

La ventaja de la revisión del código en el registro (verificación de amigos) es la retroalimentación inmediata, antes de que se completen grandes partes del código.

La desventaja de la revisión del código en el proceso de registro es que puede desalentar a las personas a registrarse hasta que se completen largos tramos de código. Si esto sucede, niega completamente la ventaja.

Lo que hace que esto sea más difícil es que no todos los desarrolladores son iguales. Las soluciones simples no funcionan para todos los programadores . Las soluciones simples son:

• Programación de pares obligatoria, que permite realizar registros frecuentes porque el amigo está justo a tu lado. Esto ignora que la programación de pares no siempre funciona para todos. Hecho correctamente, la programación de pares también puede ser realmente agotadora, por lo que no es necesariamente algo que hacer todo el día.

• Ramas de desarrollador, el código solo se revisa y verifica en la rama principal cuando termina. Algunos desarrolladores tienden a trabajar en secreto, y después de una semana se les ocurre un código que puede o no pasar la revisión debido a problemas fundamentales que podrían haberse detectado anteriormente.

• Revise cada registro de entrada, lo que garantiza revisiones frecuentes. Algunos desarrolladores se olvidan y confían en los registros frecuentes, lo que significa que otros tienen que hacer revisiones de código cada 15 minutos.

• Revise en algún momento no especificado después del check-in. Las revisiones se eliminarán aún más cuando haya una crisis de fecha límite. Se confirmará el código que depende de un código ya confirmado pero aún no revisado. Las revisiones marcarán los problemas y los problemas se colocarán en el trabajo pendiente para que se solucione "más adelante". Ok, mentí: Esta no es una solución simple, no es una solución en absoluto. Revise en algún momento específico después de que funcione el check-in, pero es menos simple porque tiene que decidir cuál es el tiempo especificado

En la práctica, haces que esto funcione al hacerlo más simple y complejo al mismo tiempo. Usted establece pautas simples y deja que cada equipo de desarrollo determine en equipo lo que deben hacer para seguir estas pautas. Un ejemplo de tales directrices es:

• El trabajo se divide en tareas que deberían tomar menos de un día.
• Una tarea no se termina si el código (si lo hubiera) no se ha registrado.
• Una tarea no se termina si el código (si lo hubiera) no se ha revisado.

Son posibles muchas formas alternativas de tales pautas. Concéntrese en lo que realmente desea (código revisado por pares, progreso de trabajo observable, responsabilidad) y deje que el equipo descubra cómo pueden darle lo que quieren.

En realidad hacemos un híbrido en LedgerSMB. Los comités comprometen cambios que son revisados después. Los que no son comprometidos envían cambios a los comentaristas para que sean revisados antes. Esto tiende a significar dos niveles de revisión. Primero tienes un mentor para que te revise y te guíe. Luego, a ese mentor se le revisa el código una segunda vez después de que él o ella lo hayan firmado y circulen los comentarios. Los nuevos usuarios suelen dedicar mucho tiempo a revisar los compromisos de otras personas al principio.

Funciona bastante bien. Sin embargo, el tema es que una revisión posterior suele ser más superficial que una revisión anterior, por lo que debe asegurarse de que la revisión posterior esté reservada para aquellos que han demostrado su valía. Pero si tiene una revisión de dos niveles para la nueva gente, significa que es más probable que se detecten los problemas y que haya discusiones.

¿Cometer dónde? Hay una rama que creé para hacer un trabajo. Me comprometo con esa rama siempre que tengo ganas. No es asunto de nadie. Luego, en algún punto, esa rama se integra en una rama de desarrollo. Y en algún punto intermedio hay una revisión de código.

El revisor revisa, obviamente, después me comprometí con mi sucursal. No está sentado en mi escritorio, por lo que no puede revisarlo antes de comprometerme con mi . Y revisa antes la fusión y el compromiso con la rama de desarrollo.

Simplemente no hagas revisiones de código en absoluto. O crees que tus desarrolladores son capaces de escribir un buen código, o debes deshacerte de ellos. Los errores en la lógica deben ser atrapados por sus pruebas automatizadas. Los errores en el estilo deben ser atrapados por las herramientas de análisis de pelusas y estáticas.

Tener humanos involucrados en lo que deberían ser procesos automáticos es un desperdicio.