El objetivo principal de la revisión de código es encontrar código difícil de mantener

 (mathstodon.xyz)
5 puntos por GN⁺ 5 시간 전 | 1 comentarios | Compartir por WhatsApp
  • La revisión de código se parece más a un proceso para sacar a la luz con anticipación el código que será difícil de mantener después, que a un procedimiento para detectar bugs o garantizar la integridad
  • Si incluso al revisor le cuesta entender el código al leerlo, es muy probable que el mantenedor del futuro enfrente el mismo problema
  • Es mejor hacer las correcciones ahora, mientras el autor original todavía recuerda el contexto, y no es realista esperar encontrar bugs de forma confiable solo con inspeccionar el código
  • Más que “encuentra bugs”, para el revisor es una tarea más ejecutable “trata de entenderlo y marca las partes que no se entienden”
  • Una buena revisión no empieza intentando demostrar todo a la perfección, sino dejando notas en los puntos que no se entienden y pidiendo mejoras

Cambiar el enfoque de la revisión de código

  • El objetivo central de la revisión de código no es que el revisor encuentre bugs, ni garantizar que el código no los tenga
  • En la práctica, es poco realista esperar que se puedan detectar bugs de forma general solo con echar un vistazo al código
  • Por eso, el centro de la revisión no está tanto en “¿es código correcto?” como en “¿otra persona podrá leerlo y corregirlo después?”

El código difícil de entender es una señal de riesgo de mantenimiento

  • El revisor lee el código tratando de entender qué hace y cómo funciona
  • Las partes que no se entienden se convierten en una señal de riesgo de que un mantenedor futuro podría quedarse atorado
  • Conviene corregir ese tipo de código de inmediato, mientras el autor original aún recuerda el contexto

Tareas de revisión más accionables que buscar bugs

  • La solicitud “busca bugs en este código” es una tarea cuyo éxito es difícil de evaluar
  • Aunque se encuentren algunos bugs, puede que se hayan pasado por alto otros que siguen ocultos, así que desde la perspectiva del revisor es fácil que solo quede claro el fracaso
  • En cambio, la solicitud “trata de entender este código y, si no puedes entenderlo, señálalo” tiene criterios mucho más claros
    • No hace falta entenderlo todo a la perfección
    • Basta con registrar las partes que no se entendieron
    • Si se intenta comprender el conjunto y se dejan notas en los puntos donde uno se atora, ya se cumplió el papel de la revisión

Criterios de revisión en el trabajo real

  • El código que el revisor no puede entender puede convertirse, por sí mismo, en algo que debe corregirse
  • Los comentarios de revisión no solo sirven para reportar bugs, sino también para poner en evidencia falta de explicación, problemas de estructura y flujos difíciles de leer
  • Con este criterio, más importante que demostrar la validez del código es dejarlo en un estado que el equipo pueda leer y manejar después

Encontrar bugs es un efecto secundario

  • Esto no significa que la revisión de código no encuentre bugs en absoluto
  • Los bugs pueden descubrirse durante la revisión, pero es difícil esperar que sea un método para encontrar todos los bugs o la mayoría de ellos
  • Una condición de éxito más realista es revisar la comprensibilidad y mejorar de inmediato, junto con el autor original, las partes difíciles de mantener

Lecturas relacionadas

1 comentarios

 
GN⁺ 5 시간 전
Opiniones en Hacker News

  • La revisión de código no tiene un solo propósito. Detectar código difícil de mantener también es importante, pero no es el único propósito, y quizá ni siquiera sea el más importante.
    Funciona como una salvaguarda que dificulta fusionar código malicioso, incluso si un desarrollador o una IA se van por un camino raro, y permite que alguien que no está demasiado cerca del problema vea una mejor forma de hacerlo o problemas que se pasaron por alto.
    Alguien que conozca mejor otras partes del sistema también puede detectar problemas de interacción, hace que al menos una persona más se familiarice con ese código y se vuelve una oportunidad de aprendizaje tanto para el autor como para el revisor.
    Es especialmente importante cuando hay diferencias de experiencia: cuando mentoreo a empleados nuevos, los agrego como revisores a todos mis PR para que vean cómo trabajo, y también reviso sus PR para orientarlos. A veces yo también aprendo de ellos.
    También sirve para detectar bugs, pero no debería ser el mecanismo principal, y es especialmente importante para bugs de seguridad y rendimiento que son difíciles de atrapar con pruebas automatizadas.

    • Para sumar un argumento antiguo: la gente escribe código de otra manera cuando sabe que su código será revisado y que, a partir de ese código, se formará una impresión a largo plazo sobre la capacidad y el encaje organizacional de quien lo envía.
    • Similar al punto 3, alguien más familiarizado con ese subdominio puede detectar partes que no encajan con el código existente o que podrían causar problemas.
    • Leí “propósito único” como una invitación a entender el código y cuestionar las partes que no se entienden. Si significa que, una vez que lo entiendes, puedes señalar lo que está mal, es tonto o no es seguro, entonces me parece razonable.
      En especial con la modularización y la descomposición: una vez que entiendes un PR enorme completo, se te forma un modelo en la cabeza y empiezas a ver si será mantenible, si algún día será una pesadilla total o si quedará en algún punto intermedio.

  • Creo que quizá la parte más importante de la revisión de código es la transferencia de conocimiento.
    En nuestro equipo pequeño, salvo que sea algo urgente, todo el equipo marca aprobación en el PR antes de hacer merge, y gracias a eso todos los integrantes tienen una idea general del estado actual de la base de código. Así evitamos sorpresas como “desapareció todo un sistema del que yo dependía”, que viví antes en empresas más aisladas en silos.
    También se vuelve un espacio para hacer preguntas sobre cómo funcionan las cosas y aumentar la comprensión. En un equipo que funciona bien, todos los desarrolladores deberían entender hasta cierto punto el sistema completo, incluidas las partes que no tocan directamente.
    Otra función importante es la verificación del conocimiento organizacional. Hace poco cambié un poco una tabla, y un colega me avisó que eso rompería un microservicio que yo no había considerado y que escribía en esa tabla. Yo no sabía ni que ese microservicio existía ni que accedía a esa tabla, pero gracias a esa revisión pudimos evitar un problema mayor y una situación de limpieza de datos.

    • Que “todo un equipo pequeño apruebe un PR antes del merge” suena bien para una base de código pequeña que se mueve lento, pero si se impone en un equipo grande o en una base de código que avanza rápido, es fácil que se convierta en un ritual formal de mirar el código por encima y apretar el botón de aprobar.
      Al final todos están ocupados con su propio trabajo y no quieren ser la persona que bloquea un PR importante, así que simplemente aprueban, mientras que en realidad nadie está revisando el código.
    • Me da curiosidad saber de qué tamaño es el equipo. Probablemente un enfoque así no escale más allá de cinco ingenieros.
      Para problemas como “desapareció todo un sistema del que yo dependía”, creo que son muy importantes las pruebas automatizadas que puedan detectarlo aunque la persona que depende de ese sistema no esté presente.
      También apoyo firmemente la propiedad compartida de todo. Es natural que la gente termine siendo dueña en la práctica de ciertas partes de la base de código, sobre todo de componentes que creó, pero eso lleva a silos y a un bajo factor bus. No debería formarse una estructura en la que una persona sea dueña de un sistema y ese sistema dependa de otro componente propiedad de una sola persona.
    • Me pregunto qué habría impedido que ese cambio que rompía cosas llegara a producción si tu colega no hubiera estado en la revisión. Si la revisión de código es importante, ¿dónde quedan las pruebas?
    • A veces creo un PR y etiqueto a otros desarrolladores aunque sea código que voy a fusionar de inmediato de todos modos. El objetivo es dar una ruta fácil para ver qué se fusionó y por qué, para que la gente no se pierda lo que hay dentro de la base de código.
    • Para cambios o tareas de limpieza que no son triviales, siempre viene bien un segundo par de ojos. Pero el enfoque de “todos leen todo” no escala con N grande.
      Cuando hay demasiado para leer, nadie puede seguir el ritmo; por eso se delega, se documenta y se hacen sesiones de panorama general.

  • Siempre he pensado que la mejor forma de ver la revisión de código es como la puerta por la que el código que pertenecía al autor pasa a ser propiedad del equipo o del proyecto.
    El código que reviso no es tu código, sino código que pronto será nuestro. Naturalmente, la mantenibilidad es un factor importante dentro de eso.

    • Qué entorno tan envidiable y lujoso.
      Nuestro equipo empezó a usar IA, así que yo cambié a un enfoque simple. No dejo comentarios; solo tomo una decisión binaria de aprobación: “¿esto es una locura o puede pasar?”.
      Estoy cuidando mi tiempo y mi salud mental.

  • Hacer esto solo vuelve más perezosos tanto al revisor como al autor.
    El propósito de una revisión de código es multifacético. Incluye si es difícil de mantener, si puede tener bugs, si se puede hacer más simple y limpio, si se ajusta al estilo de código del proyecto, si ayuda a que otros entiendan el código, si sirve para incorporar a miembros junior del equipo, y si hace un sanity check de las decisiones de diseño.
    Este tipo de textos ligeros por lo general se parecen más a una forma de justificarse para revisores de código perezosos.

    • Hay una checklist aparte de cosas que mirar en una revisión.
      Hay que ver si funcionalmente logra el objetivo según la descripción del issue o del PR, si hay código innecesario como salidas de debug restantes o claves de API privadas, y si existen defectos obvios como fugas de memoria, casos borde no manejados, fallas de seguridad o llamadas a APIs obsoletas.
      También hay que mirar si puede hacerse más entendible, si conviene agregar o quitar abstracciones, si los nombres de variables y métodos son mejores, y si sería mejor usar más o menos estilo funcional.
      También debe verificarse si es consistente con la base de código o la guía de estilo, si hay mejoras de rendimiento obvias como usar un hash set en vez de una lista o aplicar evaluación diferida, y si las pruebas son suficientes.
      Tampoco estoy completamente de acuerdo con la afirmación de que, si el código no se puede entender, no debería aprobarse. Algún código simplemente es realmente difícil de entender, y el objetivo es que sea funcionalmente correcto y tan fácil de entender como sea posible.
    • La industria se ha esforzado mucho por pasar de “culpar al autor” a “culpar al proceso y al equipo”.
      Pero este texto se parece más a bait, algo como decir: “la gente cree que la cena consiste en comer comida, pero en realidad no se trata de comer, sino de conectar con familiares y amigos”. Es un tipo específico de lógica reduccionista floja que funciona bien en HN.
    • Ahora que, por la IA, la velocidad de escribir y desplegar código aumentó, el énfasis debería trasladarse a la revisión. Hay que comprobar si el código realmente se ejecuta correctamente, si todas nuestras suposiciones son ciertas y si no hay efectos secundarios no intencionales.
      Sentí que la revisión y el debugging consumen mucho más tiempo que escribir o producir código, y simplemente “rezar para que funcione” nunca termina bien.

  • Decir que “por lo general no se pueden encontrar bugs mirando el código” no es correcto en absoluto. Se pueden encontrar bastantes en cada nivel de abstracción, y a eso se le llama code smell.
    Descriptores de archivo que no se cierran, corrutinas sin await, enormes bloques try/catch que devuelven algún valor sin registrar el error, casteos de tipo incorrectos, todo eso entra ahí.
    Como principio general, no hay que ver el type checker, el compilador y el runtime como simples etapas que hay que pasar. Hay que trabajar con esas etapas, reconocer que son herramientas valiosas y no trabajar en contra de ellas.

    • No entiendo de qué habla. He encontrado bugs solo con revisión de código sin ejecutarlo, y también otras personas han hecho eso con mi código; además, lo he visto en revisiones de otras personas.
      Se podría definir “por lo general” de alguna manera para que técnicamente sea verdad, pero entonces pierde casi todo significado.

  • Si se van a hacer afirmaciones amplias sobre la revisión de código, es importante definir de qué tipo de revisión de código se está hablando.
    Hoy las revisiones asincrónicas de PR al estilo GitHub y los comentarios inline son el estándar, pero las revisiones no se limitan a eso. Antes también había procesos de revisión presencial más parecidos a la defensa de una tesis o a una presentación en una conferencia.
    La literatura que muestra que la revisión de código es una práctica de calidad útil —de hecho, una de las pocas prácticas de calidad útiles— proviene en general de procesos de revisión mucho más estructurados que los actuales.
    Personalmente, las revisiones de PR estilo GitHub anteriores a los LLM me parecían más bien algo para sentir que el proceso estaba bien o para marcar una casilla de gobernanza, y en la era de los LLM creo que su relación costo-beneficio empeorará mucho y terminarán desapareciendo.

    • Las revisiones sincrónicas todavía son posibles. Uno de mis primeros managers me enseñó que, si una revisión de código “estándar” iba y venía más de una vez, casi siempre era mejor reunirse en persona —o por Zoom si al menos una persona estaba remota—, resolverlo allí y luego dejar el acuerdo en un comentario.
      Usando una analogía técnica forzada: la comunicación asincrónica por texto tiene más pérdida que el habla en cuanto a la información que puede codificar con éxito, y también tiene menor throughput. Por eso, cuando hay que intercambiar mucha información, vale la pena pagar el costo de sincronización.
    • En uno de mis primeros trabajos, había que imprimir el paquete de cambios, revisarlo y firmarlo. Incluso había una persona encargada de guardar la versión final en un archivero.
      Era más cercano a la ingeniería tradicional, y todos tenían que pensar en el software como algo más permanente.

  • Es cierto que garantizar la mantenibilidad es uno de los beneficios de la revisión de código, pero decir que ese es su único propósito es una afirmación bastante audaz.
    La revisión de código también es una herramienta que permite al equipo entender los cambios de código y compartir la responsabilidad colectiva sobre toda la base de código.

  • La revisión de código no es una sola cosa. Hay muchas razones: compartir conocimiento, lavar responsabilidades, calidad del código, cumplimiento regulatorio, etc.; y, como siempre, el objetivo depende del contexto de uso.

    • Decir que la mayoría de la gente no entiende la razón de las revisiones entre pares parece bastante ignorante. Creer que solo hay un único propósito parece señal de falta de experiencia trabajando con otros equipos o personas.

  • Si el autor es matemático, la frase “por lo general no se pueden encontrar bugs mirando el código” probablemente no significa que encontrar bugs sea completamente imposible.
    Más bien significa que no es posible encontrar todos los bugs, o encontrar un bug específico.

    • Por mi experiencia en clases universitarias de matemática, muchas veces los matemáticos son realmente malos comunicándose con otros seres humanos. Eso explica por qué podrían pensar que lo que dijeron y la forma en que casi todos lo leen son cosas distintas.
    • Si ese es el significado, entonces está bien.
      Conectándolo con el punto de la mantenibilidad, otro objetivo de la revisión es hacer el código lo más simple posible para aumentar la probabilidad de que sea depurable mediante revisión. No evita bugs en sentido absoluto, pero sí aumenta la probabilidad.
    • Parece que el autor, siendo matemático, no entendió el significado de sus propios cuantificadores en lenguaje natural. “Por lo general no se pueden encontrar bugs mirando el código” significa “por lo general no se puede encontrar ningún bug mirando el código”, no “no se pueden encontrar todos los bugs”.
      La primera interpretación es relevante pero falsa; la segunda es verdadera pero irrelevante.
      Probablemente el autor quiso decir “un bug dado no se puede encontrar por lo general mirando el código”, es decir, “no para todo bug B es posible encontrar B”, pero eso también es verdad y está lejos del punto central.

  • He trabajado tanto con personas que rechazan con frecuencia las sugerencias en los PR como con personas que las aceptan.
    Quien acepta sugerencias me hace pensar que, en cierta medida, está dispuesto a compartir conmigo la propiedad. Se siente como que ambos mantenemos y entendemos el código, y miramos en la misma dirección.
    En cambio, con los PR de alguien que rechaza las sugerencias, disminuyen las ganas de participar. Si de todos modos lo van a rechazar, ¿para qué dedicar tiempo a revisarlo con cuidado?

    • En nuestro equipo, por lo general anteponemos a los comentarios prefijos como thought, change, nit, fix, chat.
      Por ejemplo, thought sería algo como “quizá más adelante foo se vuelva más común, así que refactoricemos cuando llegue ese momento”; change, “esta es una abstracción con fugas, así que me gustaría que se modelara como bar”; nit, “el nombre no es del todo intuitivo, así que consideremos Baz o Boo”; fix, “esta prueba unitaria valida el campo incorrecto”; y chat, “esta es una decisión importante que definirá la forma de las soluciones de esta categoría en adelante, así que primero hablémoslo con el equipo”.
      Algunos prefijos significan que el PR queda detenido hasta que se haga el cambio, y otros que el comentario se puede aceptar o no. Para quien lo escribió, queda claro qué es algo en lo que hay que “llegar a un entendimiento compartido” y qué es una “preferencia de expresión” u “observación”.
      Eso sí: si dejaste un nit y la otra persona no está de acuerdo y lo ignora, no deberías molestarte. Si lo sentías con tanta fuerza, no debió haber sido un nit.
    • Si crees que algo es importante, debes dejar una sugerencia bloqueante y forzar la conversación.