martes, febrero 12, 2008

Code reviews, ¿buena o mala idea?

martes, febrero 12, 2008 por Martín


Hace poco que en mi empresa un compañero se encargó de preparar un sistema de code reviews. Como era de esperar, ha sido algo bastante polémico dentro del seno de los desarrolladores. Esto me ha hecho pensar bastante sobre el tema.

Uno de los problemas más importantes de las empresas es controlar la calidad del código que se genera para satisfacer las necesidades funcionales. Esa es una de las razones por las que yo mismo recomendé el realizar revisiones de código. Innegablemente, las revisiones de código son necesarias. Uno puede argumentar que con realizar tests funcionales sería suficiente, pero lamentablemente que un código cumpla su cometido funcional no garantiza que cumpla su cometido no-funcional, es decir, no garantiza que sea escalable, mantenible, extensible, o incluso... legible.

Sin embargo, ¿hasta dónde se debe llegar con con las revisiones de código? Mi experiencia es que las revisiones de código demasiado estrictas pueden ser muy contraproducentes según como sea la estructura de desarrollo de una empresa. Es muy fácil que se vean como un "aquí vienen los arquitectos a hacer de policías y hacernos la vida mucho más difícil". Con las revisiones de código es muy, pero que muy fácil caer en el autoritarismo y afectar en la moral de los desarrolladores que verán limitada su capacidad de creación.

Algunos argumentarán que cortar la capacidad de creación de los programadores es algo bueno. Es la típica aproximación de factoría de software. Tener personas robotizadas programadas para hacer lo que se le ha dicho exactamente del modo que se le ha dicho. Bajo esta suposición, el establecer revisiones de código muy estrictas sería el mejor modo de garantizar la calidad. Seguramente lo sería, pero lamentablemente sería el peor modo de garantizar la continuidad del personal. Al cortar la creatividad, no sólo se le esta prohibiendo a los desarrolladores hacer lo que más les gusta, si no que además éstos perderán toda "enlace sentimental" con el código que han creado. No esperes que estos desarrolladores vuelvan atrás, o se queden un par de horas, o piensen en casa como mejorar un código que les han obligado a escribir.

El mejor sistema de code reviews será aquel que permita un equilibrio entre los deseos del arquitecto y las ambiciones del desarrollador. Hay una serie de puntos que pueden hacer llegar a este equilibrio:

  • Utilizar herramientas de análisis estático de código: Herramientas como PMD o Findbugs ayudarán a encontrar errores de código básicos, sin tener que advertir a los programadores que han cometido errores. El estar continuamente advirtiendo peresonalmente a los desarrolladores de errores que han cometido es algo que puede ser incómodo, y en algunos casos causar problemas sociales ("ahí viene el listillo a decirme lo que ya sabía"). Para ello, nada mejor que introducir estas herramientas en la build y despreocuparse de los fallos triviales.
  • Centrarse en los aspectos no funcionales: Las revisiones de código no deberían centrarse en si el código funciona, o no, ya que eso es algo que desvelarán los tests de integración.
  • No olvidar el componente social de las revisiones: Este es un aspecto muy importante. Las revisiones no son un sistema para mostrar que el revisor sabe más, o tiene más autoridad que el revisado. Tampoco son un sistema para asegurarnos de que las cosas se han hecho como hemos ordenado. Es muy importante que las revisiones sean un mecanismo de aprendizaje mutuo. Si las cosas no se han hecho como se recomendaban, seguramente haya razones para ello, que en su caso pueden ser de peso; en caso contrario el revisor debe actuar como mentor y aconsejar, o incluso ayudar realizando pair programming si es necesario.
  • Olvidar los aspectos superfluos: Muchas veces, las revisiones tienden a derivar hacia temas bastante superfluos como el "porque esta variable es protected en vez de privada", o "por qué este valor es estático", o "por qué utilizas un espaciado de 6 si nuestro estándar dice que hay que utilizar 4". La mayor parte de estas cosas son realmente superfluas. Algunas se detectan con análisis estático, otras se solucionan con un formateador de código, etc. El valor de las revisiones no reside en estas minucias, sino en detectar el impacto de los nuevos componentes en la arquitectura del sistema. ¿Es escalable? ¿Es fácilmente mantenible? ¿Lo podremos monitorizar? ¿Está haciendo auditoría? ¿Se ajusta a nuestro modelo de componentes? ¿Define interfaces como el resto de nuestros componentes? ¿Está haciendo buen uso de los frameworks? ¿Hay problemas de rendimiento, memory leaks, o faltas graves claramente visibles?
  • Dar libertad al desarrollador: Al hilo del punto anterior, una vez revisados los aspectos no funcionales y el impacto en la arquitectura, sinceramente, queda poco por revisar. Darle libertad al desarrollador para que cree sus propios componentes es importante, incluso darle la libertad para mejorar sus creaciones (dentro de un órden) es muy valioso ya que crea vínculos morales entre el desarrollador y sus "criaturas". De hecho, es importante sentarse e interesarse por el cómo ha sido diseñado el componente, y realizar sugerencias en caso de que sea necesario. A fin de cuentas, ¡a los programadores nos encanta hablar de lo que programamos!


Resumiendo, code reviews si, pero con paciencia, calma, y desde un punto de vista didáctico. Todo el mundo puede aprender de las reviews, tanto revisores como revisados.

comments

16 Respuestas a "Code reviews, ¿buena o mala idea?"
Diego Parrilla dijo...
9:23

Las revisiones de código son buena idea. Lo que ocurre es que se deben centrar sobre aspectos no triviales. Los aspectos triviales deben automatizarse con herramientas (jalopy, checkstyle, pmd, findbugs...) y deben cumplirse por los desarrolladores ANTES de subir su código. Obviamente hay que configurar estas herramientas porque si no nos podemos volver locos, pero hay que usarlas.
Y en las revisiones de código discutir cuestiones más centradas en las buenas prácticas de diseño y arquitectura.
Lo que no veo es cómo descubrir en una revisión de código que algo es correcto desde el punto de vista funcional o no. Deben ser cosas muy obvias, no?
Y respecto a la libertad de los desarrolladores... bueno, todos tenemos que cumplir directrices y reglas... qué remedio!


chuidiang dijo...
9:30

Hola:

Estoy de acuerdo en que las revisiones hay que hacerlas con mucho cuidado para no crear "mal rollo". De todas formas, estoy de acuerdo con lo que comenta Joel on software en este artículo http://spanish.joelonsoftware.com/Articles/BigMacsvs.TheNakedChef.html

Hay programadores muy buenos y que destacan del resto. Posiblemente estos necesiten menos revisiones de su código, ya que generalmente serán pérdidas de tiempo.

Sin embargo, hay otros programadores que sí necesitan que se les revise el código. Gente que está programando porque en algo hay que meterse o gente nueva que todavía no tiene la suficiente experiencia.

Supongo que lo ideal es no adoptar una medida estándar y medir a todos por el mismo patrón, sino hacerlo en función del programador en concreto.

Se bueno.


Diego Parrilla dijo...
9:39

Yo creo que las revisiones de código no son para que los que no saben aprendan: es para que el código sea coherente, haya un único estilo que ayude a la mentalidad de equipo (filosofía ágil) y se pueda mejorar el código que ya es bueno.
Si hay programadores con poca experiencia, creo que lo mejor es asignarle un programador senior como tutor o mentor, que haga un rato de programación por parejas todos los días con el novato y le dirija un poco.
Las revisiones de código deben ser lo más cortas posibles: no vale abrir el código y mirar. Quien dirija la reunión debe llevar la lista de cuestiones a tratar y ser estricto en el tiempo. Y si hay algún desarrollador 'que no se entera', se le deja como tarea al senior que le explique aparte las cosas.
Si, las revisiones de código son coñazo. Se de compañeros que llevan látigos, espadas de plástico (para cortar las manos de algún programador...), relojes de arena (para los pesados: cuando se enrolla alguno se pone el reloj en marcha y abrevia) para hacer de las reuniones algo divertido, porque es fácil sentirse atacado.


Acido 69 dijo...
10:00

Donde trabajáis?
Yo he aprendido mucho de mi Yoda (el senior que me da collejas de vez en cuando) y la verdad que he aprendido mucho de él; pero nunca utilizamos este sistema.
suena muy interesante, investigaré sobre el tema.


Martín dijo...
10:05

Ahí le has dado en el clavo. Es fácil sentirse atacado. En nuestra empresa, hay gente muy senior que no le gusta que le revisen el código. Las revisiones adquieren un cariz un poco tenso del estilo "tú no quieres que te revisen, yo tengo que revisarte, así que vamos a hacer esto lo menos doloroso posible".

Respecto al punto de vista funcional. En nuestro caso nosotros traducimos las especificaciones funcionales en especificaciones técnicas. En nuestro mundillo es sencillo detectar que algo no va a cumplir determinado aspecto funcional (pues por ejemplo porque no se esté parseando cierto campo del XML que hemos recibido por lo que será imposible hacer un cálculo necesario, o porque no se haya hecho una conversión de divisas necesria, o...). Estas cosas, si las ves, bueno, mi opinión es advertirlas pero tampoco es que nos tengamos que centrar en eso ya que para eso tenemos un equipo de QA por detrás.


Pentacour dijo...
11:34

Hola! Donde trabajaba antes se implantaron las revisiones de código. Creo que un aspecto importante es que quién revise el código tenga la suficiente autoridad moral (que se vea como un Yoda como comentan más arriba) para evitar que el programador pueda decir "pues hazlo tú". Y como dices, buscar un buen equilibrio, ya que si no, es muy contraproducente. Allí se tuvieron que abandonar porque retrasaban notablemente los proyectos. Quizás una opción sería ir revisando selectivamente los códigos, sobre todo de los que comienzan en la empresa (para evitar "malas costumbres de programación") y dar un voto de confianza a los seniors.


Diego Parrilla dijo...
11:58

A nosotros nos pasó un poco lo mismo: la resistencia a la revisión de código es feroz. La verdad es que nunca lo he entendido, pero es así. Mi propuesta es que se implementen todas las herramientas posibles de control automático para que el desarrollador se haga responsable de su trabajo.
Si no sabe hacerse responsable, que aprenda. Si no quiere hacerse responsable o bien no aprende, creo que es mejor buscar a otro desarrollador que sí entienda que la revisión de código es importante para sus compañeros, y 'sugerirle' que se busque otra empresa en la que su filosofía de vida encaje mejor.
Suena duro, pero a todos nos revisan el trabajo de alguna manera, y debemos adaptarnos a ello. Los programadores no somos una excepción.
Eso si, no hay que pedir imposibles y hay que ser razonable en las revisiones de código.


Pentacour dijo...
12:26

Sí Diego, pero creo que si alguien ha llegado a programador senior en la empresa es porque gusta como programa. Creo que hay que hacer más incapié en los que empiezan. Y los informáticos no somos una excepción a la revisión de nuestro trabajo, pero también veo que quizás nuestro campo no es como otros en los que los controles de calidad son mucho más claros ("esto debe aguantar tantos kilos y si no está mal"), y por ello, creo que estamos de acuerdo, que antes de implantar un sistema de revisión de códigos se debe disponer de las personas o equipo adecuados y que hayan unos standards de programación lo más claros posibles en la empresa. Es decir, lo bueno es que el programador en una revisión vea claramente que se ha equivocado y que no hay lugar a dudas.


Diego Parrilla dijo...
12:58

@Pentacour, si alguien llega a programador senior es porque tiene una experiencia, ha demostrado algo. Si nadie le ha revisado el código antes no sabemos si programa bien o mal. Sufrí el caso de un programador con 20 años de experiencia cuyo código no superaba los más mínimos estándares de calidad -pero funcionaba, ojo- y era absolutamente incomprensible. Pese a que intentamos que fuese por el buen camino, resultó imposible y al final se le dejó que trabajase solo y en proyectos de bajo riesgo. Su código era inmantenible y se negaba a aceptar que lo que él hacía no estaba bien.
La situación era complicada: por un lado, no puedes echar a alguien que es productivo, pero tampoco puedes ponerle a trabajar con otros desarrolladores y a seguir un control de calidad estricto, porque no encaja.
El tema de las revisiones de código es difícil de implementar porque debes cambiar la manera de pensar de la gente, no porque sea algo difícil de hacer. Sólo una persona con un gran liderazgo y un equipo receptivo pueden conseguirlo.
Por cierto, los comentarios de estea entrada son para enmarcarlos, MArtín.


Pentacour dijo...
14:16

@Diego, lo que comentas es un caso delicado en el que veo lógica la decisión que tomasteis. Otro caso es el de una empresa joven en la que han habido programadores trabajando en equipo, sacándola adelante y con revisiones de código informales, y que cuando se han querido formalizar esas revisiones, nadie las pasaba a la primera, con los consiguientes retrasos en los plazos de entrega, incumplimiento de objetivos, etc. Lo que quiero transmitir es que veo bien las revisiones, pero bien pensadas y llevadas a cabo por personal adecuado. Que no se implanten a la ligera porque quede bien tener un jefe de control de calidad.


Diego Parrilla dijo...
14:44

@Pentacour, el caso que tratas es precisamente como no se deben abordar las revisiones de código. En el tema de formalizar procesos de revisión de código y control de calidad no puedes ir de 0 a 100 sobre un proyecto en marcha. Debe ser algo gradual.
Igual he dado el mensaje de que es algo punitivo, pero todo lo contrario, es algo constructivo.


Martín dijo...
10:01

Totalmente de acuerdo. En nuestro caso las revisiones de código empiezan utilizando herramientas como PMD integrándolas con el IDE de modo que garantizas que el código tenga una mínima calidad antes de subirse al sistema de control de versiones.

Después, no revisamos código antiguo, sino únicamente el código que ha sido creado después de que hemos comenzado el sistema de control de versiones.

Y finalmente, la revisión sólo se hace de temas realmente importantes. Por ejemplo, puedes sugerir que se haga log de esto y esto otro, puedes sugerir "para la próxima" se cambien modificadores de variables, pero realmente no son aspectos que nos importen demasiado. La idea es que las cosas más insignificantes no paren demasiado el desarrollo.

Gracias por vuestos comentarios, son realmente interesantes.


Ferdy dijo...
1:12

Estoy bastante de acuerdo con vuestros comentarios. Las revisiones de código son buenas, siempre y cuando, se apliquen criterios razonables.

Mi experiencia es que los buenos programadores, generalmente, no se sienten especialmente atacados cuando les informas de que cierto código no es la mejor solución. La clave esta en saber explicarles, de forma didáctica, el porqué, los posibles problemas y cual seria la mejor solución. Como ya habéis dicho anteriormente, huir del autoritarismos (harto difícil, por cierto). Muchas veces no es necesario darle el nuevo código, sino simplemente decirle esquemáticamente cual seria la mejor solución y que lo codifique el a su gusto. Aun así, como en todas las profesiones, hay gente que tiene el ego excesivamente alto y no acepta ningún tipo de recomendación.

Nosotros comenzamos a realizar revisiones de código hace dos años, y tengo que decir que obtuvimos resultados desiguales. El método de implantación fue mediante un equipo de revisores especializado, y es que, para comenzar, no queríamos que los desarrolladores se revisasen código entre ellos, nos daba mucho miedo ;-) . Antes de comenzar a aplicar las revisiones, publicamos y divulgamos la normativa de desarrollo de aplicaciones. Para los aspectos más triviales, utilizamos checkstyle de manera automática en nuestra herramienta de SCM. Si hay alguna regla que no se cumple, ya no se deja subir el componente. Luego, aleatoriamente, realizamos revisiones en más profundidad, en temas que las herramientas de análisis de código no saben detectar. Y es increíble lo que llegas a descubrir.

Como anécdota os diré que detectamos un caso en que el desarrollador había implantado un sistema de gestión de ficheros propio, para realizar uploads, borrar o modificar ficheros en producción sin pasar por la herramienta de SCM. Para nosotros (lease entidad financiera) este tema es muy importante, ya que te pueden meter un código malicioso sin darte cuenta y sin que quede rastro en ningún sitio. Una vez descubierto este sistema, hablamos con la empresa subcontratada, y despidieron al empleado (que conste que nosotros no forzamos el despido). Al cabo de unos meses, y por casualidad, volvimos a descubrir el mismo código en otra aplicación y oh, sorpresa, resulta que se trataba del mismo desarrollador, pero esta vez trabajando para una empresa diferente.

El problema, como siempre, es el time to market. Si realizas las revisiones antes de que el código vaya a producción, como no tengas un equipo de revisores potente, paralizas el lanzamiento de nuevos productos (ya se sabe, si no ganamos dinero es por tu culpa, que nos paras el desarrollo). En cambio, si realizas revisiones una vez el código ya esta en producción, cuesta muchísimo que lo acaben modificando (total, si ya funciona en producción para que lo vamos a cambiar!).

Joder, vaya comentario más largo me ha salido. ¿Hay por ahí algún revisor que le quiera echar un vistazo? Y ojo con lo que decís, que me enfado fácilmente ;-)


Martín dijo...
0:51

Grande y enormemente jugoso, como los buenos chuletones. Excelente comentario Ferdy.


Alex dijo...
11:41

Yo hacia code reviews en mi anterior empresa y es lo mejor que he experimentado nunca.

Al principio me sentia como "de que va este tio, de que sabes mas que yo?" pero el senior tambien pedia que revisaran su codigo y eso suavizaba ese sentimiento y te hacia darte cuenta de que al final solo se buscaba el beneficio comun.

Ademas, se comparte el conocimiento del codigo de una manera extraordinaria: cuando llega un bug o una nueva historia, cualquier miembro del equipo pude llevarla a cabo. Si alguien se pone enfermo o deja la empresa, el proyecto no se resiente, etc.

En mi nuevo empresa lo hemos intentado pero no ha funcionado muy bien basicamente por problemas de plazos. De todas formas, me ha servido para darme cuenta de lo valiosisimos que son los code reviews. Sin ellos, noto que el equipo esta cojo. Veo fallos tontos por todas partes, soluciones complicadas...

Muy buen articulo, por cierto!


Martín dijo...
18:52

Gracias por compartir la experiencia Alejandro. Los "code reviews" suelen ser de las primeras cosas que caen cuando estamos apretados de tiempo.

En cierto modo puedo entenderlo, cuando las agendas se aprietan es difícil sincronizar gente. Yo trataría de sustituirlo con una hora de pair programming al día o cada dos días. Sentarse parejas del equipo y discutir como se ha hecho esto y aquello otro.

Y si no hacéis Scrum, pues podéis coger alguna práctica como una reunión diaria entre todos explicando como habéis implementado esto y aquello otro. 15 minutillos, no más.