El código es difícil de seguir, pero parece estar funcionando (principalmente) bien, al menos con pruebas superficiales. Puede haber pequeños errores aquí y allá, pero es muy difícil saberlo leyendo el código si son síntomas de problemas más profundos o soluciones simples. Sin embargo, verificar la corrección general manualmente mediante la revisión de código es muy difícil y requiere mucho tiempo, si es que es posible.
¿Cuál es el mejor curso de acción en esta situación? ¿Insistir en una repetición? ¿Repetición parcial? Refactorización primero? ¿Solucionar solo los errores y aceptar la deuda técnica ? ¿Hacer una evaluación de riesgos sobre esas opciones y luego decidir? ¿Algo más?
code-reviews
Brad Thomas
fuente
fuente
Respuestas:
Si no puede revisarse, no puede pasar la revisión.
Debe comprender que la revisión de código no es para encontrar errores. Para eso está el control de calidad. La revisión del código es para asegurar que el mantenimiento futuro del código sea posible. Si ni siquiera puede seguir el código ahora, ¿cómo puede hacerlo en seis meses cuando le asignen mejoras de funciones y / o corrección de errores? Encontrar errores en este momento es solo un beneficio adicional.
Si es demasiado complejo, está violando una tonelada de principios SÓLIDOS . Refactor, refactor, refactor. Divídalo en funciones con nombre apropiado que hacen mucho menos, más simple. Puede limpiarlo y sus casos de prueba se asegurarán de que continúe funcionando correctamente. Tienes casos de prueba, ¿verdad? Si no, debes comenzar a agregarlos.
fuente
Todo lo que menciona es perfectamente válido para señalar en una revisión de código.
Cuando recibo una revisión de código, reviso las pruebas. Si las pruebas no proporcionan una cobertura suficiente, eso es algo que señalar. Las pruebas deben ser útiles para garantizar que el código funcione según lo previsto y continuará funcionando según lo previsto en los cambios. De hecho, esta es una de las primeras cosas que busco en una revisión de código. Si no ha demostrado que su código cumple con los requisitos, no quiero invertir mi tiempo en verlo.
Una vez que haya suficientes pruebas para el código, si el código es complejo o difícil de seguir, eso también es algo que los humanos deberían mirar. Las herramientas de análisis estático pueden señalar algunas medidas de complejidad y señalar métodos demasiado complejos, así como encontrar posibles fallas en el código (y deben ejecutarse antes de una revisión del código humano). Pero el código es leído y mantenido por humanos, y primero debe escribirse para mantenimiento. Solo si hay una razón para usar un código menos mantenible, debe escribirse de esa manera. Si necesita tener código complejo o poco intuitivo, debe documentarse (preferiblemente en el código) por qué el código es así y tener comentarios útiles para que los futuros desarrolladores entiendan por qué y qué está haciendo el código.
Idealmente, rechace las revisiones de código que no tienen pruebas apropiadas o tienen código demasiado complejo sin una buena razón. Puede haber razones comerciales para seguir adelante, y para eso necesitaría evaluar los riesgos. Si continúa con la deuda técnica en el código, coloque los tickets en su sistema de seguimiento de errores de inmediato con algunos detalles de lo que debe cambiar y algunas sugerencias para cambiarlo.
fuente
Eso no es remotamente el punto de una revisión de código. La forma de pensar en una revisión de código es imaginar que hay un error en el código y que debe solucionarlo. Con esta mentalidad, navegue a través del código (especialmente los comentarios) y pregúntese "¿Es fácil entender el panorama general de lo que está sucediendo para poder reducir el problema?" Si es así, es un pase. De lo contrario, es un fracaso. Se necesita más documentación como mínimo, o posiblemente se necesite refactorizar para que el código sea razonablemente comprensible.
Es importante no ser perfeccionista al respecto a menos que esté seguro de que es lo que busca su empleador. La mayoría del código apesta tanto que podría refactorizarse fácilmente 10 veces seguidas, volviéndose más legible cada vez. Pero es probable que su empleador no quiera pagar para tener el código más legible del mundo.
fuente
Hace muchos años, en realidad era mi trabajo hacer exactamente eso al calificar la tarea de los estudiantes. Y aunque muchos ofrecieron una calidad razonable con un error aquí y allá, hubo dos que se destacaron. Ambos siempre enviaron código que no tenía errores. Un código enviado que podía leer desde arriba y abajo a alta velocidad y marcarlo como 100% correcto sin esfuerzo. El otro código enviado era un WTF después del otro, pero de alguna manera logró evitar errores. Un dolor absoluto para marcar.
Hoy el segundo tendría su código rechazado en una revisión de código. Si verificar la corrección es muy difícil y requiere mucho tiempo, entonces ese es un problema con el código. Un programador decente descubriría cómo resolver un problema (lleva tiempo X) y antes de entregarlo a un refactorizador de revisión de código para que no solo haga el trabajo, sino que obviamente haga el trabajo. Eso lleva mucho menos de X en el tiempo y ahorra mucho tiempo en el futuro. A menudo, descubriendo errores incluso antes de pasar a la etapa de revisión de código. Luego, al hacer la revisión del código mucho más rápido. Y todo el tiempo en el futuro haciendo que el código sea más fácil de adaptar.
Otra respuesta decía que el código de algunas personas podría refactorizarse 10 veces, volviéndose más legible cada vez. Eso es triste Es un desarrollador que debería buscar un trabajo diferente.
fuente
¿Es este antiguo código que se modificó ligeramente? (100 líneas de código cambiadas en una base de código de 10000 líneas sigue siendo un ligero cambio) A veces hay limitaciones de tiempo y los desarrolladores se ven obligados a permanecer dentro de un marco antiguo e inconveniente, simplemente porque una reescritura completa tomaría aún más tiempo y está fuera del presupuesto . + generalmente hay un riesgo involucrado, que puede costar millones de dólares cuando se evalúa incorrectamente. Si es un código antiguo, en la mayoría de los casos tendrá que vivir con él. Si no lo comprende solo, hable con ellos y escuche lo que dicen, intente comprender. Recuerde, puede ser difícil de seguir para usted, pero perfectamente bien para otras personas. Ponte de su lado, míralo desde el final.
¿Es este nuevo código ? Dependiendo de las limitaciones de tiempo, debe abogar por refactorizar tanto como sea posible. ¿Está bien pasar más tiempo en revisiones de código si es necesario? No deberías hacer un timebox de 15 minutos, entender la idea y seguir adelante. Si el autor pasó una semana escribiendo algo, está bien pasar de 4 a 8 horas para revisarlo. Su objetivo aquí es ayudarlos a refactorizar. No solo devuelve el código que dice "refactorizar. Ahora". Vea qué métodos se pueden desglosar, intente proponer ideas para introducir nuevas clases, etc.
fuente
A menudo, los parches / listas de cambios "complicados" son los que hacen muchas cosas diferentes a la vez. Hay código nuevo, código eliminado, código refactorizado, código movido, pruebas ampliadas; hace que sea difícil ver el panorama general.
Una pista común es que el parche es enorme pero su descripción es pequeña: "Implementar $ FOO".
Una forma razonable de manejar un parche de este tipo es pedir que se rompa en una serie de piezas más pequeñas y autónomas. Del mismo modo que el principio de responsabilidad única dice que una función debe hacer solo una cosa, un parche también debe enfocarse en una sola cosa.
Por ejemplo, los primeros parches pueden contener refactorizaciones puramente mecánicas que no hacen cambios funcionales, y luego los parches finales pueden centrarse en la implementación real y las pruebas de $ FOO con menos distracciones y arenques rojos.
Para la funcionalidad que requiere mucho código nuevo, el nuevo código a menudo se puede introducir en fragmentos comprobables que no cambian el comportamiento del producto hasta que el último parche de la serie realmente llama al nuevo código (un cambio de marca).
En cuanto a hacer esto con tacto, generalmente lo considero mi problema y luego pido ayuda del autor: "Tengo problemas para seguir todo lo que está sucediendo aquí. ¿Podría dividir este parche en pasos más pequeños para ayudarme a comprender cómo encaja todo esto?" ¿juntos?" A veces es necesario hacer sugerencias específicas para los pasos más pequeños.
Un parche tan grande como "Implementar $ FOO" se convierte en una serie de parches como:
Tenga en cuenta que los pasos 1-5 no hacen cambios funcionales en el producto. Son triviales de revisar, lo que incluye garantizar que tenga todas las pruebas correctas. Incluso si el paso 6 sigue siendo "complicado", al menos se centra en $ FOO. Y el registro naturalmente le da una idea mucho mejor de cómo se implementó $ FOO (y por qué se cambió Frobnicate).
fuente
Como otros señalaron, la revisión de código no está realmente diseñada para encontrar errores. Si encuentra errores durante la revisión del código, eso probablemente significa que no tiene suficiente cobertura de prueba automatizada (por ejemplo, pruebas de unidad / integración). Si no hay suficiente cobertura para convencerme de que el código hace lo que se supone que debe hacer , generalmente solicito más pruebas y señalo el tipo de casos de prueba que estoy buscando y, por lo general, no permito el código en la base de código que no tiene una cobertura adecuada .
Si la arquitectura de alto nivel es demasiado compleja o no tiene sentido, generalmente convoco a una reunión con un par de miembros del equipo para hablar sobre ello. A veces es difícil iterar en una mala arquitectura. Si el desarrollador era un novato, generalmente me aseguro de analizar lo que piensan con anticipación en lugar de reaccionar a una solicitud de extracción incorrecta. Esto suele ser cierto incluso con desarrolladores más experimentados si el problema no tiene una solución obvia que probablemente sea elegida.
Si la complejidad está aislada al nivel del método, generalmente se puede corregir de forma iterativa y con buenas pruebas automatizadas.
Un último punto Como revisor, debe decidir si la complejidad del código se debe a una complejidad esencial o accidental . La complejidad esencial se relaciona con las partes del software que son legítimamente difíciles de resolver. La complejidad accidental se refiere a todas las otras partes del código que escribimos que es demasiado complejo sin ninguna razón y podría simplificarse fácilmente.
Por lo general, me aseguro de que el código con complejidad esencial sea realmente eso y no pueda simplificarse aún más. También busco más cobertura de prueba y buena documentación para estas partes. La complejidad accidental casi siempre debe limpiarse durante el proceso de solicitud de extracción, ya que esa es la mayor parte del código con el que tratamos y puede causar fácilmente una pesadilla de mantenimiento, incluso a corto plazo.
fuente
¿Cómo son las pruebas? Deben ser claros, simples y fáciles de leer, idealmente con una sola afirmación. Las pruebas deben documentar claramente el comportamiento previsto y los casos de uso del código.
Si no está bien probado, es un buen lugar para comenzar a revisar.
fuente