En las revisiones de código, ¿debería el revisor presentar siempre una solución para los problemas? [cerrado]

Como revisor, su trabajo es verificar si una parte del código (o un documento) cumple con ciertos objetivos acordados antes de la revisión.

Algunos de estos objetivos normalmente implicarán una llamada de juicio si el objetivo se ha cumplido o no. Por ejemplo, el objetivo de que el código debe ser mantenido normalmente requiere una llamada de juicio.

Como revisor, es su trabajo señalar dónde no se han cumplido los objetivos y el trabajo del autor es asegurarse de que su trabajo realmente cumpla los objetivos. De esta manera, no es su trabajo decir cómo deben hacerse las correcciones.

Por otro lado, solo decirle al autor "esto es defectuoso. Solucionarlo" no suele generar una atmósfera positiva en el equipo. Para una atmósfera positiva, es bueno al menos indicar por qué hay algo defectuoso en sus ojos y ofrecerle una mejor alternativa si tiene una.
Además de eso, si está revisando algo que parece "incorrecto" pero realmente no tiene una mejor alternativa, entonces también podría dejar un comentario como "Este código / diseño no me sienta bien, pero No tengo una alternativa clara. ¿Podemos discutir esto? " y luego tratar de obtener algo mejor juntos.

Algunas buenas respuestas aquí, pero creo que falta un punto importante. Hace una gran diferencia el código que está revisando, la experiencia de esa persona y cómo maneja esas sugerencias. Si conoce bien a su compañero de equipo y espera una nota como "este código es defectuoso porque ..., hágalo de manera diferente" para que él o ella puedan encontrar una mejor solución, entonces un comentario puede estar bien Pero definitivamente hay personas en las que tal comentario no es suficiente, y a quienes se les debe decir exactamente cómo mejorar su código. Así que, en mi humilde opinión, esta es una decisión de juicio que solo puede hacer para el caso individual.

  

Por lo general, como revisor, ¿está bien decir que "este código es defectuoso,   hazlo de manera diferente "o tienes que encontrar una solución específica?

Ninguno de los dos es IMO ideal. Lo mejor que puedes hacer es hablar con el autor y solucionar el problema en colaboración.

Las revisiones de código no tienen que ser asíncronas. Muchos problemas se desbloquearán si dejas de verlos como un proceso burocrático y tomas un poco de tiempo para la comunicación en vivo.

  

En las revisiones de código, ¿el revisor siempre presenta una solución para los problemas?

No. Si estás haciendo eso, no eres un crítico, eres el siguiente programador.

  

En las revisiones de código, ¿el revisor nunca presenta una solución para los problemas?

No. Su trabajo es comunicar el problema en cuestión. Si presentar una solución deja claro el problema, entonces hazlo. Pero no esperes que siga tu solución. Lo único que puedes hacer aquí es hacer un punto. No llegas a dictar la implementación.

  

¿Cuándo debería un revisor presentar una solución para los problemas?

Cuando esa es la forma más efectiva de comunicarse. Somos monos codificados no mayores ingleses. A veces, la mejor manera de mostrar que el código apesta ... es menos que óptimo ... es mostrarles el código que apesta menos ... es más opcional ... oh, diablos, ya sabes lo que quiero decir.     

El problema principal es que si las personas supieran cómo escribir mejor el código, en primer lugar lo habrían hecho. Si una crítica es lo suficientemente específica depende mucho de la experiencia del autor. Los programadores muy experimentados podrían ser capaces de tomar una crítica como "esta clase es demasiado complicada" y volver al tablero de dibujo y proponer algo mejor, porque tuvieron un mal día debido a un dolor de cabeza o estaban siendo descuidados porque estaban en un apuro.

Por lo general, sin embargo, debes identificar al menos la fuente de la complicación. "Esta clase es demasiado complicada porque rompe la Ley de Demeter por todas partes". "Esta clase mezcla las responsabilidades de capa de presentación y capa de persistencia". Aprender a identificar esas razones y explicarlas de manera sucinta es parte de ser un mejor revisor. Rara vez tiene que entrar en muchos detalles acerca de las soluciones.

Hay dos tipos de malos programadores: los que no siguen las prácticas estándar y los que "solo" siguen las prácticas estándar.

Cuando he tenido un contacto laboral limitado / proporcionando comentarios a alguien, no diría: "Este es un mal diseño". Pero algo como "¿Me puedes explicar esta clase?" Puede descubrir que es una buena solución, el desarrollador hizo lo mejor que pudo e incluso un reconocimiento de que es una mala solución, pero es lo suficientemente bueno.

Dependiendo de la respuesta, vas a tener una mejor idea de cómo abordar cada situación y persona. Pueden reconocer rápidamente el problema y descubrir la solución por sí mismos. Pueden pedir ayuda o solo intentarán resolverlo por su cuenta.

Hay prácticas sugeridas en nuestro negocio, pero casi todas tienen excepciones. Si comprende el proyecto y cómo el equipo lo está abordando, ese puede ser el contexto para determinar el propósito de la revisión del código y cómo abordar las inquietudes.

Me doy cuenta de que esto es más un enfoque del problema que una solución explícita. Habrá demasiada variabilidad para cubrir todas las situaciones.

Cuando reviso el código, solo proporciono una solución para los problemas que identifico si puedo hacerlo con poco esfuerzo. Sin embargo, trato de ser específico sobre lo que creo que es el problema, refiriéndome a la documentación existente siempre que sea posible. Esperar que un revisor proporcione una solución para cada problema identificado crea un incentivo perverso: desalentará al revisor a señalar los problemas.

Mi opinión es más fuerte para no proporcionar código en la mayoría de los casos, por varias razones:

• Si la explicación por sí sola no es suficiente, siempre pueden pedir una muestra de lo que estás pensando.
• No estás perdiendo el tiempo intentando familiarizarte con un código que no has tocado en mucho tiempo, solo para modificarlo ligeramente, mientras que alguien más ha dedicado su tiempo a hacer eso.
• Si ya están familiarizados con la parte del código y usted no lo está, dar solo los comentarios puede resultar en un código mejor del que escribiría. Darle a alguien una solución lista a menudo hará que solo la usen, sin considerar la posibilidad de mejorarla más.
• Siempre proporcionar una solución es casi condescendiente. Estás trabajando con alguien, con suerte porque fueron lo suficientemente buenos para ser contratados. Si logró aprender por qué algo es una mala idea, ¿por qué no lo aprenderían escuchando sus comentarios y haciéndolos ellos mismos?
• Siempre proporcionar una solución es simplemente extraño. Imagina que estás programando en pareja en su escritorio: acaban de terminar un par de líneas que crees que no son geniales. ¿Simplemente les dice lo que vio y por qué, o toma su teclado y muestra su versión de inmediato? Esto es lo que siempre se puede sentir al proporcionar su solución a otras personas.
• En su lugar, siempre puedes decir lo que harías, sin perder el tiempo para escribirlo. Hiciste exactamente eso al describir el primer problema de la pregunta.
• No distribuyas comida, enseña a pescar;)

Claro, hay algunos casos en los que estás pensando en alguna alternativa específica, y vale la pena adjuntarla. Pero eso es realmente raro en mi experiencia. (muchos comentarios, grandes proyectos) El autor original siempre puede pedirle una muestra si es necesario.

Incluso entonces, debido a la tercera razón, al dar una muestra, vale la pena decir, por ejemplo, "usar x.foo() haría esto más simple" en lugar de una solución completa, y dejar que el autor la escriba. Esto también te ahorra tiempo.

Creo que la clave para codificar las revisiones es acordar las reglas antes de la revisión.

Si tiene un conjunto claro de reglas, no debería haber necesidad de ofrecer una solución. Sólo estás comprobando que se han seguido las reglas.

La única vez que surgiría la pregunta de un suplente sería si el desarrollador original no puede pensar en una forma de implementar la función que se ajuste a las reglas. Supongamos que tiene un requisito de rendimiento, pero la función tarda más que su umbral después de varios intentos de optimización.

Sin embargo! si sus reglas son subjetivas "¡Los nombres deben ser aprobados por mí!" entonces, sí, usted se acaba de nombrar namer en jefe y debe esperar solicitudes de listas de nombres aceptables.

El ejemplo de su herencia (parámetros opcionales) es quizás mejor, int porque he visto reglas de revisión de código que prohíben los métodos largos y los parámetros de función "demasiados". Pero normalmente estos pueden resolverse trivialmente mediante la división. Es la parte de "esta solución parecía complicar demasiado las cosas" en la que su objetividad es intrusa y quizás requiera una justificación o una solución alternativa.

Si una solución potencial es rápida y fácil de escribir, trato de incluirla en mis comentarios de revisión por pares. Si no, al menos enumero mi preocupación y por qué encuentro problemático ese artículo en particular. En el caso de nombres de funciones / variables donde no se puede pensar en algo mejor, por lo general, reconozco que no tengo una idea mejor y finalizo el comentario en forma de una pregunta abierta en caso de que alguien pueda hacerlo. . De esa manera, si a nadie se le ocurre una mejor opción, la revisión no se está demorando.

Para el ejemplo que dio en su pregunta, con la clase mal diseñada. Dejaría algunos comentarios de que, si bien parece que puede ser una exageración, la herencia probablemente sea la mejor manera de abordar el problema que el código está tratando de resolver, y dejarlo así. Trataré de expresarlo de una manera que indique que no es un factor decisivo y depende de la discreción del desarrollador si lo arreglamos o no. También abriría con un reconocimiento de que no está particularmente familiarizado con esa parte del código e invitaría a desarrolladores y / o revisores más informados para aclarar si hay una razón por la que se hace de la manera correcta.

Ve y habla con la persona cuyo código estás revisando. Dígales, de manera amistosa, que le ha resultado un poco difícil de entender, y luego hable con ellos sobre cómo se podría aclarar.

La comunicación escrita lleva a una gran cantidad de tiempo perdido, así como a resentimientos y malentendidos.

Cara a cara, el ancho de banda es mucho más alto y uno tiene el canal lateral emocional para evitar la hostilidad.

Si realmente hablas con el chico, es mucho más rápido, puedes hacerte un nuevo amigo y descubrir que ambos disfrutan más de tu trabajo.