El colega bloquea la solicitud de cambio en la revisión por pares debido a errores percibidos en el código, pero las mejoras sugeridas no funcionan

En la empresa para la que trabajo, las solicitudes de cambio pasan por varios pasos, incluido un paso de desarrollo, un paso de revisión por pares y un paso de prueba. En esta solicitud de cambio en particular, se me asignó como desarrollador y se asignó a un colega desarrollador como revisor.

Este colega es el que me entrenó originalmente. Ha estado en la empresa durante más de 10 años, mientras que yo me uní hace un año, justo después de la universidad. Tiene más años de experiencia como desarrollador que yo. A pesar de eso, sigue siendo mi compañero ya que ambos somos desarrolladores de "nivel medio" (ya no junior pero tampoco senior).

Esta solicitud de cambio en particular implica una función antigua y complicada. Está mal escrito de acuerdo con cualquier estándar de código, pero funciona. Me tomó algunas horas incluso encontrar lo que necesitaba cambiar. El cliente deseaba un orden de evaluación diferente para determinar el valor inicial de un campo (por ejemplo, comprobar dirección.cliente antes que dirección.deudor en lugar de al revés).

Mi colega notó que estaba tardando mucho y miró el código fuente conmigo. Señaló un código y dijo: "Tienes que borrar eso". Tenía mis dudas al respecto, pero lo intenté de todos modos. Sin embargo, al intentarlo me di cuenta de dónde estaba el problema. Al final, tuve que cambiar dos declaraciones.

Documenté el cambio, lo probé y funcionó según lo previsto. Reenvié la solicitud de cambio a mi gerente, quien verificó que funcionaba y la reenvió al revisor. Lo recibí unos minutos después con los comentarios "eso no funcionará, cambiaste el código incorrecto" y "no hiciste lo que dije".

Dije que probé lo que dijo, pero no funcionó. El código al que se refirió no estaba relacionado con la solicitud de cambio, excepto que estaban haciendo algo con este mismo campo. Su respuesta fue que lo que dijo fue solo una pista, porque necesito poder resolver las cosas por mi cuenta.

Ahora se niega a pasar la solicitud de cambio a los evaluadores hasta que corrija mi error. Le pedí más detalles y sigue diciendo que necesito resolverlo por mi cuenta, porque necesito aprender. Necesito comenzar por borrar el código que menciona, según él. Dado que ese código no tiene nada que ver con la solicitud de cambio en cuestión, soy muy reacio. Le pregunté a nuestro gerente y me dijo que presentara mi argumento al revisor, ya que el gerente no sabe de programación. Intenté eso, pero el crítico es inflexible en su punto y me despide con "solo haz lo que dije".

En parte debido a la antigüedad de este código, no hay pruebas unitarias y no se pueden escribir para ellas sin reescribir la mitad del código base.

¿Cómo soluciono esta situación?

Solo por curiosidad, ¿es un departamento de TI corporativo o una tienda de desarrollo de TI?
¿Puedes pasar la solicitud de cambio a los probadores sin su ayuda?
Hacer que los compañeros revisen el código es una pérdida de tiempo. Si el código funciona, cualquier cosa que pueda estar mal caerá en la categoría de juicio y opinión, en lugar de en un hecho objetivo, y eso lo convierte en un trabajo para la gerencia.

Respuestas (9)

Puede que tenga razón en que el código necesita ser reparado, pero lo está haciendo de manera incorrecta para cualquier revisión de código que haya visto. Las revisiones de código deberían fallar en puntos direccionables muy específicos, por ejemplo:

  1. Sangría inconsistente en ScoreHandler.cpp, líneas 45-50. Viola la guía de estilo #8.
  2. getConsistentReview()en Foo.java no es seguro para subprocesos como se requiere, sincronizando en el bloqueo incorrecto.
  3. getFoo()en Foo.java está iterando sobre una lista para filtrar los resultados. Dado que nuestro nivel de fuente ahora es 8, esto debería usar la API de flujo para filtrar en su lugar, que es mucho más conciso.

Parece que tienes a alguien que está (mal) tratando de guiarte al hacer que "arregles" el código de la manera que él quiere, sin decirte exactamente por qué está roto o qué tiene de malo. Ni siquiera está claro si la razón por la que quiere que lo cambies es estilística o funcional.

Podrías negarte a jugar a la pelota y empujarlo hacia él. Si cree que funcionará en general o si causará más problemas depende de la cultura.

Si desea un enfoque más suave, intente responder con más detalles de lo que intentó, por qué no funcionó, e indique exactamente lo que necesita para continuar:

Hola x,

Anteriormente intenté eliminar yel código como sugirió, pero eso hace zque suceda, lo que hace que la función falle (estas) pruebas en (estos) casos. No tengo muy claro el requisito aquí: ¿el cambio que está pidiendo es estilístico o funcional? Si es estilístico, ¿podría señalar las pautas de estilo específicas que infringe el código y, si es funcional, podría proporcionarme un caso de prueba que falle?

No es irrazonable pedir nada de lo anterior en una revisión de código; es más irrazonable que no se haya proporcionado para empezar.

Si sigue diciendo "haz lo que intenté" después de eso, puedes responder con "Como antes, me temo que no funcionó y necesitaré muchos más detalles para resolver el problema". Si alguien te critica por tomarte mucho tiempo, muéstrale el rastro en papel; Debería quedar claro bastante rápido que él es el que arrastra los pies y se niega a jugar a la pelota.

Actualización: acabo de probar esto y funcionó. Terminó viniendo a mi escritorio y me ayudó a mejorar mi código. No estaba mal después de todo, pero podría ser mejor. Su solución no estaba completa, pero no estaba tan mal como pensaba. Supongo que ambos estábamos equivocados.
@ Belle-Sophie Quizás vio esta publicación y se dio cuenta de que estás realmente atascado.
@Belle-Sophie Genial para escuchar, feliz de ayudar.
@Belle-Sophie: creo que es una buena lección para el futuro; sí, las revisiones de código generalmente se llevan a cabo de forma asincrónica a través de herramientas... sin embargo, cada vez que se sienta confundido por una pregunta/respuesta en la revisión de código, podría ser mejor cambiar a comunicación telefónica/cara a cara para aclarar malentendidos rápidamente :)
Aún así, ¿hubo algún beneficio en sus cambios? ¿O fue simplemente una pérdida de tiempo para todos?
Otra posibilidad es que el revisor haya visto casos de uso adicionales que no se han solucionado. Estos pueden o no ser errores adicionales, según los detalles del informe de errores y cómo se gestionan los errores. De cualquier manera, el enfoque todavía funciona.
@Belle-Sophie: actualice su pregunta con eso, ya que prácticamente invalida toda la premisa de su pregunta
OPL "El código al que se refirió no estaba relacionado con la solicitud de cambio, excepto que estaban haciendo algo con este mismo campo ". [énfasis mío]. ¡Así que está relacionado!

En el flujo de trabajo de mi empresa, eso sería fácil.

Marque la solicitud de cambio como "no se corregirá" con un comentario "solución correcta rechazada por el revisor", luego asígnele la solicitud de cambio. Obviamente, él sabe cómo arreglarlo, por lo que debería ser un trabajo de cinco minutos para él.

Y de hecho, si estaba en lo correcto, entonces eso sería lo correcto. No tiene sentido perder el tiempo. Así que no hay nada de lo que pueda quejarse.

Con un código antiguo y oscuro, pueden suceder dos cosas: lo dejo sin cambios o lo mejoro y asumo la responsabilidad si se rompe. Lo único que no va a suceder es que le ordene a otra persona que haga esos cambios. Y definitivamente no durante una revisión de código.

Intenté eso. Simplemente me lo devuelve.
Ahí es donde se lo lleva a su gerente. Su gerente no sabe de programación, pero dice que las sugerencias de sus colegas no funcionan, él dice que funcionan, por lo que en esa situación sería obvio para su jefe que debería hacer el trabajo.
@Belle-Sophie Entonces... ¿él insiste en que hagas su cambio a tu nombre? Eso no es muy críquet. Sugiero agregar este detalle a tu pregunta ya que resalta la mentalidad del chico.
@rath ¿Puede explicar su uso de la palabra 'cricket' en este contexto? Me temo que estoy completamente perdido.
@Two-BitAlchemist es un término en inglés que significa "simplemente no es la forma correcta de hacer las cosas con honor", de los juegos de cricket donde siempre se esperaba que los jugadores jugaran con justicia y conducta caballerosa.
Sí. ¿Sabes lo que no es "cricket"? Respondiendo a la idiotez con más idiotez. Los comentarios pasivo-agresivos en el código (y las revisiones de código) son una gran señal de alerta sobre la idoneidad de una persona para la colaboración. no lo hagas
"No se soluciona" suele ser un motivo cercano en un ticket. ¿Por qué cerraría el ticket antes de reasignarlo?
Así que si transmites problemas no sabes cómo hacerlo; ¿cómo vas a aprender a hacerlos?

Es posible que esto no se aplique a usted por razones técnicas u organizativas, pero la respuesta general es:

Hacer una prueba unitaria

Una prueba unitaria es prueba de que se ha corregido un error. Cada vez que corrige un error, o cada vez que necesita demostrar un error, las pruebas unitarias ahorran muchos argumentos. Vea si hay una persona de control de calidad con la que pueda hablar y solucionar este problema.

Recuerda que no rechazó tu cambio porque no funcionó, lo rechazó porque no era su cambio. Tener una unidad de prueba que lo respalde fortalece su posición.


Específicamente para tu situación: recuerda que si él quiere que hagas lo que dijo, debe hacerlo él mismo. Es tu tarea y tienes que hacer lo que dices. Esa es su mentalidad, ahora para el diálogo.

Llame al desarrollador y hable con él. Demuestre que su solución funciona, y si le preguntan por qué no hizo lo que dijo, puede decir algo como esto:

Esto no es exactamente lo que dijiste, pero funciona y soluciona el problema. Entiendo esta solución en mi cabeza, la otra forma probablemente funcionaría también, pero me habría llevado mucho más tiempo descubrirla. Entonces, creo que podemos marcar este como Aprobado en la revisión del código.

Me gusta esta respuesta. Desafortunadamente, no funcionará para mí (actualicé la pregunta), pero es una buena respuesta para las personas con un problema similar que en realidad es comprobable.
@Belle-Sophie Pensé que tu situación podría ser un poco diferente :) El tipo suena como un colega que no me gustaría tener
Parece que tiene una solución, pero para la próxima persona en esta situación, aunque generalmente no es práctico probar por completo una función heredada gruesa, casi siempre es posible factorizar cada pieza individual de lógica en una función comprobable, luego encadenarlos juntos, y a menudo el hecho de hacer esto hace que la idea se encienda en torno a dónde está el error.

Estoy de acuerdo con berry120. Sin embargo, sus notas de revisión fueron

"eso no funcionará, cambiaste el código equivocado" y "no hiciste lo que dije"

Mi sugerencia:

(No dedique más de medio día más o menos a esto. Documente de manera concisa el comportamiento del código relevante, los errores/advertencias, las acciones que tomó y las razones para hacerlo).

  • asegúrese de que su nuevo código funcione en la versión X (sin errores ni advertencias) de acuerdo con la solicitud del cliente
  • crear una bifurcación o una versión del código antes de sus cambios
  • haz exactamente lo que indican las notas (intenta corregir los errores, si los hay), esta es ahora la versión Y
  • si estos cambios no abordan la solicitud del cliente, tome nota especial de esto e intente incluir la Versión X en la Versión Y
  • informe (envíe un correo electrónico) a su revisor y gerente:

Creo que he terminado de implementar los cambios solicitados por el cliente en la Versión X.
Para abordar las notas de revisión, tomé los siguientes pasos adicionales para la Versión Y.

Enumere aquí su progreso y hallazgos, incluya su documentación de este proceso (sea breve, de 5 a 10 líneas como máximo).
Si ayuda a la legibilidad, también puede tener esta lista al final del correo electrónico y consultarla en consecuencia.

Si las notas de revisión no respondieron a la solicitud del cliente:
(a continuación se explicará su retraso e incumplimiento inicial de las notas)

Por lo que pude ver (vea mi informe arriba/abajo), las notas de revisión por sí solas no abordarían la solicitud de los clientes y, además, implementé la Versión X.

Escribí la Versión X primero para asegurar que la solicitud de los clientes se abordara antes de agregar otros cambios en el código.

Si la Versión Y aún tiene errores que no pudo corregir:
(Probablemente serán las notas de revisión o la adición de la Versión X a las notas de revisión; ¡ajuste la redacción a continuación en consecuencia!)

Estoy feliz de continuar trabajando en la Versión Y.
Desafortunadamente, integrar las notas de revisión con la Versión X (los cambios solicitados por el cliente en funcionamiento) está resultando más complejo que enviar solo la Versión X y necesitaré más tiempo asignado.

No estoy seguro de incluir lo siguiente para apaciguar un poco a su revisor y, al mismo tiempo, señalarle a la gerencia que sus cambios están causando los errores. También podría hacer que la gerencia interprete que aún necesita la supervisión de los revisores.

inserte el nombre del revisor , si tiene un momento, agradecería mucho su orientación con respecto a la motivación detrás (o posiblemente menos inquisitivo: "la naturaleza de") los cambios en su código y los errores resultantes que obtengo para la Versión Y.

Obviamente, use los números de versión reales en lugar de X / Y (;

EDITAR:

Para hacer mi respuesta más legible y concisa, la cambié. Por favor, deje comentarios si esto es mejor. Gracias.

Me sucedió algo similar: mi primer trabajo después de la universidad, un desarrollador senior tuvo que hacer un cambio para respaldar algo en lo que estaba trabajando, pero el cambio que hizo no funcionó. Lo que terminó funcionando para mí, y lo que te sugiero que hagas es lo siguiente:

Involucre a otro desarrollador senior, programe una reunión con ambos y explique su punto de vista. Si es posible, demuestre las pruebas que realizó. Entonces permítale explicar su enfoque. Averigüe cuál es la mejor solución y hágalo.

Asegúrate de no dar la impresión de ser un adversario, algo así como:

Hola Mike. Parece que seguimos hablando entre nosotros sobre el error #123. ¿Tienes 30 minutos el lunes para repasarlo? Haré que Jim se una a nosotros ya que ha estado trabajando en eso últimamente.

En mi último trabajo teníamos un proceso de revisión por pares. La gerencia nos dijo que no le digamos a la persona cómo resolverlo, sino que solo le demos sugerencias y orientación. Como tal, he estado en el extremo opuesto de la situación OP en la que soy el desarrollador principal que entrena a una nueva persona que "solucionó" un problema, pero los cambios no tienen en cuenta ciertas situaciones únicas.

La forma en que lo resolví fue explicando directamente las situaciones únicas y la persona pudo codificar ese comportamiento, pero el código general era propenso a errores porque era un repositorio heredado. Mi idea es que deberías preguntarle al desarrollador si hay algún tipo de situación única que no te das cuenta y que él puede prever. Si no hay ningún caso fuera de lugar, y él dice que está "roto" porque no lo seguiste, entonces vuelve a ponérselo. Escriba en el ticket que el código funciona, fue probado por la administración y, lamentablemente, no ve ninguna situación en la que el código sea incorrecto.

Suena extraño que se niegue a tu dosis y se niegue a darte detalles. Así que es hora de escalar un poco. Eso significa enviar el correo electrónico que le envió al líder de su equipo diciendo que no comprende por qué se rechazó su solución y que no comprende la funcionalidad lo suficiente como para descubrir lo que dice su colega, por lo que solicita ayuda del TL.

Expresaría esto en términos de "funcionalidad antigua de la que no comprende la historia o las implicaciones", pero es posible que desee refinar eso, pero va a la TL "para obtener ayuda con esta tarea" sabiendo que él lo hará. ver el rastro de cambios en el código (es decir, "muéstrame lo que has intentado hasta ahora") que resaltará la falta de cooperación de tu colega. Eso debería ser suficiente para que esta tarea tenga cierta visibilidad en todo el equipo y un enfoque en arreglarla, sin impedimentos artificiales e inútiles de su colega (que está actuando de manera poco profesional).

En ningún momento necesita quejarse, o incluso mencionar la falta de ayuda del colega, eso será claro.

Si no tiene un líder de equipo, vuelva a colocarlo en la cola de tareas pendientes y mencione (¿en una reunión de progreso?) que no comprende el problema lo suficiente como para solucionarlo más allá de lo que ya ha intentado. Lo dejo para que lo arregle alguien con más experiencia.

Su colega le ha dado algunos consejos y le ha dicho que necesita aprender. Intentan ser útiles y brindarle las herramientas para solucionar problemas similares por su cuenta en el futuro.

Probó sus sugerencias, pero por una razón desconocida no funcionaron (o las sugerencias estaban equivocadas, no las entendió o alguna combinación de las dos). No puede resolver ninguno de esos problemas por su cuenta, por lo que deberá volver al revisor para obtener ayuda. Cuando haga eso, debe explicarle al revisor lo que intentó y por qué no funcionó.

Esto demuestra que te estás esforzando tú mismo, en lugar de confiar en su experiencia para solucionar el problema (lo que podría convertirte en un "vampiro de ayuda"). Si sus sugerencias fueron incorrectas, se darán cuenta en este punto y luego podrá discutir otras soluciones. Si no entendió bien sus sugerencias, obtendrán una mejor comprensión de su forma de pensar y podrán reformular sus sugerencias para alejarlo de su malentendido.

Leyendo los comentarios, la respuesta que parece que usaste en realidad es hablar con la persona sobre tus dificultades y sus expectativas; y luego averiguar por qué lo que esperaban que funcionara no funcionó.

Independientemente de las herramientas que utilice su empresa para realizar un seguimiento de los problemas, suelen ser herramientas para el mantenimiento de registros y la comunicación; y si bien tienen su lugar creo que esto demuestra que la comunicación verbal no puede ser reemplazada.

Para ser honesto, este debería ser el primer puerto de escala para muchos problemas relacionados con el trabajo; ya que es sólo parte de cómo trabajar en equipo.