Antipatrones de la revisión de código

• La revisión de código parece una buena idea, ¿no?
  • La revisión de código crea una oportunidad para que dos desarrolladores miren el código, detecten problemas y compartan comprensión sobre cómo evoluciona el proyecto
  • El revisor puede aprender trucos útiles al mirar de cerca el código del autor, o encontrar oportunidades para enseñarle trucos útiles
• Pero así es como la usan los revisores de código del “lado luminoso”. La usan para mejorar el código y elevar colectivamente las habilidades técnicas de los desarrolladores
• La revisión de código también puede ser una herramienta poderosa para objetivos completamente distintos. Si el revisor se pasa al “lado oscuro”, puede usar muchas maneras de obstaculizar o retrasar la mejora del código
  • También puede perseguir otros objetivos personales, como molestar al autor del parche o dejarlo completamente frustrado
• Si hace poco te pasaste al “lado oscuro”, quizá todavía no hayas pensado en todas las posibilidades
  • Por eso este artículo presenta una lista de antipatrones para revisores de código del lado oscuro

[Antipatrones]

The Death of a Thousand Round Trips

• Mientras lees el código, en cuanto detectas algo menor lo señalas en un comentario de revisión y de inmediato dejas de leer
• El desarrollador corrige diligentemente ese detalle menor y envía un parche actualizado
• El revisor empieza a leer esa versión, encuentra otro detalle menor. Podría haberlo mencionado en la primera revisión, pero no lo hizo. Señala ese detalle menor y vuelve a dejar de leer
• Repite esto hasta que el desarrollador pierda la esperanza

The Ransom Note

• Este parche parece especialmente importante para el desarrollador
• Pero para el revisor no es tan importante. Por lo tanto, el revisor está en una posición de poder
• Puede tomar como rehén el cambio que el desarrollador quiere hasta que se completen tareas adicionales que sí le importan al revisor
  • En realidad esas tareas adicionales no tienen por qué estar en el mismo commit, pero son importantes para el revisor

The Double Team

• Un parche, dos revisores
• Cada vez que un revisor exige cambios, el desarrollador obedece. Entonces llega el turno del otro revisor para quejarse
• Los revisores se turnan para presentar requisitos contradictorios
  • Pero siempre le comentan al desarrollador. Evitan discutir directamente entre ellos en el hilo de revisión
• La idea es ver cuántas veces pueden rebotar al desarrollador de un lado a otro hasta que se rinda

The Guessing Game

• Hay un problema, y existen varias formas de resolverlo
• El desarrollador elige una solución y envía el parche
• El revisor critica esa solución específica por razones que no tienen relación con si realmente resuelve el problema
  • Por ejemplo, una supuesta violación de principios de diseño ambiguos o incompatibilidad con futuros planes de desarrollo
• Si la crítica es lo bastante vaga, el desarrollador no tendrá claro cómo modificar el parche actual para resolverla
  • Desde su punto de vista, lo mejor puede ser elegir una solución completamente distinta al problema original e implementarla en su lugar
• Entonces el revisor vuelve a rechazarla de una manera igual de poco útil

The Priority Inversion

• En la primera revisión de código se señalan cosas simples y menores
• Se espera a que el desarrollador las corrija, y luego se plantea un problema grave
  • Hay un problema fundamental que requiere reescribir por completo una parte importante del parche. Eso significa desechar gran parte de los arreglos menores ya realizados
  • Pocas cosas comunican mejor “no queremos tu trabajo y tu tiempo no vale” que hacer que alguien trabaje mucho y luego tirar ese trabajo a la basura
• Eso por sí solo puede bastar para que el desarrollador se rinda

The Late-Breaking Design Review

• Durante semanas o meses ha estado en marcha un trabajo inmensamente complejo en muchos parches separados
• El revisor no está de acuerdo con el diseño de todo ese trabajo, pero no participó en la discusión original o estaba del lado que perdió el debate
• Entonces ahora le piden revisar una parte menor pero importante en medio de ese trabajo, por ejemplo una corrección sencilla para un bug que está bloqueando a muchos desarrolladores. Para el revisor, esta es una oportunidad
• El revisor exige una justificación de todo el diseño del trabajo hecho hasta el momento
• Si el desarrollador solo se encarga de una parte del trabajo total y no conoce la respuesta, eso no es problema del revisor. No va a presionar el botón de “aprobar” hasta quedar satisfecho

The Catch-22

• Si es un parche grande, es demasiado difícil de leer. Exige que se divida en partes más pequeñas y autocontenidas
• Por el contrario, si hay muchos parches pequeños, algunos no tienen sentido por sí solos. Exige que vuelvan a juntarse
• Si parece que algún tipo de trade-off es relevante en un caso concreto, puedes aprovecharlo para encontrar un motivo de queja
  • Por ejemplo, si el código está escrito para que sea fácil de entender, entonces no rendirá bien; y si está bien optimizado, será difícil de leer y de mantener

The Flip Flop

• Hay ciertos tipos de cambios que la gente suele hacer en una misma parte del código
• El revisor ya ha revisado muchas veces este tipo de cambios
• Llega otro cambio. El autor estudió con detalle cambios anteriores, siguió cuidadosamente el patrón existente y eligió a un revisor que parece conocer bien esa área
• El revisor de pronto objeta un aspecto del cambio que antes nunca había cuestionado. Seguir el patrón existente ya no basta
• ¿Qué pasa si el autor muestra cambios anteriores iguales y señala la inconsistencia del revisor?
  • El revisor responde: “Tienes razón. Eso también debería cambiarse”
  • El revisor debe tener cuidado de no ofrecerse a cambiar todos los casos existentes. Con suerte, el desarrollador interpretará esto como una instrucción para cambiarlos él mismo y así el revisor se ahorrará mucho esfuerzo

Pero hablando en serio ...

• Hasta aquí, este artículo era una broma. Tampoco pretende sugerir que los revisores hagan deliberadamente estas malas prácticas
  • La mayoría de las descripciones son exageraciones de cosas que los revisores realmente hacen, o exageraciones de cómo se siente quien envía un parche cuando está frustrado
• ¡De verdad, no hagan esto!
  • Intenten minimizar las idas y vueltas, pidan las reescrituras importantes antes de ponerse a señalar detalles menores y, al criticar un parche, hagan sugerencias constructivas sobre qué versión aceptarían
  • Si tienen opiniones sobre toda la base de código, deberían discutirlo de manera general con todos los desarrolladores, en vez de ponerse quisquillosos al revisar el parche de una sola persona
  • Si hicieron algo así por accidente, tomen conciencia. Reconozcan que sin querer le hicieron la vida más difícil a otro desarrollador, discúlpense e intenten ser más útiles
• El problema de fondo es el abuso de poder
  • Cuando un desarrollador se convierte en revisor del parche de otro, adquiere un poder temporal. El revisor tiene la autoridad de impedir que ese parche se integre
  • El poder conlleva responsabilidad. Debe usarse para el propósito previsto y solo en la medida necesaria
  • La mayoría de los antipatrones, o de las conductas leves que aquí se satirizan, son abusos de poder. El revisor usa como palanca su autoridad temporal sobre el desarrollador para perseguir agendas personales que no tienen nada que ver con mejorar el parche, o incluso van en contra de ello
  • La agenda personal varía según el antipatrón: trabajo no relacionado, preferencias personales de estilo, flojera, resistencia al cambio o antipatía personal hacia quien envió el parche
• Si quien hoy envía el parche actuó así en el pasado cuando fue revisor de código, entonces esa antipatía podría estar justificada. Por eso el poder del revisor debe usarse con cuidado
  • Si los desarrolladores caen en un círculo vicioso de venganza mutua, el proyecto de software está condenado

Revisión de código como gatekeeping

• Hasta aquí el enfoque ha estado en la revisión de código entre pares
  • El revisor y quien envía el parche son colegas, no se supervisan mutuamente ni tienen la decisión final sobre la base de código
  • Por eso el poder que se obtiene en la revisión de código es temporal
• En una situación de revisión entre pares, el revisor y el autor deberían tener básicamente el mismo objetivo
  • Si hay desacuerdos serios sobre qué funcionalidad incluir, cuánto pulido hace falta antes de aprobar o cuál es un estilo de codificación aceptable, eso debe tratarse fuera del contexto de la revisión de código
• Pero en otros tipos de revisión de código no es así. Sobre todo cuando alguien de fuera del proyecto envía un parche no solicitado, la situación es muy distinta
  • Este escenario suele darse en el software libre
  • Porque cualquiera en el mundo puede modificar el código fuente y algunos intentarán reenviar sus cambios
• Pero también puede ocurrir en otras situaciones
  • Dentro de una empresa que desarrolla código propietario, los desarrolladores de un equipo pueden enviar parches al proyecto de otro equipo y esperar tener suerte
• En estas situaciones, es mucho más probable que quien recibe el parche ni siquiera quiera ese cambio, o no esté nada de acuerdo con la forma en que se hizo, y por eso no lo acepte en absoluto
  • En ese caso no se trata de abusar del poder temporal otorgado como revisor entre pares, sino de ejercer legítimamente un poder permanente como responsable del proyecto
  • Yo decido la dirección de mi proyecto de software
• Cuando el revisor de código cumple este papel de “gatekeeping”, no siempre está mal criticar un parche porque viola principios generales de diseño o requisitos ya existentes
  • Puede que simplemente no sepa cómo resolver ese problema de una manera que sí cumpla con los requisitos
  • De hecho, esa podría ser la parte difícil, y quizá la única razón por la que todavía no he hecho yo mismo ese mismo cambio
• Sin embargo, incluso en un contexto de gatekeeping, aplicar “The Guessing Game” sin explicación es una grosería
  • Yo por lo general intento explicar que el desarrollador pasó por alto la parte difícil y, si yo tampoco sé la respuesta, lo digo claramente
• Desde luego, no hay excusa para una obstrucción pasivo-agresiva como “The Death of a Thousand Round Trips”
  • Si de verdad no quieres que el parche entre al código bajo ninguna circunstancia y tienes la autoridad legítima para tomar esa decisión en tu papel de gatekeeper, puedes decirlo con palabras para que quien lo envió no siga perdiendo tiempo

Disclaimer

• Durante años he ido reuniendo notas para este artículo a partir de revisiones de código en las que participé, en ambos lados, de revisiones que observé entre otras personas y de revisiones de las que solo oí hablar en conversaciones
• Así que esto no va dirigido a ninguna persona en particular que haya revisado mi código recientemente

[intajon]

Puede que, sorprendentemente, no sea una exageración....

[magulhalmom]

Esto de verdad me pasó; casi dejo de ser desarrollador. De verdad me costó mucho recuperarme.

[bungker]

Casi me dio PTSD al leer el artículo

[kayws426]

Parece que has estado recopilando muy bien las notas para este artículo hasta ahora!!

[gongsil1212]

Solo con leerlo ya se siente como maltrato psicológico...

[wedding]

¿O sea que la clave está en la última línea? jajaja.....

[jypark]

Guau... por poco me da algo; ;

[ahwjdekf]

Este tipo de cosas, si vas a lugares donde hacen si + sm, también se ven con frecuencia en Corea. A eso comúnmente le dicen territorialismo. Hay personas malas que hacen todo tipo de cosas para proteger su puesto.

[kenuheo]

Hay muchas formas malintencionadas.

[gooksangom]

A largo plazo, quien hace ese tipo de cosas sin una razón razonable termina en uno de estos casos: 1) lo excluyen temprano de la red de contactos entre desarrolladores, o 2) aunque tenga una personalidad detestable, es tan capaz que le confían una parte importante y resulta difícil apartarlo, así que alguien asume el papel de adaptador y sirve como canal de comunicación para mantener esa conexión; por eso la situación se sostiene, pero si esa persona intermediaria desaparece por cualquier motivo, no pasa mucho tiempo antes de que lo terminen excluyendo. Por más que alguien sea increíblemente bueno, al final el dinero circula porque la gente se junta para hacer cosas, y cuando el dinero circula también circula la gente, así que quien no sabe llevarse bien con los demás suele terminar excluido y rezagado dentro de un grupo.

Normalmente, la gente que sobrevive mucho tiempo dentro de un grupo a pesar de tener una personalidad horrible suele engañarse pensando que sigue ahí porque es algo extraordinario, como un Sherlock de drama, una especie de sociópata de alto funcionamiento, pero en realidad solo pasa que a su alrededor aguantan y lo aprovechan porque todavía les resulta útil; en cuanto deja de ser útil, la relación se convierte en un "fue horrible trabajar contigo y mejor no volvamos a vernos". Incluso el Sherlock protagonizado por Cumberbatch nos parece desde afuera un sociópata atractivo, pero si no hubiera tenido a su alrededor gente que no lo abandonara y lo apreciara, simplemente no habría historia.

[dokdok]

Aunque es común que una persona con muy mal carácter que ha logrado sobrevivir mucho tiempo se engañe pensando que ha sobrevivido porque es algo grandioso, como un sociópata altamente funcional al estilo de Sherlock de algún drama, en realidad solo pasa que a su alrededor se aguantan y la aprovechan porque les resulta útil; cuando esa utilidad desaparece, la relación se convierte en un "fue desagradable coincidir contigo y no nos volvamos a ver". ==> Es una frase increíble. Tengo que recordarla.

[savvykang]

Me recuerda al acoso laboral, o más bien al pawahara.

[xguru]

Dicen que es humor, pero me dio una rabia tremenda mientras lo leía..