¿Es una buena idea una revisión de código que usa solo comentarios de código?

16

Condiciones previas

  • El equipo usa DVCS
  • IDE admite el análisis de comentarios (como TODO y etc.)
  • Herramientas como CodeCollaborator son caras para el presupuesto
  • Las herramientas como gerrit son demasiado complejas para instalar o no se pueden usar

Flujo de trabajo

  • El autor publica en algún lugar de la rama de funciones de repositorio central
  • El revisor lo busca y comienza a revisar
  • En caso de que algún revisor de preguntas / problemas cree un comentario con una etiqueta especial, como "REV". Dicha etiqueta NO DEBE estar en el código de producción, solo en la etapa de revisión:

    $somevar = 123;
    // REV Why do echo this here?
    echo $somevar;
    
  • Cuando el revisor termina de publicar comentarios, solo se compromete con mensajes estúpidos "comentarios" y retrocede

  • El autor retira la rama de la función y responde comentarios de manera similar o mejora el código y lo retrocede
  • Cuando los comentarios "REV" han desaparecido, podemos pensar que esa revisión ha finalizado con éxito.
  • El autor reajusta interactivamente la rama de la característica, la aplasta para eliminar esas confirmaciones de "comentario" y ahora está listo para fusionar la característica para desarrollar o realizar cualquier acción que normalmente podría ocurrir después de una revisión interna exitosa

Soporte IDE

Sé que las etiquetas de comentarios personalizados son posibles en eclipse y netbeans. Claro que también debería estar en la familia blablaStorm.

Ejemplo de tarea personalizada de comentarios filtrados en eclipse

Preguntas

  1. ¿Crees que esta metodología es viable?
  2. ¿Sabes algo similar?
  3. ¿Qué se puede mejorar en él?
gaRex
fuente
Buena pregunta, pero no tiene nada que ver con las servilletas, no es un gran título.
MarkJ
@ MarkJ ¿cómo nombrarlo entonces? Me refiero a algo de artesanía, cabaña, hecho en casa. En ruso tenemos la frase "на коленке". Algo no estable, no ideal, no sólido, pero funciona.
gaRex
2
@ MarkJ, gaRex: ¿qué pasa con este nuevo título? Siéntase libre de revertir si considera que no es apropiado para esta pregunta.
Arseni Mourzenko
@MainMa, está bien
gaRex
1
Atlassian Crucible es esencialmente gratuito para hasta 10 desarrolladores. También es la mejor herramienta de revisión de código que he usado. El enfoque de comentarios es viable pero puede dificultar el seguimiento del estado.
Andrew T Finnell

Respuestas:

14

La idea es realmente muy buena. Al contrario de los flujos de trabajo comunes, mantiene la revisión directamente en el código, por lo que técnicamente, no necesita nada más que editor de texto para usar este flujo de trabajo. El soporte en el IDE también es bueno, especialmente la capacidad de mostrar la lista de revisiones en la parte inferior.

Todavía hay algunos inconvenientes:

  • Funciona bien para equipos muy pequeños, pero los equipos más grandes requerirán un seguimiento de lo que se revisó, cuándo, por quién y con qué resultado. Si bien realmente tiene este tipo de seguimiento (el control de versiones permite saber todo eso), es extremadamente difícil de usar y buscar, y a menudo requerirá una búsqueda manual o semi-manual a través de las revisiones.

  • No creo que el revisor tenga suficientes comentarios del revisor para saber cómo se aplicaron realmente los puntos revisados .

    Imagina la siguiente situación. Alice está revisando por primera vez el código de Eric. Se da cuenta de que Eric, un desarrollador joven, usó la sintaxis que no es la más descriptiva en el lenguaje de programación realmente utilizado.

    Alice sugiere una sintaxis alternativa y envía el código a Eric. Reescribe el código utilizando esta sintaxis alternativa que cree que comprende correctamente, y elimina el // BLAcomentario correspondiente .

    La próxima semana, ella recibe el código para la segunda revisión. ¿Sería capaz de recordar realmente que hizo este comentario durante su primera revisión, para ver cómo Eric lo resolvió?

    En un proceso de revisión más formal, Alice pudo ver de inmediato que hizo un comentario y ver la diferencia del código relevante, para notar que Eric no entendió la sintaxis que le contó.

  • La gente sigue siendo gente. Estoy bastante seguro de que algunos de esos comentarios terminarán en el código de producción, y algunos permanecerán como basura mientras estén completamente desactualizados .

    Por supuesto, el mismo problema existe con cualquier otro comentario; por ejemplo, hay muchos comentarios TODO en producción (incluido el más útil: "TODO: Comente el código a continuación"), y muchos comentarios que no se actualizaron cuando se realizó el código correspondiente.

    Por ejemplo, el autor original del código o el revisor pueden irse, y el nuevo desarrollador no entendería lo que dice la revisión, por lo que el comentario permanecerá para siempre, esperando que alguien sea demasiado valiente para borrarlo o para entender realmente qué dice.

  • Esto no reemplaza una revisión cara a cara (pero el mismo problema se aplica también a cualquier otra revisión más formal que no se haga cara a cara).

    Especialmente, si la revisión original requiere una explicación, el revisor y el revisor comenzarán una especie de discusión . No solo se encontrará con grandes comentarios de BLA, sino que esas discusiones también contaminarán el registro del control de versiones .

  • También puede encontrar problemas menores con la sintaxis (que también existe para los comentarios TODO). Por ejemplo, ¿qué pasa si un comentario largo "// BLA" se genera en varias líneas y alguien decide escribirlo de esta manera:

    // BLA This is a very long comment which is way beyond 80 characters, so it actually
    // fills more then one line. Since the next lines start with slashes, but not "BLA"
    // keyword, the IDE may not be able to show those lines, and display only the first one.
    
  • Y, por último, como una nota menor y muy personal: no elija "BLA" como palabra clave. Suena feo ;)

Arseni Mourzenko
fuente
"para saber cómo se aplicaron realmente los puntos revisados" Sí :) Solo la honestidad del revisor. Aquí tenemos más políticas que herramientas.
gaRex
1
"Las personas son personas" Sí :( Ya tenemos millones de estos TODOs puede ser justo negar a tener tal característica en entornos de desarrollo.?
GAREX
"contaminar el registro del control de versiones" Para esto, todo el trabajo se realiza en una rama de características independiente, que luego se aplasta y fusiona en el desarrollo. Puede ser que esto se pueda hacer con un simple gancho en DVCS. Pero como veo, toda la complejidad pasa de las herramientas de revisión de código a IDEs y DVCS.
gaRex
"problemas menores con la sintaxis" ¿Puede ser que no sea un problema? Esos REV son solo como algunos marcadores para ir rápidamente desde el panel.
gaRex
1
@gaRex: entonces los miembros de su equipo deben usar el correo electrónico para acordar una fecha de presentación cara a cara ;-)
Doc Brown
4

Complementaría los comentarios en el código con un documento complementario. Esto resumiría los hallazgos y seguiría vivo después de que se eliminaran los comentarios. Las ventajas de esto serían:

  • compacidad Si la persona rutinariamente falla en verificar que un puntero que se pasa no sea nulo (o algún otro error de principiante común en el idioma que está usando), el revisor puede dejar docenas de comentarios REV en ese sentido, y en el documento puede decir " Encontré 37 lugares donde los punteros no se verificaron primero "sin enumerarlos todos
  • Lugar de alabanza. Un comentario comoREV this is a nice design simplemente parece un poco extraño, pero mis revisiones de código a menudo incluyen aprobación y correcciones
  • interactividad Su comentario de muestra es "¿por qué hiciste esto?" y es muy típico Un enfoque de solo comentarios no admite una conversación. La persona puede cambiar su código y eliminar el comentario, o eliminar el comentario. Pero "¿por qué hiciste esto?" y "por favor cambia esto, está mal" no son lo mismo.
  • llevar un registro Un revisor posterior puede verificar si el código se cambió realmente o si se eliminaron los comentarios. También pueden verificar que los comentarios se hayan "tomado en cuenta" y que el desarrollador ya no cometa los mismos errores en una revisión posterior.

También usaría un elemento de trabajo para hacer la revisión y responder a la revisión, y asociar los registros con ella. Eso facilita encontrar los comentarios en un conjunto de cambios antiguo y ver cómo se manejó cada uno en otro conjunto de cambios.

Kate Gregory
fuente
entonces necesitamos alguna buena herramienta de revisión de código. Nuestro enfoque actual descrito es principalmente político, ya que las personas deberían inventar un etiquet y seguirlo.
gaRex
"compacidad": creo que podría hacerse con un comentario como "// REV Dude, tenemos 37 lugares donde los punteros no se verificaron primero, incluido este. Por favor RTFM"
gaRex
"lugar de alabanza": también es posible, pero después de que se pierda el aplastamiento :( Ya veo un problema: perdimos el historial de revisiones al
cometer aplausos
"interactividad": el autor puede responder en otros comentarios, debajo de los iniciales. Al igual que en el estilo wikipedia.
gaRex
"la persona puede ... eliminar el comentario": es un problema. +1
gaRex
2

Otros han hablado de las limitaciones de este enfoque. Usted mencionó que las herramientas como Gerrit eran difíciles de instalar. Le sugiero que eche un vistazo a phabricator (http://www.phabricator.com). Este es el sistema de revisión de código que Facebook ha usado durante años, y recientemente fue de código abierto. No es difícil de instalar, tiene excelentes flujos de trabajo y resuelve todos los problemas mencionados en los otros comentarios.

Adam Hupp
fuente
¡Guauu! Necesitamos probarlo en lugar de nuestra encantadora mina roja.
gaRex
"gerrit" lo instalé, no fue tan difícil. ¡Simplemente no lo uso! Y también tiene una interfaz de usuario y un flujo de trabajo feos.
gaRex
@gaRex Utilizamos Reviewboard ( reviewboard.org ) en paralelo con Redmine. Sirven para diferentes propósitos, así que está bien. Y puede configurar RB para vincular a los problemas de Redmine ...
Johannes S.
@JohannesS. ¿Qué vcs estás usando? ¿También tienes algún tutorial o wiki sobre su integración? Es bueno tener tal.
gaRex
@gaRex Lo siento por la respuesta tardía. Usamos SVN, pero RB también admite git (en realidad, las personas RB usan git ellos mismos). Sugiero echar un vistazo al sitio web de RB. Hay una demostración en línea disponible (por ejemplo, visite demo.reviewboard.org/r/101 )
Johannes S.