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

93

Cuando reviso el código, normalmente trato de hacer recomendaciones específicas sobre cómo resolver los problemas. Pero debido al tiempo limitado que uno puede pasar para revisar, esto no siempre funciona bien. En estos casos, encuentro más eficiente si el desarrollador encuentra una solución por sí mismo.

Hoy revisé un código y descubrí que una clase obviamente no estaba bien diseñada. Tenía una serie de atributos opcionales que solo se asignaban a ciertos objetos y se dejaban en blanco para otros. La forma estándar de resolver esto sería dividir la clase y usar la herencia. Sin embargo, en este caso específico, esta solución parecía complicar demasiado las cosas. Yo no participé en el desarrollo de este software y no estoy familiarizado con todos los módulos. Por lo tanto, no me sentí lo suficientemente informado como para tomar una decisión específica.

Otro caso típico que experimenté muchas veces es que encuentro una función, un nombre de clase o variable obviamente sin sentido o incluso engañoso, pero no puedo encontrar un buen nombre yo mismo.

En general, como revisor, ¿está bien decir "este código es defectuoso porque ..., lo hace de manera diferente" o tiene que encontrar una solución específica?

Frank Puffer
fuente
24
@gnat: No, el código no es demasiado complicado. Y es solo un ejemplo. Generalmente pregunto si el revisor es responsable de presentar una solución.
Frank Puffer
55
no, yo diría que, como revisor, no estás obligado a decir cómo mejorarlo. Si puedes describir lo que se siente mal allí, hazlo; si no, solo proporcione comentarios generales. (Uno de los comentarios de revisión más útiles que recuerdo haber recibido fue literalmente como "esta clase es basura total")
mosquito
55
"La forma estándar de resolver esto sería dividir la clase y usar la herencia". ¡Espero que no estés revisando mi código!
cabeza de jardín
77
Identificar posibles problemas podría ser suficiente. El revisor mira el código como usuario, mantenedor o diseñador. Proporcionar una vista de ángulo diferente o detectar problemas que el codificador aún no haya notado puede ayudarlo a mejorar su trabajo. Y si ve algo que le gusta, no estaría de más informarlo también. No debería ser un ejercicio correctivo, sino más bien esclarecedor. Es por eso que se llama "revisión por pares".
Martin Maat

Respuestas:

139

Como revisor, su trabajo es verificar si un código (o documento) cumple con ciertos objetivos que se acordaron antes de la revisión.

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

Como revisor, es su trabajo señalar dónde no se han cumplido los objetivos y es el trabajo del autor asegurarse de que su trabajo realmente cumpla con 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. Arreglarlo" generalmente no conduce a una atmósfera positiva en el equipo. Para una atmósfera positiva, es bueno al menos indicar por qué algo tiene defectos en los ojos y proporcionar 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 en la línea de "Este código / diseño no me sienta bien, pero yo no tengo una alternativa clara. ¿Podemos discutir esto? " y luego intenten obtener algo mejor juntos.

Bart van Ingen Schenau
fuente
23
+1 para que el debate llegue a una solución juntos: esta es la forma en que más aprendo de los programadores senior que revisan mi código
dj18
19
+1. Al dar retroalimentación, es mejor hacer una crítica constructiva siempre que sea posible.
FrustratedWithFormsDesigner
77
Estoy de acuerdo con el último bit especialmente. Está perfectamente bien decir, "esta solución se siente mal porque ..." o "Me preocupa que esta parte pueda ser problemática porque ..." sin dar una solución.
Daniel T.
1
@dotancohen: El "podemos discutir esto" fue una pregunta. Personalmente, tendría la discusión de todos modos, incluso si es solo para aprender algo yo mismo.
Bart van Ingen Schenau
1
El peligro sutil es que, con suficiente discusión y comunicación de implementación, esto deja de ser una revisión y se convierte en programación en pareja. La programación de pares es buena, pero una vez que se hace, necesita una tercera persona para revisar porque se ha perdido la independencia.
candied_orange
35

Algunas buenas respuestas aquí, pero creo que falta un punto importante. Hace una gran diferencia el código que está revisando, qué tan experimentada es esa persona y cómo maneja esas sugerencias. Si conoce bien a su compañero de equipo y espera que una nota como "este código sea defectuoso porque ..., hágalo de manera diferente" sea ​​suficiente para que él o ella encuentre una mejor solución, entonces ese comentario puede estar bien. Pero definitivamente hay personas en las que dicho comentario no es suficiente, y que necesitan que se les diga exactamente cómo mejorar su código. Entonces, en mi humilde opinión, esta es una decisión judicial que solo puede hacer para el caso individual.

Doc Brown
fuente
29

Por lo tanto, en general, como revisor, ¿está bien decir "este código tiene fallas, hacerlo de manera diferente" o tiene que encontrar una solución específica?

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

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

guillaume31
fuente
¡El "proceso burocrático" es una buena manera de decirlo!
viernes
17

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

No. Si estás haciendo eso, no eres un crítico, eres el próximo codificador.

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

No. Su trabajo es comunicar el problema en cuestión. Si presentar una solución aclara el problema, hágalo. Simplemente no esperes que siga tu solución. Lo único que puedes hacer aquí es hacer un punto. No puede dictar la implementación.

¿Cuándo debe 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 opt ... oh diablos, ya sabes a qué me refiero.

naranja confitada
fuente
8
No codifique en el vacío, porque apesta.
Hm. Cuando sugiero una solución a un problema, a menudo tiene beneficios que conozco, pero simplemente llevaría demasiado tiempo dar una lista exhaustiva de todos ellos. (Estos a menudo están relacionados con la estabilidad y la confianza de que la cosa seguirá funcionando mientras cambiamos otras cosas a su alrededor). Entonces, si haces algo más que no resuelve tantos problemas, no estaría exactamente feliz (al menos no, a menos que puedas decirme una buena razón por la que lo que sugerí no funcionó). ¿Cómo manejarías eso?
jpmc26
1
PD: "Code monkey" se usa a menudo para describir a un programador no calificado e irreflexivo que simplemente hace lo que se le dice, incluso si es una mala idea y no tiene una buena sensibilidad de diseño. Ver diccionario urbano . Incluso Wikipedia señala que a veces es despectivo.
jpmc26
@ jpmc26 si va a usar un código para comunicarse conmigo, entonces espero que use un código que muestre cómo podría resolverse mejor el problema. Además, Code Monkey se puede usar con cariño. Sin duda, más afecto que los comandantes ingleses llegar
candied_orange
"Code monkey se levanta a tomar café. Code monkey va a trabajar. Code monkey tiene una reunión aburrida, con el aburrido gerente Rob. Rob dice que el código monkey es muy diligente, pero su salida apesta ..."
Baldrickk
13

El problema principal es que si las personas supieran cómo escribir mejor el código, por lo general lo habrían hecho en primer lugar. Si una crítica es lo suficientemente específica depende mucho de la experiencia del autor. Los programadores con mucha experiencia podrían recibir críticas como "esta clase es demasiado complicada" y volver a la mesa de dibujo y encontrar algo mejor, porque simplemente tuvieron un mal día debido a un dolor de cabeza o estaban siendo descuidados porque estaban escapado.

Sin embargo, por lo general, debe identificar al menos la fuente de la complicación. "Esta clase es demasiado complicada porque infringe la Ley de Demeter por todas partes". "Esta clase mezcla las responsabilidades de la capa de presentación y la capa de persistencia". Aprender a identificar esas razones y explicarlas sucintamente es parte de convertirse en un mejor crítico. Raramente tiene que entrar en muchos detalles sobre las soluciones.

Karl Bielefeldt
fuente
44
+100 mi frustración más común con las Revisiones de Código es que si hubiera conocido una mejor manera, probablemente habría escrito de esa manera.
RubberDuck
Amo tu primera oración. Me hizo pensar en preguntarme: "¿es este código lo suficientemente bueno?" ¡Luego lanza una moneda para decidir si molestarse en mejorarla o no! (Normalmente yo sigo en ello hasta que me quede sin tiempo, pero tal vez podría detenerse cuando es lo suficientemente bueno en su lugar?)
OMI "Este código es complicado porque infringe la Ley de Demeter" es un mal comentario. "Este código es complicado porque X está demasiado acoplado a Y y Z" es mejor.
Immibis
"Si las personas supieran cómo escribir mejor el código, por lo general lo habrían hecho". Hay excepciones. Desarrollé este código que funciona, pero nos morderá por el culo en el futuro. El gerente no técnico no entiende "No me gusta este código y quiero tres días para mejorarlo". El gerente no técnico entiende que "Joe revisó y rechazó este código y necesito tres días para mejorarlo".
gnasher729
4

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

Cuando he tenido contacto laboral limitado / proporcionar comentarios a alguien, no diría, "Este es un mal diseño". pero algo así como "¿Puedes explicarme esta clase?" Puede descubrir que es una buena solución, el desarrollador sinceramente hizo lo mejor que pudo o incluso reconoció que es una mala solución, pero es lo suficientemente buena.

Dependiendo de la respuesta, tendrá una mejor idea de cómo abordar cada situación y persona. Pueden reconocer rápidamente el problema y descubrir la solución por su cuenta. Pueden pedir ayuda o simplemente intentarán ir y resolverla por su cuenta.

Existen 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.

JeffO
fuente
1
Pero, si requiere una explicación para ser un buen diseño reconocible, faltan comentarios en línea.
Comodín
1
A veces las reglas no tienen excepciones, pero generalmente no las tienen.
@Wildcard: eso depende de la capacidad y las preferencias / opiniones de las personas que lo miran.
JeffO
1
@Wildcard Tomo el enfoque de que los comentarios deben expresarse como una pregunta, pero la respuesta (eventualmente) tomará la forma de un comentario, o tal vez un cambio de código (por ejemplo, una mejor denominación). Eso deja la puerta abierta para que el desarrollador explique su pensamiento y discuta las opciones, en lugar de sentirse como una demanda, o accidentalmente hacer su trabajo por ellos.
IMSoP
3

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 cuál 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 problemas.

BagOfSpanners
fuente
3

Mi opinión es cada vez 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á desperdiciando su tiempo tratando de familiarizarse con un código que no ha tocado en mucho tiempo, solo para modificarlo ligeramente, mientras que alguien más acaba de pasar su tiempo haciendo exactamente eso.
  • Si ya están familiarizados con el fragmento de código y usted no lo está, dar solo los comentarios puede resultar en un mejor código del que escribiría. Darle a alguien una solución lista a menudo hará que solo la use, sin considerar mejorarla aún más.
  • Proporcionar siempre una solución raya en el condescendiente. Estás trabajando con alguien, con suerte porque era lo suficientemente bueno como para ser contratado. Si lograste aprender por qué algo es una mala idea, ¿por qué no lo aprenderían escuchando comentarios y haciéndolo 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. ¿Les dice lo que vio y por qué, o toma su teclado y muestra su versión de inmediato? Esto es lo que siempre puede brindarle su solución a otras personas.
  • Siempre puede decir lo que haría en su lugar, sin perder el tiempo para escribirlo. Hiciste exactamente eso al describir el primer problema en la pregunta.
  • No repartir comida, enseñar a pescar;)

Claro, hay algunos casos en los que está pensando en alguna alternativa específica, y vale la pena adjuntarla. Pero eso es realmente raro en mi experiencia. (muchas revisiones, 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, puede valer 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.

viraptor
fuente
Tu quinto punto me hizo sonreír, estaba imaginando "teclados en duelo" para ver quién podría obtener una gran solución primero. ¿Quién sabía que la programación en pareja podría ser como esos dos juegos de carreras de autos o un deporte de contacto completo? " Steve acaba de registrar brutalmente a Ron en el BSOD, 2 minutos en el área de penalización ... "
2

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

Si tiene un conjunto claro de reglas, entonces no debería ser necesario ofrecer una solución. Solo está comprobando que se han seguido las reglas.

La única vez que surgiría la cuestión de una alternativa sería si el desarrollador original no puede pensar en una forma de implementar la característica 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í, se acaba de nombrar a usted mismo como jefe y debe esperar solicitudes de listas de nombres aceptables.

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

Ewan
fuente
2
"Creo que la clave para codificar las revisiones es acordar las reglas antes de la revisión". Este sería el caso ideal. En la práctica, no podemos suponer que todos los desarrolladores conocen todas las reglas. Las revisiones de código son útiles para difundir este conocimiento y explicar las reglas con ejemplos prácticos. Tal vez ese sea uno de los mayores beneficios de hacer revisiones de código ...
Frank Puffer
escriba las reglas en el documento de estándares de codificación y entregue a los nuevos desarrolladores
Ewan
1
Hemos escrito estándares de codificación y estos se entregan a los nuevos desarrolladores. Esto funciona la mayor parte del tiempo, pero a veces hay malas interpretaciones. Además de los estándares de codificación escritos, hay principios generales como SECO o SÓLIDO que también se abordan en las revisiones de código. Esperamos un conocimiento básico sobre estos de nuestros desarrolladores y también hacemos algunos entrenamientos internos para mejorarlo. Este es un proceso continuo y las revisiones de código son parte de él.
Frank Puffer
0

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 variables / funciones en los que no se puede pensar en algo mejor, por lo general, reconozco que no tengo una mejor idea y termino el comentario en forma de una pregunta abierta en caso de que alguien pueda . De esa manera, si a nadie se le ocurre una mejor opción, la revisión no se está retrasando.

Para el ejemplo que diste en tu pregunta, con la clase mal diseñada. Dejaría algunos comentarios que, aunque parece que puede ser excesivo, la herencia probablemente sea la mejor manera de abordar el problema que el código está tratando de resolver, y dejarlo así. Trataría de expresar de una manera que indique que no es un show-stopper y depende del criterio del desarrollador si solucionarlo 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 expertos para que aclaren si hay una razón por la que se hace de la manera en que está.

Eric Hydrick
fuente
0

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

La comunicación escrita conduce a grandes cantidades de tiempo perdido, así como a resentimientos y malentendidos.

Cara a cara, el ancho de banda es mucho mayor, y uno tiene el canal lateral emocional para prevenir la hostilidad.

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

John Lawrence Aspden
fuente
Esto no parece ofrecer nada sustancial sobre los puntos hechos y explicados en las 11 respuestas anteriores
mosquito