Tradicionalmente, realizamos la revisión del código antes de la confirmación, hoy tuve una discusión con mi colega, que prefería la revisión del código después de la confirmación.
Primero, aquí hay algunos antecedentes,
- Tenemos algunos desarrolladores experimentados y también tenemos nuevas contrataciones con casi cero experiencia en programación.
- Nos gustaría realizar iteraciones rápidas y cortas para lanzar nuestro producto.
- Todos los miembros del equipo se encuentran en el mismo sitio.
Las ventajas de la revisión de código antes de confirmar que he aprendido:
- Mentor de nuevas contrataciones
- Trate de evitar errores, fallas, malos diseños al inicio del ciclo de desarrollo.
- Aprende de los demás
- Copia de seguridad del conocimiento si alguien renuncia
Pero también he tenido algunas malas experiencias:
- Baja eficiencia, algunos cambios pueden revisarse durante días
- Difícil de equilibrar velocidad y calidad, especialmente para novatos
- Un miembro del equipo sintió desconfianza
En cuanto a la revisión posterior a la confirmación, sé poco sobre esto, pero lo que más me preocupa es el riesgo de perder el control debido a la falta de revisión. Alguna opinión?
ACTUALIZAR:
- Estamos usando Perforce para VCS
- Codificamos y confirmamos en las mismas ramas (troncales o ramas de corrección de errores)
- Para mejorar la eficiencia, hemos tratado de dividir el código en pequeños cambios. También probamos algunas revisiones de diálogos en vivo, pero no todos siguieron la regla. Sin embargo, este es otro problema.
code-reviews
quinto
fuente
fuente
Respuestas:
Como Simon Whitehead menciona en su comentario , depende de su estrategia de ramificación.
Si los desarrolladores tienen su propia rama privada para el desarrollo (que recomendaría en la mayoría de las situaciones de todos modos), realizaría la revisión del código antes de fusionarlo con el tronco o el repositorio principal. Esto permitirá a los desarrolladores tener la libertad de registrarse con la frecuencia que deseen 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ódigo suenan más como un problema con el proceso de revisión que tienen soluciones. Al revisar el código en fragmentos individuales más pequeños, puede asegurarse de que no tome demasiado tiempo. Un buen número es que se pueden revisar 150 líneas de código en una hora, pero la tasa 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 (un crítico de seguridad requiere más tiempo). Esta información puede ser útil para mejorar la eficiencia y decidir quién participa en las revisiones de código.
fuente
Hay un mantra que nadie parece haber citado todavía: verifique temprano y con frecuencia :
He trabajado para un par de compañías que tenían diferentes enfoques para esto. Uno lo permitió, siempre que no impidiera la compilación. El otro se asustaría si registraras algún error. El primero es muy preferido. Debería desarrollarse en un tipo de entorno que le permita colaborar con otras personas en cosas que todavía están en progreso, con el entendimiento de que todo se probará más adelante.
También está la declaración de Jeff Atwood: no tengas miedo de romper cosas :
También agregaría que para las revisiones por pares, si alguien quiere que cambie algo, tener el historial de su versión original en la fuente es una herramienta de aprendizaje muy valiosa.
fuente
Recientemente comencé a hacer revisiones previas al compromiso en un proyecto en el que estoy y debo decir que estoy gratamente sorprendido de lo poco problemático que es.
El mayor inconveniente de las revisiones posteriores a la confirmación que veo es que a menudo es un asunto solo para una sola persona: alguien revisa el código para verificar que sea correcto, pero el autor generalmente no está involucrado a menos que haya un error grave. Pequeños problemas, sugerencias o sugerencias no suelen llegar al autor en absoluto.
Esto también cambia el resultado percibido de las revisiones de código: se ve como algo que solo produce trabajo adicional, a diferencia de algo en el que todos (el revisor y el autor del código) pueden aprender cosas nuevas cada vez.
fuente
Las revisiones de código previas a la confirmación parecen tan naturales que 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, ¿verdad?
Tal vez sea porque la forma en que siempre lo hemos hecho en mis equipos es un diálogo en vivo iniciado por el desarrollador original, sin embargo, no revisiones asincrónicas, basadas en documentos y basadas en controles.
fuente
La mayoría de los repositorios de hoy admiten una confirmación de dos fases o un conjunto de estanterías (rama privada, solicitud de extracción, envío de parches o como quiera llamarlo), que le permitirá inspeccionar / revisar el trabajo antes de colocarlo en la línea principal. Diría que aprovechar estas herramientas le permitiría hacer siempre revisiones previas.
Además, puede 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 montaje en lugar de después de que el automóvil haya salido.
fuente
Haz ambos :
fuente
Cualquier revisión formal debe llevarse a cabo en los archivos de origen que están bajo control de configuración, y los registros de revisión indican claramente la revisión del archivo.
Esto evita cualquier argumento de tipo "no tiene 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 puede ser anotado con ese hecho.
fuente
Para la revisión del código en sí, mi voto es por 'durante' la confirmación.
Un sistema como el gerrit o el trébol (creo) puede organizar un cambio y luego hacer que el revisor lo confirme en el control de la fuente (push in git) si es bueno. Eso es lo mejor de ambos mundos.
Si eso no es práctico, creo que después de cometer es el mejor compromiso. Si el diseño es bueno, entonces solo la mayoría de los desarrolladores junior deberían tener las cosas lo suficientemente malas como para no querer que se comprometan nunca. (Haga una revisión previa al compromiso para ellos).
Lo que conduce a la revisión del diseño, aunque puede hacerlo en el momento de la revisión del código (o, en ese momento, en el momento de la implementación del cliente), la búsqueda de problemas de diseño debe hacerse antes de eso, antes de que el código se escriba realmente.
fuente
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 ocasionalmente, 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 lleva menos tiempo solucionarlos. Además, los novatos aprenderán mucho más rápido.
Lo que viene a prevenir un mal diseño, esto debe hacerse antes de la codificación. Si hay algún cambio / mejora considerable / nueva pieza de diseño, 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 al lado del desarrollador y explicar cada cambio o mejora realizada por ellos.
fuente
Las revisiones se benefician de los compromisos previos y posteriores.
Compromiso previo a la revisión
Sin compromisos de 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 recomiendo no hacerlo.
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 elemento de revisión como una falla, sugerencia o investigación. Para resolver desacuerdos y garantizar que los elementos de investigación se borren correctamente, el equipo de revisión depende del juicio 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 inequívoco, y para la mayoría de los tipos de fallas lógicas y de programación, los resultados de la revisión son generalmente claros. Ocasionalmente hay un debate sobre la selección de liendres o un punto de estilo puede degenerar en argumento. Sin embargo, dar poder de decisión al moderador evita el estancamiento.
Compromiso posterior a la revisión
fuente
Depende de la composición de tu equipo. Para un equipo relativamente experimentado que es bueno con los pequeños compromisos frecuentes y luego la revisión posterior al compromiso solo para tener un segundo par de ojos en el código, está bien. Para las confirmaciones más grandes y complejas y / o para los desarrolladores menos experimentados, las revisiones previas a la confirmación para solucionar los problemas antes de que entren parezcan más prudentes.
En ese sentido, tener un buen proceso de CI y / o registros cerrados disminuye la necesidad de revisiones antes de la confirmación (y posiblemente post confirmación para muchas de ellas).
fuente
Mis colegas y yo hicimos una investigación científica sobre este tema recientemente, por lo que me gustaría agregar algunas de nuestras ideas a pesar de que esta es una pregunta bastante antigua. Creamos un modelo de simulación de un proceso / equipo de desarrollo ágil de Kanban y comparamos la revisión previa y posterior a la confirmación para una gran cantidad de situaciones diferentes (diferente número de miembros del equipo, diferentes niveles de habilidades, ...). Observamos los resultados después de 3 años de tiempo de desarrollo (simulado) y buscamos 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:
De estos, derivamos las siguientes reglas heurísticas:
El documento de investigación completo está disponible aquí: http://dx.doi.org/10.1145/2904354.2904362 o en mi sitio web: http://tobias-baum.de
fuente
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 ramificación. El uso de una rama de desarrollador o una rama de características tiene una serie de beneficios ... entre ellos, facilitar la revisión del código posterior 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 comprometidos para incluir en la revisión. Crisol mostrará qué archivos se tocaron en los conjuntos de cambios seleccionados, realizará un seguimiento de los archivos que cada revisor ya ha leído (mostrando un% completado en general) y permitirá que los revisores hagan comentarios fácilmente.
http://www.atlassian.com/software/crucible/overview
Algunos otros beneficios de las ramas de usuario / función:
Para los desarrolladores sin experiencia, una buena consulta es consultar con un mentor y / o programar un par, pero no consideraría esto como una "revisión de código".
fuente
Ambos. (Mas o menos.)
Debería revisar su propio código de forma resumida antes de confirmarlo. En Git, creo que el área de preparación es genial. Después de organizar mis cambios, corro
git diff --cachedpara ver todo lo que está organizado. Utilizo esto como una oportunidad para asegurarme de que no estoy registrando ningún archivo que no pertenezca (artefactos de compilación, registros, etc.) y asegurándome de que no tengo ningún código de depuración o ningún código importante comentado fuera. (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).Una vez dicho esto, la revisión de su código de pares generalmente debe realizarse después de que se comprometa, 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 asincrónica (es decir, usando Google Code o Atlassian Crucible), puede cambiar fácilmente de sucursal y trabajar en otra cosa sin tener que hacer un seguimiento por separado de todos sus diferentes parches / diferencias que están actualmente en revisión durante unos días.
Si no está trabajando en una rama temática, debería hacerlo . Reduce el estrés y las molestias y hace que la planificación de la liberación sea mucho menos estresante y complicada.
Editar: también debería agregar que debería hacer una revisión del código después de la prueba, que es otro argumento a favor de confirmar el código primero. No desea que su grupo de prueba juegue con docenas de parches / diferencias de todos los programadores, y luego presente errores solo porque aplicaron el parche incorrecto en el lugar equivocado.
fuente
Programación 100% pareada (no importa cuán alto piense que es) 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 posteriores al compromiso para cambios grandes o riesgosos. Si debe tener algún tipo de revisión cerrada / previa a la confirmación, Gerrit funciona.
fuente
La ventaja de la revisión de código en el check in (verificación de amigos) es la retroalimentación inmediata, antes de que se completen grandes piezas de código.
La desventaja de la revisión del código en el registro es que puede desanimar a las personas a registrarse hasta que se hayan completado largos tramos de código. Si esto sucede, niega por completo la ventaja.
Lo que lo hace 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 obligatoria de pares, que permite hacer 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.
Desarrollador de sucursales, el código solo se revisa y verifica en la sucursal principal cuando finaliza. Algunos desarrolladores son propensos 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.
Revisión en cada check-in, lo que garantiza revisiones frecuentes. Algunos desarrolladores se olvidan y confían en 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 retrasarán aún más cuando haya una fecha límite. El código que depende del código ya confirmado pero aún no revisado se confirmará. Las revisiones marcarán problemas y los problemas se colocarán en la cartera de pedidos para que se solucionen "más tarde". 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 el check-in funcione, pero es menos simple porque tiene que decidir cuál es ese tiempo específico
En la práctica, hace que esto funcione haciéndolo aún más simple y más complejo al mismo tiempo. Establece pautas simples y deja que cada equipo de desarrollo descubra como equipo lo que deben hacer para seguir estas pautas. Un ejemplo de tales pautas es:
Son posibles muchas formas alternativas de tales pautas. Concéntrese en lo que realmente quiere (código revisado por pares, progreso de trabajo observable, responsabilidad) y deje que el equipo descubra cómo pueden darle lo que quieren.
fuente
en realidad hacemos un híbrido en LedgerSMB. Los committers cometen cambios que se revisan después. Los no comprometidos envían cambios a los confirmadores para que sean revisados antes. Esto tiende a significar dos niveles de revisión. Primero obtienes un mentor para que te revise y te oriente. Luego, ese mentor revisa el código por segunda vez después de que él o ella lo haya firmado y los comentarios circulan. Los nuevos encargados usualmente pasan mucho tiempo revisando los compromisos de otras personas al principio.
Funciona bastante bien Sin embargo, la cuestión 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 hayan demostrado su valía. Pero si tiene una revisión de dos niveles para las nuevas personas, significa que es más probable que se detecten los problemas y se hayan discutido.
fuente
Comprometerse donde? Hay una rama que creé para hacer un trabajo. Me comprometo con esa rama cada vez que tengo ganas. No es asunto de nadie. Luego, en algún momento, esa rama se integra en una rama de desarrollo. Y en algún punto intermedio hay una revisión de código.
El crítico revisa obviamente después de comprometerme con mi sucursal. Él no está sentado en mi escritorio, así que no puede revisarlo antes de que me comprometa con mi sucursal. Y revisa antes de la fusión y se compromete con la rama de desarrollo.
fuente
Simplemente no hagas revisiones de código en absoluto. O cree que sus desarrolladores son capaces de escribir un buen código, o debería deshacerse de ellos. Los errores en la lógica deben ser detectados por sus pruebas automatizadas. Los errores en el estilo deben ser detectados por las herramientas de análisis de pelusa y estática.
Tener humanos involucrados en lo que deberían ser procesos automáticos es simplemente un desperdicio.
fuente