¿Cómo manejo el desacuerdo en una revisión de código con respecto a un caso marginal poco probable?

Estoy trabajando en una startup de robótica en un equipo de cobertura de ruta y después de enviar una solicitud de extracción, mi código se revisa.

Mi compañero de equipo, que ha estado en el equipo durante más de un año, ha hecho algunos comentarios a mi código que sugieren que trabajo mucho más de lo que creo que es necesario. No, no soy un desarrollador perezoso. Me encanta el código elegante que tiene buenos comentarios, nombres de variables, sangría y maneja los casos correctamente. Sin embargo, tiene un tipo diferente de organización en mente con el que no estoy de acuerdo.

Proporcionaré un ejemplo:

Había pasado un día escribiendo casos de prueba para un cambio en un algoritmo de búsqueda de transición que hice. Me sugirió que manejara un caso oscuro que es extremadamente improbable que suceda; de hecho, no estoy seguro de que sea posible que ocurra. El código que hice ya funciona en todos nuestros casos de prueba originales y en algunos nuevos que encontré. El código que hice ya pasa nuestras más de 300 simulaciones que se ejecutan todas las noches. Sin embargo, manejar este oscuro caso me llevaría 13 horas que podrían gastarse mejor intentando mejorar el rendimiento del robot. Para ser claros, el algoritmo anterior que habíamos estado usando hasta ahora tampoco manejó este oscuro caso y ni una sola vez, en los 40k informes que se han generado, ha ocurrido alguna vez. Somos una startup y necesitamos desarrollar el producto.

Nunca he tenido una revisión de código antes y no estoy seguro si estoy siendo demasiado discutidor; ¿Debería estar callado y hacer lo que él dice? Decidí mantener la cabeza baja y simplemente hacer el cambio, aunque no estoy de acuerdo en que fue un buen uso del tiempo.

Respeto a mi compañero de trabajo y lo reconozco como un programador inteligente. Simplemente no estoy de acuerdo con él en un punto y no sé cómo manejar el desacuerdo en una revisión de código.

Siento que la respuesta que elegí cumple con este criterio de explicar cómo un desarrollador junior puede manejar el desacuerdo en una revisión de código.

un caso oscuro que es extremadamente improbable que suceda; de hecho, no estoy seguro de que sea posible

No tener comportamientos no probados en el código puede ser muy importante. Si se ejecuta un fragmento de código, por ejemplo, 50 veces por segundo, una probabilidad de uno en un millón ocurrirá aproximadamente cada 5,5 horas de tiempo de ejecución. (En su caso, las probabilidades parecen ser más bajas).

Puede hablar sobre las prioridades con su gerente (o quien sea una persona de más alto rango a cargo de la unidad en la que trabaja). Comprenderá mejor si, por ejemplo, trabajar en el rendimiento del código o si el código es a prueba de balas es la máxima prioridad, y cuán improbable puede ser ese caso de esquina. Su revisor también puede tener una idea sesgada de las prioridades. Habiendo hablado con la persona a cargo, será más fácil (des) aceptar las sugerencias de sus revisores y tendrá algo a lo que referirse.

Siempre es una buena idea tener más de un revisor. Si su código solo es revisado por un colega, pídale a otra persona que conozca ese código, o la base de código en general, que eche un vistazo. Una segunda opinión, nuevamente, lo ayudará a estar más fácilmente (des) de acuerdo con las sugerencias del revisor.

Tener una serie de comentarios recurrentes durante varias revisiones de código generalmente apunta a que algo más grande no se comunica claramente, y los mismos problemas surgen una y otra vez. Intente descubrir esa cosa más grande y discútala directamente con el revisor. Haga suficiente por qué preguntas. Me ayudó mucho cuando comencé a practicar revisiones de código.

Puedo darle un ejemplo de un caso de esquina que nunca podría ocurrir que causó un desastre.

Cuando se desarrolló el Ariane 4, los valores de los acelerómetros laterales se escalaron para ajustarse a un entero con signo de 16 bits y porque la salida máxima posible de los acelerómetros nunca podría exceder 32767 y el mínimo nunca podría caer por debajo. 32768 "no había necesidad de la sobrecarga de la verificación de rango". En general, se supone que todas las entradas deben verificarse en el rango antes de cualquier conversión, pero en este caso eso estaría tratando de detectar un caso de esquina imposible.

Varios años después, el Ariane 5 se estaba desarrollando y el código para escalar los acelerómetros laterales se reutilizó con pruebas mínimas, ya que se "probó en uso". Desafortunadamente, el cohete más grande podría esperar aceleraciones laterales más grandes, por lo que los acelerómetros se actualizaron y podrían producir valores de flotación de 64 bits más grandes .

Estos valores más grandes "envueltos" en el código de conversión, recuerde que no se verificaron los rangos, y los resultados en el primer lanzamiento en 1996 no fueron buenos. Le costó a la compañía millones y causó una gran interrupción en el programa.

El punto que estoy tratando de hacer es que no debe ignorar los casos de prueba como nunca suceden, extremadamente improbable, etc.: los estándares para los que codificaban exigían la verificación del rango de todos los valores de entrada externos. Si eso hubiera sido probado y manejado, entonces el desastre podría haberse evitado.

Tenga en cuenta que en Ariane 4 esto no fue un error (ya que todo funcionó bien para todos los valores posibles) , fue un incumplimiento de los estándares. Cuando el código se reutilizó en un escenario diferente, falló catastróficamente, mientras que si se hubiera recortado el rango de valores, probablemente habría fallado con gracia, y la existencia de un caso de prueba para esto podría haber desencadenado una revisión de los valores. También vale la pena señalar que, si bien los codificadores y evaluadores recibieron algunas críticas de los investigadores luego de la explosión, la administración, el control de calidad y el liderazgo quedaron con la mayor parte de la culpa.

Aclaración

Si bien no todo el software es crítico para la seguridad, ni tan espectacular cuando falla, mi intención era resaltar que las pruebas "imposibles" aún pueden tener valor. Este es el caso más dramático que conozco, pero la robótica también puede producir algunos resultados desastrosos.

Personalmente, diría que una vez que alguien ha resaltado un caso de esquina para el equipo de prueba, se debe realizar una prueba para verificarlo. El líder del equipo de implementación o el gerente del proyecto pueden decidir no tratar de abordar las fallas encontradas, pero deben tener en cuenta que existen deficiencias. Alternativamente, si la prueba es demasiado compleja o costosa de implementar, se puede plantear un problema en cualquier rastreador que esté en uso y / o en el registro de riesgos para dejar en claro que este es un caso no probado, que puede ser necesario abordar antes de un cambio de uso o evitar un uso inapropiado.

Como no se manejó antes, está fuera del alcance de su esfuerzo. Usted o su colega pueden preguntarle a su gerente si vale la pena cubrir este caso.

Con algoritmos complejos, es muy difícil demostrar que ha pensado en cada caso de prueba que surgirá en el mundo real. Cuando dejas intencionalmente un caso de prueba roto porque no aparecerá en el mundo real, potencialmente estás dejando otros casos de prueba rotos que aún no has pensado.

El otro efecto que ocurre a menudo es cuando maneja casos de prueba adicionales, su algoritmo necesariamente se vuelve más general, y debido a eso, encuentra formas de simplificarlo y hacerlo más robusto para todos sus casos de prueba. Esto le ahorra tiempo en mantenimiento y solución de problemas en el futuro.

Además, hay innumerables casos de errores de "esto nunca debería suceder" en la naturaleza. Esto se debe a que su algoritmo podría no cambiar, pero sus entradas podrían cambiar en el futuro cuando nadie recuerde este caso de uso no manejado. Es por eso que los desarrolladores experimentados manejan este tipo de cosas cuando están frescos en sus mentes. Vuelve a morderte más tarde si no lo haces.

Esta no es una cuestión técnica, sino una decisión de estrategia comercial. Observa que la solución sugerida es para un caso muy oscuro que casi nunca sucederá. Aquí hace una gran diferencia si está programando un juguete o si está programando, por ejemplo, equipo médico o un dron armado. Las consecuencias de un mal funcionamiento raro serán muy diferentes.

Al realizar revisiones de código, debe aplicar una comprensión básica de las prioridades comerciales al decidir cuánto invertir en el manejo de casos raros. Si no está de acuerdo con su colega en su interpretación de las prioridades del negocio, es posible que desee conversar con alguien sobre el lado comercial de las cosas.

Las revisiones de código no se tratan únicamente de la corrección del código. En realidad, eso está bastante abajo en la lista de beneficios, detrás del intercambio de conocimientos y de ser un proceso lento pero constante hacia la creación de un consenso de diseño / estilo de equipo.

Como está experimentando, lo que cuenta como "correcto" a menudo es discutible, y cada uno tiene su propio ángulo sobre lo que es. En mi experiencia, el tiempo y la atención limitados también pueden hacer que las revisiones de código sean altamente inconsistentes: el mismo problema puede ser resuelto o no dependiendo de diferentes desarrolladores y en diferentes momentos por el mismo desarrollador. Revisores en la mentalidad de "¿qué mejoraría este código?" a menudo sugerirán cambios que no agregarían a sus propios esfuerzos de forma natural.

Como codificador experimentado (más de 15 años como desarrollador), a menudo los codificadores me revisan con menos años de experiencia que yo. A veces me piden cambios con los que estoy ligeramente en desacuerdo o que creo que no son importantes. Sin embargo, todavía hago esos cambios. Lucho en las batallas por los cambios solo cuando siento que el resultado final causaría una debilidad en el producto, donde el costo de tiempo es muy alto, o donde un punto de vista "correcto" puede ser objetivo (por ejemplo, el cambio que se solicita ganó " No funciona en el idioma que estamos usando, o un punto de referencia muestra que una mejora de rendimiento afirmada no es una)

Así que sugiero elegir tus batallas con cuidado. Dos días codificando un caso de prueba que crees que no es necesario, probablemente no valga la pena el tiempo / esfuerzo para luchar. Si está utilizando una herramienta de revisión, como las solicitudes de extracción de GitHub, entonces tal vez comente allí sobre el costo / beneficio que percibe, para tomar nota de su objeción, pero acepta seguir haciendo el trabajo. Esto cuenta como un suave retroceso, por lo que el revisor sabe que están llegando a un límite y, lo que es más importante, incluya su justificación para que casos como este se puedan escalar si entran en un punto muerto. Desea evitar la escalada de desacuerdos por escrito (no desea tener una discusión estilo foro de Internet sobre las diferencias de trabajo), por lo que puede ser útil analizar el tema primero y registrar un resumen justo del resultado de la discusión,una discusión amistosa decidirá por los dos de todos modos).

Después del evento, este es un buen tema para la revisión del sprint o las reuniones de planificación del equipo de desarrollo, etc. Preséntelo de la manera más neutral posible, por ejemplo, "En la revisión del código, el Desarrollador A identificó este caso de prueba adicional, tomó dos días adicionales para escribir, ¿El equipo cree que la cobertura adicional se justificó a este costo? " - este enfoque funciona mucho mejor si realmente hace el trabajo, ya que lo muestra de manera positiva; usted ha hecho el trabajo y solo quiere encuestar al equipo por aversión al riesgo versus desarrollo de características.

Le aconsejaría al menos afirmar contra el oscuro caso. De esa forma, no solo los futuros desarrolladores ven que usted decidió activamente en contra del caso, sino que con un buen manejo de fallas, que ya debería estar en su lugar, esto también sorprendería.

Y luego, haga un caso de prueba que afirme esa falla. De esta manera, el comportamiento está mejor documentado y se mostrará en las pruebas unitarias.

Obviamente, esta respuesta supone que su opinión de que incluso es "extremadamente improbable si es posible" es correcta y no podemos juzgar eso. Pero si es así, y su colega está de acuerdo, entonces una afirmación explícita en contra del evento debería ser una solución satisfactoria para ambos.

Como parece ser nuevo allí, solo hay una cosa que puede hacer: consultar con el líder del equipo (o el líder del proyecto). 13 horas es una decisión comercial; para algunas empresas / equipos, mucho; para algunos nada. No es tu decisión, todavía no.

Si el plomo dice "cubre ese caso", está bien; si dice "no, joder", está bien, su decisión, su responsabilidad.

En cuanto a las revisiones de códigos en general, relájate. Que le devuelvan una tarea una o dos veces es perfectamente normal.

Una cosa que no creo que haya visto abordada en especie, aunque fue mencionada en la respuesta de @SteveBarnes:

¿Cuáles son las repercusiones de un fracaso?

En algunos campos, una falla es un error en una página web. Una PC con pantallas azules y reinicios.

En otros campos, es vida o muerte: el auto sin conductor se traba. El marcapasos médico deja de funcionar. O en la respuesta de Steve: las cosas explotan y se pierden millones de dólares.

Hay un mundo de diferencia en esos extremos.

Si vale la pena o no 13 horas para cubrir un "fracaso" en última instancia, no debería depender de usted. Debería depender de la gerencia y los propietarios. Deben tener una idea de la imagen más grande.

Debería poder adivinar lo que valdrá la pena. ¿Su robot simplemente disminuirá la velocidad o se detendrá? Rendimiento degradado? ¿O una falla del robot causará daños monetarios? ¿Pérdida de vida?

La respuesta a ESA pregunta debería conducir la respuesta a "¿Vale la pena 13 horas de tiempo de las empresas". Aviso: dije el tiempo de las empresas. Pagan las cuentas y finalmente deciden lo que vale la pena. Su gerencia debe tener la última palabra de cualquier manera.

¿Tal vez hablar con la persona responsable de priorizar el trabajo? En el inicio podría ser CTO o propietario del producto. Podría ayudar a encontrar si se requiere este trabajo adicional y por qué. También puede traer sus preocupaciones durante las paradas diarias (si las tiene).

Si no hay una responsabilidad clara (por ejemplo, el propietario del producto) por el trabajo de planificación, trate de hablar con las personas que lo rodean. Más tarde, podría convertirse en un problema que todos empujen el producto en la dirección opuesta.

La mejor manera de manejar el desacuerdo es la misma, independientemente de si usted es un desarrollador junior o un desarrollador senior, o incluso un CEO.

Actúa como Columbo .

Si nunca has visto un Columbo, fue un espectáculo fantástico. Columbo era un personaje muy modesto: la mayoría de la gente pensaba que estaba un poco loco y que no valía la pena preocuparse. Pero al parecer humilde y solo pedirle a la gente que explique que fue capaz de atrapar a su hombre.

Creo que también está relacionado con el método socrático .

En general, desea hacerse preguntas a usted mismo y a los demás para asegurarse de tomar las decisiones correctas. No desde una posición de "Tengo razón, estás equivocado", sino desde una posición de descubrimiento honesto. O al menos lo mejor que puedas.

En su caso, tiene dos ideas aquí, pero tienen básicamente el mismo objetivo: mejorar el código.

Tiene la impresión de que escatimar en la cobertura del código para un caso potencialmente improbable (¿imposible?) A favor del desarrollo de otras características es la mejor manera de hacerlo.

Su compañero de trabajo tiene la impresión de que tener más cuidado con las vitrinas es más valioso.

¿Qué ven ellos que tú no ves? ¿Qué ves que ellos no ven? Como desarrollador junior, en realidad estás en una excelente posición porque, naturalmente, deberías hacer preguntas. En otra respuesta, alguien menciona cuán sorprendentemente probable es un caso de esquina. Entonces, puede comenzar con: "Ayúdame a entender, tenía la impresión de que X, Y y Z, ¿qué me estoy perdiendo? ¿Por qué framboozle el widget? Tenía la impresión de que funcionaría bajo circunstancias cromulentas. Will ¿el palo de swizzle en realidad embiggen los cepillos ANZA? "

A medida que cuestione sus suposiciones y sus hallazgos, investigará, descubrirá prejuicios y eventualmente descubrirá cuál es el curso de acción correcto.

Comience con la suposición de que todos en su equipo son perfectamente racionales y que tienen en mente los mejores intereses del equipo y del producto, como usted. Si están haciendo algo que no tiene sentido, entonces debes descubrir lo que no sabes que hacen, o lo que sabes que no saben.

13 horas no es gran cosa, solo lo haría. Recuerda que te pagan por ello. Simplemente tíralo como "seguridad laboral". También es mejor mantener un buen karma entre el equipo. Ahora, si fuera algo que te llevaría una semana o más, entonces podrías involucrar a tu gerente y preguntarle si ese es el mejor uso de tu tiempo, especialmente si no estás de acuerdo.

Sin embargo, parece que necesita influencia en su grupo. Así es como obtienes apalancamiento: pide perdón, no pidas permiso. Agregue cosas al programa como mejor le parezca (dentro del alcance del curso, es decir, asegúrese de que resuelva por completo el problema que el jefe quiere ...), y cuéntele al gerente o sus colegas después del hecho. No les pregunte: "¿Está bien si agrego la función X". En cambio, solo agregue las características que desea personalmente en el programa. Si se molestan con una nueva función o no están de acuerdo, puede eliminarla. Si les gusta, quédense.

Además, cada vez que le pidan que haga algo, haga un esfuerzo adicional y agregue muchas cosas que olvidaron mencionar o cosas que funcionarían mejor de lo que dijeron. Pero no les pregunte si está "bien" hacer un esfuerzo adicional. Solo hazlo y cuéntales de manera casual después de que esté hecho. Lo que estás haciendo es entrenarlos ...

Lo que sucederá será que su gerente lo identificará como un "emprendedor" y comenzará a confiar en usted, y sus colegas comenzarán a verlo como el líder porque está comenzando a ser el propietario del programa. Y luego, cuando sucedan cosas como las que mencionas en el futuro, tendrás más voz porque esencialmente eres la estrella del equipo y tus compañeros retrocederán si no estás de acuerdo con ellos.

La revisión del código tiene varios propósitos. Uno de los que obviamente es consciente es: " ¿Es este código adecuado para su propósito? " En otras palabras, ¿es funcionalmente correcto? ¿Está adecuadamente probado? son las partes no obvias debidamente comentadas; ¿se ajusta a las convenciones del proyecto?

Otra parte de la revisión del código es el intercambio de conocimientos sobre el sistema. Es una oportunidad para que tanto el autor como el revisor aprendan sobre el código modificado y cómo interactúa con el resto del sistema.

Un tercer aspecto es que puede proporcionar una revisión de los problemas que existían antes de realizar cualquier cambio. Muy a menudo, al revisar los cambios de otra persona, detectaré algo que me perdí en una iteración anterior (a menudo algo propio). Una declaración como, "Aquí hay una oportunidad para hacer esto más robusto de lo que era", no es una crítica, ¡y no lo tome como tal!

Mi equipo actual considera la revisión de código no solo como una puerta de entrada o obstáculo que el código debe pasar indemne antes de la confirmación, sino principalmente como una oportunidad para una discusión algo estructurada de un área particular de funcionalidad. Es una de las ocasiones más productivas para compartir información. (Y esa es una buena razón para compartir la revisión en todo el equipo, en lugar de siempre el mismo revisor).

Si percibes las revisiones de códigos como una actividad de confrontación, esa es una profecía autocumplida. Si, por el contrario, los considera la parte más colaborativa de su trabajo, estimularán mejoras continuas en su producto y en la forma en que trabajan juntos. Ayuda si una revisión puede ser clara sobre las prioridades relativas de sus sugerencias x; por ejemplo , hay una diferencia de una milla entre "Me gustaría un comentario útil" y "Esto se rompe si alguna vez es negativo".

Habiendo hecho muchas declaraciones generales arriba, ¿cómo se aplica eso a su situación específica? Espero que ahora sea obvio que mi consejo es responder a la revisión con preguntas abiertas y negociar qué enfoque tiene más valor. Para su caso de ejemplo en el que se sugiere una prueba adicional, entonces algo como "Sí, podemos probar para eso; calculo que tomará <tiempo> en implementarse. ¿Cree que el beneficio vale la pena? ¿Y hay algo más que nosotros Qué puede hacer para garantizar que la prueba no sea necesaria?

Una cosa que me sorprende cuando leo su pregunta: si toma dos días de esfuerzo escribir un nuevo caso de prueba, entonces su nueva prueba es un escenario muy diferente a sus pruebas existentes (en cuyo caso probablemente tenga muchos valor) o ha identificado la necesidad de una reutilización de código mejorada en el conjunto de pruebas.

Finalmente, como un comentario general sobre el valor de las revisiones de código (y como un breve resumen de lo que he dicho anteriormente), me gusta esta declaración, en Maintainers Don't Scale de Daniel Vetter :

Al menos para mí, la revisión no se trata solo de garantizar una buena calidad de código, sino también de difundir el conocimiento y mejorar la comprensión. Al principio, tal vez hay una persona, el autor (y eso no es un hecho), que comprende el código. Después de una buena revisión, debe haber al menos dos personas que lo entiendan completamente, incluidos los casos de esquina.

El código SIEMPRE puede ser mejor.

Si está en una revisión de código y no ve nada que pueda ser mejor o una prueba de unidad que pueda detectar un error, no es el código perfecto, sino el revisor que no está haciendo su trabajo. Si elige mencionar la mejora es una elección personal. Pero casi cada vez que su equipo hace una revisión de código, debería haber cosas que alguien note que podrían ser mejores o que todos probablemente hayan perdido su tiempo.

Dicho esto, ya sea que actúes sobre los comentarios o no, depende de tu equipo. Si sus cambios solucionan el problema o agregan suficiente valor sin cambios para que su equipo los acepte, combínelos y registre sus comentarios en la cartera de pedidos para que alguien los aborde más adelante. Si el equipo encuentra que sus cambios agregan más riesgo o complejidad que valor, entonces debe resolver los problemas en consecuencia.

Solo recuerde que cualquier código tiene al menos un caso perimetral más que se pueda probar y que pueda usar al menos una refactorización más. Esta es la razón por la cual las revisiones de código se realizan mejor en grupo, con todos mirando el mismo código al mismo tiempo. Para que, al final, todos puedan llegar a un consenso sobre si el código en revisión es o no aceptable (como lo es) y agrega suficiente valor para fusionarse con la base de la comunidad o si ciertas cosas deben hacerse antes de que haya suficiente valor para fusionarse .

Como está haciendo esta pregunta, supongo que en realidad no está haciendo "revisiones de código", sino que está creando una solicitud de extracción u otro mecanismo de envío para que otros comenten de una manera no determinista. Así que ahora estás en un problema de gestión y una definición de hecho. Supongo que su administración es indecisa y en realidad no comprende el proceso y el propósito de las revisiones de código y es probable que no tenga una "definición de hecho" (DOD). Porque si lo hicieran, tu DOD respondería explícitamente a esta pregunta y no tendrías que venir aquí y preguntar.

Como lo arreglas? Bueno, pídale a su gerente que le dé un DOD y le diga si debe implementar siempre todos los comentarios. Si la persona en cuestión es su gerente, entonces la respuesta es evidente.

Esta pregunta no trata sobre las virtudes de la programación defensiva, los peligros de los casos de esquina o los riesgos catastróficos de errores en los productos físicos. De hecho, ni siquiera se trata de ingeniería de software .

De lo que se trata realmente es de cómo un practicante junior maneja una instrucción de un practicante senior cuando el junior no puede estar de acuerdo o apreciarla.

Hay dos cosas que debes apreciar sobre ser un desarrollador junior. En primer lugar, significa que si bien es posible que estés en lo correcto y que él esté equivocado, es poco probable que, en el balance de probabilidades. Si su compañero de trabajo está haciendo una sugerencia de la que no puede ver el valor, debe considerar seriamente la posibilidad de que simplemente no tenga la experiencia suficiente para comprenderlo. No tengo ese sentido de esta publicación.

La segunda cosa a apreciar es que su socio principal se llama así porque tiene más responsabilidad. Si un joven rompe algo importante, no tendrá problemas si sigue las instrucciones. Sin embargo, si un senior les permitiera romperlo, al no plantear problemas en la revisión del código, por ejemplo, con bastante razón se sentirían muy molestos.

En última instancia, es un requisito implícito de su trabajo que observe las instrucciones de aquellos en quienes confía la empresa para liderar sus proyectos. ¿Generalmente no puede diferir a las personas mayores cuando hay una buena razón para valorar su experiencia? ¿Tiene la intención de seguir ninguna instrucción que no pueda entender? ¿Crees que el mundo debería detenerse hasta que estés convencido? Estos valores son incompatibles con el trabajo en equipo.

Un punto final. Piense en los proyectos que escribió hace seis meses. Piensa en los proyectos que escribiste en la universidad. ¿Ves lo mal que parecen ahora, todos los errores y el diseño al revés, los puntos ciegos y las abstracciones equivocadas? ¿Qué pasa si te digo que dentro de seis meses contarás los mismos defectos en el trabajo que estás haciendo hoy? ¿Eso ayuda a demostrar cuántos puntos ciegos hay en su enfoque actual? Porque esa es la diferencia que hace la experiencia.

constantemente hace comentarios a mi código que me sugieren hacer mucho más trabajo del necesario.

Puede dar recomendaciones, pero en última instancia, no es su función decidir qué es necesario. Es su trabajo implementar lo que la administración o (en este caso, su revisor) decida que es necesario. Si no está de acuerdo con lo que es necesario demasiado o con demasiada fuerza, es probable que se quede sin trabajo. En consecuencia, es parte de su profesionalismo aceptarlo y estar en paz con él.

Me sugirió que manejara un caso oscuro que es extremadamente improbable que suceda.

Hay otras excelentes respuestas aquí que muestran cómo incluso los que no son errores (es decir, algo que probablemente no puede fallar nunca ) aún deben ser modificados a veces. (por ejemplo, en el caso de construir en el futuro la seguridad del producto, seguir los estándares, etc.) Parte del papel de un gran desarrollador es tener la mayor confianza posible de que su código será robusto en cada situación concebible en todo momento y en el futuro a prueba de fallas, no solo trabajando en situaciones probadas en las condiciones actuales la mayor parte del tiempo

Sugerencias a los revisores de código para aumentar la utilidad comercial de su revisión de código (usted, como OP, debe proponer dicho cambio):

• Marque sus comentarios por tipo. "Crítico" / "Imprescindible" / "Opcional" / "Mejoras sugeridas" / "bueno tener" / "Estoy reflexionando".

  Si esto parece demasiado CDO / anal / complicado, utilice al menos 2 niveles: "Debe solucionarlo para pasar la revisión y poder fusionar su cambio" / "Todos los demás".

Sugerencias para manejar sugerencias de revisión de código que parecen menos críticas de hacer:

• Cree un boleto abierto en su sistema de boletos de elección (su equipo usa uno, ¿con suerte?), Siguiendo la sugerencia

• Ponga el número de ticket como un comentario de respuesta al elemento de revisión de código si su proceso permite respuestas a comentarios como Fisheye o revisiones de correo electrónico.

• Comuníquese con el revisor y pregunte explícitamente si ese elemento es del tipo "debe repararse o no se fusionará / liberará".

  • Si la respuesta es "Sí", pero no está de acuerdo, deje que la persona responsable de la gestión del proyecto (PM, su gerente, etc.) tome una decisión: presente el desacuerdo de manera completa y honesta. No se trata de cuál de ustedes es "correcto" sino de lo que es mejor para el proyecto, por lo que el trabajo de PM / gerente.

Ahora, trate ese ticket como cualquier otra solicitud de desarrollo.

1. Si se decide que es urgente después de la escalada, trátelo como cualquier solicitud de desarrollo urgente. Desprioretizar otro trabajo y trabajar en esto.

2. De lo contrario, trabaje en él según la prioridad que le fue asignada y su ROI (que puede diferir en función de su línea de negocio como se explica en otras respuestas).

No debe escalar esto a la gerencia.

En la mayoría de las empresas, el encargado de la gestión siempre elegirá no escribir esa prueba adicional, no perder el tiempo mejorando marginalmente la calidad del código, no perder tiempo refactorizando.

En muchos casos, la calidad del código depende de las reglas no escritas en el equipo de desarrollo y del esfuerzo adicional que los programadores ponen.

Eres un desarrollador junior y esta es tu primera revisión de código . Debes aceptar el consejo y hacer el trabajo. Solo puede mejorar el flujo de trabajo y las reglas en su equipo si primero las conoce y respeta por un tiempo para que pueda entender por qué están allí. De lo contrario, serás ese chico nuevo que no respeta las reglas y se convierte en el lobo solitario del equipo.

Eres nuevo en el equipo, sigue los consejos que recibes por un tiempo, descubre por qué están allí, no traigas el primer consejo que cuestiones en la reunión de scrum. Las prioridades comerciales reales serán evidentes para usted después de un tiempo sin preguntar (y pueden no ser lo que el gerente le dirá cara a cara).

Es muy importante que cree un código que satisfaga su solicitud de liderazgo / gestión. Cualquier otro detalle sería un "agradable tener". Si es un experto (lea eso: "si no es un desarrollador junior") en su campo, entonces es "elegible" para abordar problemas menores que encuentre en el camino sin consultar a sus líderes cada vez.

Si crees que algo está mal y eres relativamente experto en tu campo, entonces es muy probable que tengas razón .

Aquí hay algunas declaraciones que podrían serle útiles:

• Me pidieron que hiciera X, el revisor de códigos sugiere que también haga Y, ¿debería hacerlo o debería pasar a cosas más importantes?
• Me estás sugiriendo S, entonces, ¿puedes descubrir al menos un caso de prueba que atrape ese comportamiento para que pueda probarlo? Creo que ese código no se llamará.
• ¿Tal vez no deberíamos desarrollar una reserva segura para los casos descubiertos primero? Entonces los atrapamos temprano y los arreglamos sobre la marcha para que podamos concentrarnos en cosas más importantes.
• OK, mientras estoy implementando Y, ¿podría ser tan amable de escribir algunos casos de prueba para que podamos hacer eso de una vez por todas ?
• Me pidieron que hiciera X, y creo que podría hacer Y a menos que haya otras prioridades. La próxima vez, ¿por qué no presenta una solicitud de función en lugar de ponerla como un comentario de revisión en mi código ? Al menos podemos escuchar más opiniones de otros miembros del equipo sobre esa característica antes de implementarla (generalmente cualquier cosa importante debe ser una característica y debe ser manejada por más de una persona; por lo general, la revisión del código debe ser sobre la revisión del código y los enfoques de soluciones; el resto es la corrección de errores y características).

¿Cree seriamente que la actitud de su revisor está dañando el proyecto o cree que tiene razón la mayoría de las veces (excepto que tal vez a veces solo comete errores menores en la evaluación y exagera eso)?

Para mí, suena más como si estuviera escribiendo cosas que no pertenecen a una revisión de código, lo cual es una mala práctica porque hace que sea más difícil para todos rastrear cosas. Sin embargo, no sé qué otros comentarios de revisión hizo, por lo que no puedo decir nada sobre otros casos.

En general, trate de evitar lo siguiente:

• No estás haciendo lo que se te solicitó.
• Hace que su crítico se sienta tonto

Si en realidad está revisando su código, es porque la gerencia confía en él más que en usted.

Cuando hay desacuerdo durante la revisión del código sobre el alcance:

1. Documente el alcance que en realidad está cubierto por el código. A nadie le gustan las sorpresas desagradables.
2. Darse cuenta de que el alcance es una decisión comercial. El alcance ya debe conocerse cuando comience a trabajar en una función, pero si no lo es, siempre puede solicitar una aclaración más adelante.

Si el revisor de código es el que toma las decisiones comerciales, puede cambiar el alcance en cualquier momento, incluso durante la revisión de código, pero no lo está haciendo en su papel de revisor de código.

Si no puede demostrar que el caso límite es imposible, debe asumir que es posible. Si es posible, entonces es inevitable que eventualmente ocurra, y más temprano que tarde. Si el caso límite no se ha producido en las pruebas, puede ser una pista de que la cobertura de la prueba está incompleta.

1. Acepta los comentarios.
2. Antes de realizar cualquier cambio en su código, haga todo lo posible para crear una prueba para el caso límite y ver si puede obtener una falla de prueba (prueba de que el problema existe). Si es imposible crear un caso de prueba de este tipo y obtener una falla de prueba, entonces puede llegar a la conclusión de que el caso límite es realmente imposible (aunque dudaría en sacar esa conclusión).
3. Si puede obtener una falla en la prueba, aplique el cambio de código apropiado para pasar la prueba.

Por el bien del producto, es probable que desee poder producir el caso límite e inducir una falla, de modo que pueda aplicar la solución y tener la confianza de que se ha evitado un posible problema.

El objetivo de las revisiones de código es poner ojos adicionales en el código. Ninguno de nosotros es inmune a errores o descuidos. Es muy común mirar un fragmento de código muchas veces y no notar un error obvio, donde un par de ojos nuevos pueden detectarlo de inmediato.

Como dijiste, has implementado un nuevo algoritmo. No sería prudente sacar conclusiones sobre el comportamiento de su nuevo algoritmo basado en el comportamiento u observaciones sobre su predecesor.

Hay revisores de código que saben lo que están haciendo, y si dicen que algo necesita ser cambiado, entonces necesita ser cambiado, y cuando dicen que algo necesita ser probado, entonces necesita ser probado.

Hay revisores de código que necesitan justificar su propia existencia creando trabajo inútil para otros.

Cuál es el que debe decidir, y cómo manejar el segundo tipo es más una cuestión para el lugar de trabajo.

Si usa scrum, entonces la pregunta es si su trabajo hace lo que se supone que debe hacer (aparentemente lo hace), y puede poner el manejo del caso extremadamente raro y tal vez imposible en el trabajo atrasado, dónde se priorizará y si entra en un sprint, entonces su revisor puede sentirse libre de recogerlo y hacer el trabajo de 13 horas. Si hace el trabajo X y porque hace el trabajo X, se da cuenta de que el trabajo Y también debe hacerse, entonces el trabajo Y no se convierte en parte del trabajo X, es su propio trabajo independiente.