¿Qué hacer cuando el código enviado para la revisión del código parece ser demasiado complicado?

Si no se puede revisar, no se puede aprobar.

Tienes que entender que la revisión del código no es para encontrar errores. Para eso es el control de calidad. La revisión del código es para asegurar que el futuro mantenimiento del código sea posible. Si ni siquiera puede seguir el código ahora, ¿cómo puede hacerlo en seis meses cuando está asignado a realizar mejoras de funciones y / o corrección de errores? Encontrar errores ahora mismo es solo un beneficio secundario.

Si es demasiado complejo, está violando una tonelada de principios SOLID . Refactor, refactor, refactor. Divídalo en funciones apropiadamente nombradas que hacen mucho menos, más simple. Puede limpiarlo y sus casos de prueba se asegurarán de que continúe funcionando correctamente. Usted tiene casos de prueba, ¿verdad? Si no, debes comenzar a agregarlos.

Todo lo que mencionas es perfectamente válido para señalarlo en una revisión de código.

Cuando recibo una revisión del código, sí reviso las pruebas. Si las pruebas no brindan suficiente cobertura, eso es algo que debe señalarse. Las pruebas deben ser útiles para garantizar que el código funciona 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 probado que su código cumple con los requisitos, no quiero invertir mi tiempo en revisarlo.

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 estar viendo. Las herramientas de análisis estático pueden señalar algunas medidas de complejidad y marcar métodos excesivamente complejos, así como encontrar fallas potenciales 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 mantenerlo. Solo si hay una razón para usar un código menos mantenible, debe escribirse de esa manera. Si necesita tener un código complejo o no 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é hace el código.

Lo ideal es rechazar las revisiones de código que no tengan las pruebas adecuadas o que tengan un 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 tickets en su sistema de seguimiento de errores inmediatamente con algunos detalles de lo que debe cambiar y algunas sugerencias para cambiarlo.

  

La verificación de la corrección general de forma manual a través de la revisión del código, sin embargo, es muy difícil y requiere mucho tiempo, si es que es posible.

Ese no es el punto de una revisión de código de forma remota. La forma de pensar en una revisión de código es imaginar que hay un error en el código y que hay que solucionarlo. Con esta mentalidad, navegue por el código (especialmente los comentarios) y pregúntese "¿Es fácil entender el panorama general de lo que está pasando para poder reducir el problema?" Si es así, es un pase. De lo contrario, es un fracaso. Por lo menos se necesita más documentación, o posiblemente se necesita refactorización para hacer 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 ser refactorizado fácilmente 10 veces seguidas, cada vez más legibles. Pero es probable que su empleador no quiera pagar para tener el código más legible del mundo.

  

La verificación de la corrección general de forma manual a través de la revisión del código, sin embargo, es muy   Difícil y lento, si es posible.

Hace muchos años, en realidad era mi trabajo hacer exactamente eso al calificar las tareas de los estudiantes. Y aunque muchos entregaron una calidad razonable con un error aquí y allá, hubo dos que se destacaron. Ambos siempre presentaron código que no tenía errores. Un código enviado que pude leer desde arriba y abajo a alta velocidad y marcarlo como correcto al 100% sin esfuerzo. El otro código enviado era un WTF después del otro, pero de alguna manera logró evitar cualquier error. Un dolor absoluto para marcar.

Hoy el segundo tendría su código rechazado en una revisión de código. Si la verificación de 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 (toma X) y antes de darle un código lo refactoriza para que no solo haga el trabajo, sino que obviamente haga el trabajo. Eso toma mucho menos que X en el tiempo y ahorra mucho tiempo en el futuro. A menudo, descubriendo errores incluso antes de que lleguen a la etapa de revisión del código. A continuación, haciendo 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 dijo que el código de algunas personas podría ser refactorizado 10 veces, cada vez más legible. Eso es simplemente triste. Es un desarrollador que debe buscar un trabajo diferente.

¿Este código antiguo se modificó ligeramente? (100 líneas de código cambiadas en un código base de 10000 líneas son todavía 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 . + Por lo general, existe 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ás que vivir con él. Si no lo entiende por su cuenta, hable con ellos y escuche lo que dicen, trate de entender. Recuerde, puede ser difícil de seguir para usted, pero perfectamente bien para otras personas. Tomar su lado, verlo desde su extremo.

¿Es este nuevo código ? Dependiendo de las limitaciones de tiempo, debe recomendar refactorizar tanto como sea posible. ¿Está bien pasar más tiempo en revisiones de código si es necesario? No deberías cronometrarte a 15min, captar la idea y seguir adelante. Si el autor pasó una semana escribiendo algo, está bien pasar 4-8 horas para revisarlo. Tu objetivo aquí es ayudarlos a refactorizar. No solo devuelve el código que dice "refactor. Ahora". Vea qué métodos se pueden desglosar, intente encontrar ideas para introducir nuevas clases, etc.

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 expandidas; hace que sea difícil ver el panorama general.

Una pista común es que el parche es enorme pero su descripción es minúscula: "Implementar $ FOO".

Una forma razonable de manejar tal parche es pedir que se rompa en una serie de piezas más pequeñas y autónomas. Al igual 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 realizan cambios funcionales, y luego los parches finales pueden enfocarse en la implementación y prueba real de $ FOO con menos distracciones y pistas falsas.

Para una funcionalidad que requiere un montón de código nuevo, el nuevo código a menudo se puede introducir en trozos comprobables que no cambian el comportamiento del producto hasta que el último parche de la serie llama al nuevo código (un giro de bandera).

En cuanto a hacer esto con tacto, generalmente lo menciono como mi problema y luego le pido ayuda al autor: "Tengo problemas para seguir todo lo que sucede aquí. ¿Podría dividir este parche en pasos más pequeños para ayudarme a entender cómo todo esto encaja? " 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:

1. Presente una nueva versión de Frobnicate que tome un par de iteradores porque voy a necesitar llamarlo con secuencias distintas de vector para implementar $ FOO.
2. Cambie todas las personas que llaman de Frobnicate para que usen la nueva versión.
3. Eliminar el antiguo Frobnicate.
4. Frobnicate estaba haciendo demasiado. Factoriza el paso de refrumple en su propio método y agrega pruebas para eso.
5. Presente Zerzify, con pruebas. No se ha utilizado todavía, pero lo necesitaré por $ FOO.
6. Implemente $ FOO en términos de Zerzify y el nuevo Frobnicate.

Tenga en cuenta que los pasos 1 a 5 no realizan cambios funcionales en el producto. Son triviales de revisar, incluyendo asegurarse de que tenga todas las pruebas correctas. Incluso si el paso 6 sigue siendo "complicado", al menos se enfoca en $ FOO. Y, naturalmente, el registro le da una mejor idea de cómo se implementó $ FOO (y por qué se cambió Frobnicate).

Como han señalado otros, la revisión de código no está realmente diseñada para encontrar errores. Si está encontrando 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 , por lo general 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 Codebase que no tiene cobertura adecuada.

Si la arquitectura de alto nivel es demasiado compleja o no tiene sentido, generalmente llamo 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 que pasemos por lo que piensan adelante de tiempo en lugar de reaccionar ante una solicitud de mala voluntad. Esto suele ser cierto incluso con desarrolladores más experimentados si el problema no tiene una solución obvia que sea más probable que se elija.

Si la complejidad se aísla 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 la esencial o accidental complejidad . 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 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 más. También aspiro a una mayor 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 son la mayor parte del código con el que tratamos y pueden causar fácilmente pesadillas de mantenimiento incluso a corto plazo.

¿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 deseado y los casos de uso del código.

Si no está bien probado, es un buen lugar para comenzar a revisar.