Estoy reuniendo algunas pautas para las revisiones de código. Todavía no tenemos un proceso formal y estamos tratando de formalizarlo. Y nuestro equipo está distribuido geográficamente.
Estamos utilizando TFS para el control de origen (también lo utilizamos para tareas / seguimiento de errores / gestión de proyectos, pero migramos eso a JIRA ) con Visual Studio 2008 para el desarrollo.
¿Cuáles son las cosas que busca al hacer una revisión de código?
- Estas son las cosas que se me ocurrieron
- Hacer cumplir las reglas de FxCop (somos una tienda de Microsoft)
- Verifique el rendimiento (¿alguna herramienta?) Y la seguridad (pensando en usar OWASP - rastreador de código ) y la seguridad del hilo
- Adherirse a las convenciones de nomenclatura
- El código debe cubrir casos extremos y condiciones de límites.
- Debe manejar las excepciones correctamente (no tragar excepciones)
- Compruebe si la funcionalidad está duplicada en otro lugar
- El cuerpo de un método debe ser pequeño (20-30 líneas), y los métodos deben hacer una cosa y solo una cosa (sin efectos secundarios y evitar el acoplamiento temporal)
- No pase / devuelva nulos en métodos
- Evitar código muerto
- Documentar métodos / propiedades / variables públicas y protegidas
¿Qué otras cosas debemos tener en cuenta?
Estoy tratando de ver si podemos cuantificar el proceso de revisión (produciría resultados idénticos cuando lo revisen diferentes personas) Ejemplo: decir "el cuerpo del método no debe tener más de 20-30 líneas de código" en lugar de decir "el método el cuerpo debe ser pequeño ".
¿O es la revisión de código muy subjetiva (y diferiría de un revisor a otro)?
El objetivo es tener un sistema de marcado (digamos -1 punto por cada violación de la regla FxCop, -2 puntos por no seguir las convenciones de nomenclatura, 2 puntos por refactorización, etc.) para que los desarrolladores sean más cuidadosos cuando revisen su código. De esta manera, podemos identificar a los desarrolladores que constantemente escriben código bueno / malo. El objetivo es hacer que el revisor pase aproximadamente 30 minutos como máximo, para hacer una revisión (sé que esto es subjetivo, teniendo en cuenta el hecho de que el conjunto de cambios / revisión puede incluir múltiples archivos / grandes cambios en la arquitectura existente, etc., pero se obtiene La idea general es que el revisor no debe pasar días revisando el código de alguien).
¿Qué otro sistema objetivo / cuantificable sigue para identificar el código bueno / malo escrito por los desarrolladores?
Referencia del libro: Código limpio: un manual de artesanía de software ágil de Robert Martin .
fuente
this.Foo().FooBar().FooBarBar();Si el objeto devuelto aquí desde Foo es nulo, definitivamente puede evitar "Referencia de objeto no establecida a una instancia de un objeto" al llamar a FooBar ()Respuestas:
La calificación de las personas en una revisión es contraria a los sistemas más exitosos con los que he trabajado, tal vez todos. Pero el objetivo que he tratado de alcanzar durante más de 20 años es menos errores y una mayor productividad por hora de ingeniero. Si calificar a las personas es un objetivo, supongo que se podrían utilizar las revisiones. Nunca he visto una situación en la que se requiera, como trabajador o como líder.
Algunos estudios objetivos (Fagan, etc.) y mucha sabiduría popular sugieren que las relaciones entre pares facilitan las revisiones de código destinadas a reducir errores y aumentar la productividad. Los gerentes de trabajo pueden participar como trabajadores, pero no como gerentes. Se observan puntos de discusión, los cambios para satisfacer a los revisores generalmente son algo bueno pero no obligatorio. De ahí la relación entre pares.
Cualquier herramienta automatizada que pueda ser aceptada sin más análisis o juicio es buena - pelusa en C, C ++, Java. Recopilación regular. Los compiladores son REALMENTE buenos para encontrar errores de compilación. Documentar las desviaciones en los controles automáticos suena como una sutil acusación de los controles automáticos. Las directivas de código (como lo hace Java) que permiten las desviaciones son bastante peligrosas, en mi humilde opinión. Excelente para depurar, para permitirle entender el problema rápidamente. No es tan bueno de encontrar en un bloque de código de 50,000 líneas sin comentarios y mal documentado del que te has hecho responsable.
Algunas reglas son estúpidas pero fáciles de hacer cumplir; valores predeterminados para cada declaración de cambio, incluso cuando son inalcanzables, por ejemplo. Entonces es solo una casilla de verificación, y no tiene que gastar tiempo y dinero probando con valores que no coinciden con nada. Si tienes reglas , tendrás necedad , están inextricablemente unidas . El beneficio de cualquier regla debe valer la tontería que cuesta, y esa relación debe verificarse a intervalos regulares.
Por otro lado, "Corre" no es una virtud antes de la revisión, ni una defensa en revisión. Si el desarrollo siguió el modelo en cascada , le gustaría hacer la revisión cuando la codificación esté completa en un 85%, antes de encontrar y resolver errores complicados, porque la revisión es una forma más económica de encontrarlos. Dado que la vida real no es el modelo de cascada, cuándo revisar es algo así como un arte y equivale a una norma social. Las personas que realmente leerán su código y buscarán problemas en él son oro sólido. La administración que respalda esto de manera continua es una perla por encima del precio. Las revisiones deben ser como registros, temprano y con frecuencia .
He encontrado estas cosas beneficiosas:
1) No hay guerras de estilo . Donde van las llaves abiertas solo debe estar sujeto a una verificación de consistencia en un archivo dado. Todos iguales. Eso está bien, entonces. Ídem profundidad de sangría ** sy ** anchos de tabulación . La mayoría de las organizaciones descubren que necesitan un estándar común para la pestaña, que se utiliza como un gran espacio.
2) `Harapiento
texto que no
por contenido.`
Por cierto, K&R sangró cinco (CINCO) espacios, por lo que las apelaciones a la autoridad no tienen valor. Solo sé consistente.
3) Debe señalarse una copia del archivo que se revisará, numerada en línea, sin cambios y disponible públicamente, durante 72 horas o más antes de la revisión.
4) Sin diseño sobre la marcha. Si hay un problema o un problema, tenga en cuenta su ubicación y siga avanzando.
5) Las pruebas que atraviesan todos los caminos en el entorno de desarrollo son una muy, muy, muy buena idea. Las pruebas que requieren datos externos masivos, recursos de hardware, uso del sitio del cliente, etc., son pruebas que cuestan una fortuna y no serán exhaustivas.
6) Un formato de archivo no ASCII es aceptable si existen herramientas de creación, visualización, edición, etc., o si se crean al principio del desarrollo. Este es un sesgo personal mío, pero en un mundo donde el sistema operativo dominante no puede salir de su propio camino con menos de 1 gigabyte de RAM, no puedo entender por qué los archivos de menos de 10 megabytes deberían ser algo que no sea ASCII o algún otro formato comercialmente compatible. Existen estándares para gráficos, sonido, películas, ejecutables y herramientas que los acompañan. No hay excusa para un archivo que contiene una representación binaria de cierto número de objetos.
Para el mantenimiento, refactorización o desarrollo del código publicado, un grupo de compañeros de trabajo que utilicé revisó por otra persona, sentado en una pantalla y mirando un diferencial de lo antiguo y lo nuevo , como una puerta de entrada para el registro de sucursales. Me gustó, fue barato, rápido, relativamente fácil de hacer. Los recorridos para personas que no han leído el código por adelantado pueden ser educativos para todos, pero rara vez mejoran el código del desarrollador.
Si está distribuido geográficamente, mirar diferencias en una pantalla mientras habla con otra persona que mira lo mismo sería relativamente fácil. Eso cubre a dos personas que miran los cambios. Para un grupo más grande que ha leído el código en cuestión, múltiples sitios no es mucho más difícil que todos en una sola habitación. Varias habitaciones conectadas por pantallas de computadora compartidas y cajas de squak funcionan muy bien, en mi humilde opinión. Cuantos más sitios, más gestión de reuniones se necesita. Un gerente como facilitador puede ganarse la vida aquí. Recuerde seguir sondeando los sitios en los que no se encuentra.
En un momento, la misma organización tenía pruebas unitarias automatizadas que se usaban como pruebas de regresión. Eso estuvo muy bien. Por supuesto, luego cambiamos las plataformas y las pruebas automatizadas se quedaron atrás. La revisión es mejor, como señala el Manifiesto Ágil , las relaciones son más importantes que el proceso o las herramientas . Pero una vez que hayas revisado, las pruebas unitarias automatizadas / pruebas de regresión son la siguiente ayuda más importante para crear un buen software.
Si puede basar las pruebas en los requisitos , bueno, como dice la señora en "Cuando Harry conoció a Sally" , ¡tendré lo que está teniendo!
Todas las revisiones deben tener un estacionamiento para capturar los requisitos y los problemas de diseño en el nivel superior a la codificación. Una vez que se reconoce que algo pertenece al estacionamiento, la discusión debe detenerse en la revisión.
A veces creo que la revisión de código debería ser como revisiones esquemáticas en el diseño de hardware: completamente público, exhaustivo, tutorial, el final de un proceso, una puerta de enlace después de la cual se construye y se prueba. Pero las revisiones esquemáticas son pesadas porque cambiar los objetos físicos es costoso. Las revisiones de arquitectura, interfaz y documentación para el software probablemente deberían ser pesadas. El código es más fluido. La revisión del código debe ser más ligera.
En muchos sentidos, creo que la tecnología tiene que ver tanto con la cultura y las expectativas como con una herramienta específica. Piense en todas las improvisaciones de " Swiss Family Robinson " / Flintstones / McGyver que deleitan el corazón y desafían la mente. Queremos que nuestras cosas funcionen . No hay un solo camino para eso, al igual que no hubo "inteligencia" que de alguna manera podría ser abstraída y automatizada por los programas de inteligencia artificial de la década de 1960 .
fuente
La mayoría de los puntos que describió son solo una cuestión de formateo de código o cosas de "superficie":
Todo esto podría verificarse utilizando alguna herramienta automatizada : no es necesario que un desarrollador experimentado pase tiempo revisando el código para ver eso.
No sé nada sobre .NET , pero, para PHP , tenemos herramientas para verificar ese tipo de cosas; Teniendo en cuenta que a menudo se dice que .NET es "más industrial" que PHP, me sorprendería saber que no hay ninguna herramienta para verificar ese tipo de cosas.
La herramienta automatizada puede:
El correo se puede enviar a todo el equipo o al tipo que cometió el código que no pasa una prueba, o puede usar alguna interfaz web de informes (la misma nota sobre .NET y PHP)
También agregaría que las pruebas automatizadas pueden ayudar mucho, para detectar una cierta cantidad de errores antes de que el código se use en producción. Y las pruebas automatizadas también pueden ayudar con algunas métricas, supongo.
Las revisiones de código realizadas por desarrolladores experimentados también tienen otra gran ventaja de la que no habló:
Pero para una revisión de código que va más allá del formato de código, necesitará más de media hora ...
fuente
Mi experiencia con la revisión de código es que debería ser un esfuerzo combinado para mejorar el código, no una 'medida' para decidir quién es bueno o malo en su trabajo. Cuando no importa si recibe muchos comentarios durante la revisión del código, los revisores serán más estrictos y, por lo tanto, darán sugerencias para mejorar el código.
Para mejorar la calidad del código registrado, haga cumplir que los comentarios de revisión se procesen (deje que el revisor apruebe los comentarios procesados) y también use herramientas de verificación de código estático para forzar un nivel de calidad para la confirmación inicial.
fuente
Creo que su sistema de calificación es una mala idea. ¿Cual es el punto? ¿Identificar buenos y malos programadores? Todos en esa revisión de código pueden formar una evaluación sobre un programador en particular basado en el código presentado en la revisión de código mejor que alguna asignación arbitraria de valores a un conjunto de características algo arbitrario. Si desea identificar programadores buenos y malos ... pregúnteles a los programadores. Le garantizo que los humanos pueden hacer esta evaluación mejor que su tonta heurística.
Mi sugerencia sería intentar mejorar las revisiones de código para que las personas compartan ideas y opiniones abiertamente en un entorno sin prejuicios ni hostilidad. Si pudieras hacer eso, estarás 100 veces mejor que emitir juicios sobre los programadores basados en tus listas de verificación tontas que pretenden hacer un buen trabajo al evaluar a los programadores. Creo que muchos programadores ya están orgullosos y duros con ellos mismos si les va mal en las revisiones de código; Me pregunto si un "castigo" adicional por el bajo rendimiento es generalmente útil.
fuente
Mi único consejo sería evitar que el proceso de revisión de código sea demasiado estricto ; lo más importante es que la revisión de código realmente se realice y que se tome en serio .
Cuanto más agotador sea el proceso para el revisor, menos probable es que se realicen revisiones de código y que se tomen en serio en lugar de verse como una molestia. Además, el valor real de las revisiones de código radica en la capacidad del revisor para usar sus propias herramientas automatizadas de juicio que pueden usarse para verificar cosas como si se aprueban las reglas de FXCop.
fuente
Como regla general, evite pasar tiempo en una revisión de código haciendo algo que pueda hacer la máquina. Por ejemplo, su primer elemento es "hacer cumplir las reglas de FxCop", pero presumiblemente eso puede hacerlo FxCop sin que los humanos tengan que hacerlo también.
fuente
Si puede medirlo, si es objetivo, cuantificable, intente que una herramienta lo haga. Donde necesita un revisor experimentado es para las cosas subjetivas difusas.
fuente
Ya se han hecho muchos buenos comentarios sobre cuestiones de estilo, lo cual es importante. En un proyecto de equipo, es valioso que todo el código parezca que fue escrito por un solo autor. Esto facilita que otros miembros del equipo entren y solucionen problemas cuando ocurran. Las medidas cuantitativas que elija para garantizar este objetivo más amplio son menos importantes.
Un elemento adicional es garantizar que el código coincida con la arquitectura general acordada para el resto del sistema. Problemas similares deberían resolverse de la misma manera. Si la lógica de la aplicación se ha dividido en capas, ¿el código que se está revisando separa su funcionalidad de la misma manera que lo hace el resto del sistema? Alternativamente, ¿el código que se está revisando enseña algo nuevo que debería retirarse en el resto del sistema? Al igual que las comprobaciones de estilo aseguran que todo el código se vea igual, la revisión de la arquitectura debería garantizar que todo el código funcione de la misma manera. El énfasis aquí nuevamente está en la mantenibilidad. Cualquier persona en el equipo debería poder ingresar a este código y tener una idea de lo que está sucediendo de inmediato.
La idea de calificación parece un desastre en ciernes, pero conoces mejor a tu equipo. Es posible que este sistema los motive, pero creo que es más probable que las personas empiecen a preocuparse más por su calificación que por resolver problemas. Uno de los efectos secundarios realmente valiosos de las revisiones de código son las oportunidades de mentoría que ofrecen. El revisor debe tratar a la persona que escribió el código como alguien a quien está asesorando. Cada problema encontrado no es un problema, sino una oportunidad para crear un miembro del equipo más informado y sofisticado y un equipo más unido en general.
fuente
En realidad, francamente me importan más las cosas "subjetivas" que cualquier otra cosa. Lo que quiero de una buena revisión de código es que alguien verifique mi lógica, no mi escritura. Y eso es en lo que me centro cuando doy una revisión de código.
El formato general que me gusta tomar es:
Sin eso, solo mirar las diferencias tiende a dar información sobre problemas menores o puntos estilísticos. Me preocupa mucho más si la lógica es correcta, el enfoque utilizado en general es bueno y si la solución será mantenible.
Como ejemplo, recientemente vi un código de un compañero de trabajo. El problema original fue una violación de FxCop. Pero lo que se estaba haciendo era intentar determinar la presencia o ausencia de una función de Windows al verificar el número de versión. Mi principal aportación fue que esta era una forma frágil de hacerlo, y sería mejor consultar el servicio directamente, ya que la asignación entre la existencia de características y el sku de Windows podría cambiar en el futuro, y no era a prueba del futuro.
fuente
La complejidad ciclomática (CC) es una forma de evaluar el código que 'no está mal'.
En el código real que tiene un CC alto, tengo un alto factor "lo que está sucediendo aquí, no recuerdo". El código CC inferior es más fácil de entender.
Obviamente, las advertencias habituales se aplican a las métricas.
fuente
Las revisiones de código son subjetivas y objetivas. Reglas como "el cuerpo del método debe ser de 20-30 líneas" son subjetivas (algunas personas podrían pensar que 100 líneas están bien), pero si su empresa ha decidido que 20-30 líneas es el límite, está bien y puede medirlo. Creo que el sistema de puntos que se te ocurrió es una gran idea. Tendrá que volver a evaluarlo periódicamente, ya que encontrará que ciertas reglas deben tener más o menos peso en la puntuación, pero siempre que todos conozcan las reglas, parece un buen sistema.
Otras cosas que buscaría:
fuente
Parece que te estás volviendo demasiado detallado demasiado rápido. Deberías desglosarlo más. Debe observar el código por su calidad y por el cumplimiento de sus características. Deberías haberte separado y eso ni siquiera es el final de la historia ... así que aquí está mi sugerencia:
Código de calidad:
Cumplimiento de características:
Si puede cubrir estos dos aspectos de una revisión de código, está de oro.
fuente
Podría escribir algunos párrafos, pero solo pasaría por alto lo que Karl Wiegers explica en "Revisiones por pares en software: una guía práctica" . Creo que su libro contiene respuestas explícitas y concisas a su pregunta (y mucho más).
fuente
Depende.
Algunas partes de la revisión son fácilmente cuantificables (sin problemas de FxCop, sin errores de StyleCop , sin errores de CAT.NET , etc.)
Sin embargo, el estilo puede ser subjetivo, pero como usted dice, una vez que comienza a ser más específico (sin método> 20 líneas), puede medirlo, y las herramientas como NDepend pueden hacerlo automáticamente. Sin embargo, algunas cosas nunca serán automáticas: verificar el manejo de casos extremos requeriría pruebas para hacerlo, lo que muestra la cobertura del código, y el 100% es un ideal inalcanzable en muchos casos. La verificación de duplicación es difícil de hacer automáticamente. Verificaciones nulas, bueno, no estoy seguro de estar de acuerdo con usted, pero puede escribir reglas NDepend o reglas FxCop para eso.
Cuantas más herramientas, mejor, y si las herramientas permiten que los desarrolladores verifiquen su trabajo antes de confirmar cambios y que se realicen verificaciones como parte del proceso de CI , entonces minimizará la necesidad de revisiones.
fuente
Un sistema de marcado parece difícil de corregir, pero vale la pena tenerlo como herramienta de medición: no puede mejorar lo que no puede medir. Pero probablemente debería aceptar que algunas cosas serán difíciles / imposibles de cuantificar con precisión. Lo complicado será determinar cuántos puntos debe obtener cada calidad: por ejemplo, si se adhiere a las convenciones de nomenclatura se obtienen 2 puntos, ¿cuántos puntos por mantener pequeños los métodos?
Quizás algo mejor como una simple lista de verificación sería mejor, de modo que el código se pueda marcar como conforme, parcialmente conforme o no conforme a una calidad particular. Más tarde, puede agregar puntaje a la lista de verificación una vez que vea qué problemas de calidad surgen con mayor frecuencia o causan la mayoría de los problemas.
El proceso de revisión también debe ser lo suficientemente flexible como para permitir que el código falle en partes de la revisión, siempre que esto pueda justificarse y documentarse. ¡Cumplir ciegamente con algún estándar de calidad de código que hace que un componente se vuelva innecesariamente complejo / inmanejable es una mala idea!
fuente
Si desea que el código de las personas esté más estandarizado, sin hacer que "pierdan su tiempo formateando", como algunos desarrolladores se quejarán. Invierta en una herramienta como ReSharper, ya que hace que corregir el formato y otras tareas de refactorización sea un proceso casi automatizado.
fuente
fuente