Conciliar la regla de Boy Scout y la refactorización oportunista con revisiones de código

55

Soy un gran creyente en la Regla de los Boy Scouts :

Siempre revise un módulo más limpio que cuando lo revisó ". No importa quién sea el autor original, ¿qué pasaría si siempre hiciéramos algún esfuerzo, no importa cuán pequeño, para mejorar el módulo? ¿Cuál sería el resultado? Creo que si todos seguían esa simple regla, veríamos el final del incesante deterioro de nuestros sistemas de software. En cambio, nuestros sistemas mejorarían gradualmente a medida que evolucionaran. También veríamos equipos cuidando el sistema en su conjunto, más bien que solo las personas que cuidan su propia pequeña parte.

También creo firmemente en la idea relacionada de la refactorización oportunista :

Aunque hay lugares para algunos esfuerzos de refactorización programados, prefiero alentar la refactorización como una actividad oportunista, realizada cuando sea y donde sea que el código necesite ser limpiado, por quien sea. Lo que esto significa es que en cualquier momento que alguien vea algún código que no sea tan claro como debería ser, debería aprovechar la oportunidad para solucionarlo allí mismo o en ese momento, o al menos en unos minutos.

Particularmente tenga en cuenta el siguiente extracto del artículo de refactorización:

Tengo cuidado con las prácticas de desarrollo que causan fricción para la refactorización oportunista ... Mi sensación es que la mayoría de los equipos no refactorizan lo suficiente, por lo que es importante prestar atención a cualquier cosa que desaliente a las personas a hacerlo. Para ayudar a eliminar esto, tenga en cuenta cada vez que se sienta desanimado de realizar una pequeña refactorización, una que está seguro que solo tomará uno o dos minutos. Cualquier barrera de este tipo es un olor que debería provocar una conversación. Por lo tanto, tome nota del desánimo y créelo con el equipo. Por lo menos, debe discutirse durante su próxima retrospectiva.

Donde trabajo, hay una práctica de desarrollo que causa mucha fricción: la revisión de código (CR). Cada vez que cambio algo que no está dentro del alcance de mi "asignación", mis revisores me reprochan que haga que el cambio sea más difícil de revisar. Esto es especialmente cierto cuando se trata de refactorizar, ya que dificulta la comparación de diferencias "línea por línea". Este enfoque es el estándar aquí, lo que significa que la refactorización oportunista rara vez se realiza, y solo se lleva a cabo la refactorización "planificada" (que generalmente es muy poco, demasiado tarde), si es que se realiza.

Afirmo que los beneficios valen la pena, y que 3 revisores trabajarán un poco más duro (para comprender realmente el código antes y después, en lugar de analizar el alcance limitado de qué líneas cambiaron; la revisión en sí misma sería mejor solo por eso ) para que se beneficien los próximos 100 desarrolladores que leen y mantienen el código. Cuando presento este argumento, mis revisores dicen que no tienen ningún problema con mi refactorización, siempre que no esté en el mismo CR. Sin embargo, afirmo que esto es un mito:

(1) La mayoría de las veces solo te das cuenta de qué y cómo quieres refactorizar cuando estás en medio de tu tarea. Como dice Martin Fowler:

A medida que agrega la funcionalidad, se da cuenta de que parte del código que está agregando contiene cierta duplicación con algún código existente, por lo que debe refactorizar el código existente para limpiar las cosas ... Puede que algo funcione, pero tenga en cuenta que sería mejor si se cambia la interacción con las clases existentes. Aproveche la oportunidad de hacer eso antes de considerarse terminado.

(2) Nadie te mirará favorablemente al liberar CR "refactorizadoras" que no debías hacer. Un CR tiene ciertos gastos generales y su gerente no quiere que "pierda su tiempo" en la refactorización. Cuando se incluye con el cambio que se supone que debes hacer, este problema se minimiza.

Resharper exacerba el problema, ya que cada archivo nuevo que agrego al cambio (y no puedo saber de antemano exactamente qué archivos terminarían modificados) generalmente está lleno de errores y sugerencias, la mayoría de los cuales son correctos y merecen totalmente fijación.

El resultado final es que veo un código horrible, y lo dejo allí. Irónicamente, siento que arreglar ese código no solo no mejorará mi posición, sino que en realidad los bajará y me pintará como el tipo "desenfocado" que pierde el tiempo arreglando cosas que a nadie le importan en lugar de hacer su trabajo. Me siento mal porque realmente desprecio el mal código y no puedo soportar verlo, ¡mucho menos llamarlo desde mis métodos!

¿Alguna idea sobre cómo puedo remediar esta situación?

t0x1n
fuente
40
Me sentiría incómodo trabajando en un lugar dondeyour manager doesn't want you to "waste your time" on refactoring
Daenyth
19
Además de tener múltiples CR, el punto clave es que cada confirmación debe ser para un solo propósito: uno para el refactorizador, uno para el requisito / error / etc. De esa manera, una revisión puede diferenciar entre el refactorizador y el cambio de código solicitado. También diría que el refactorizador solo debe hacerse si hay pruebas unitarias establecidas que prueben que su refactor no rompió nada (el tío Bob está de acuerdo).
2
@ t0x1n No veo eso como algo diferente
Daenyth
2
@ t0x1n sí, me perdí esa. No hay suficiente café esta mañana. En mi experiencia, hay algunas maneras de refactorizar. Tal vez mire el código que necesita modificar e inmediatamente sepa que necesita limpieza, así que haga eso primero. Quizás tenga que refactorizar algo para realizar su cambio porque el nuevo requisito es incompatible con el código existente. Yo diría que el refactor es intrínsecamente parte de su cambio y no debe considerarse por separado. Finalmente, tal vez vea que el código apesta a la mitad de su cambio, pero puede terminarlo. Refactorizar después del hecho.
66
La viñeta 1 no afirma que la separación de los commits sea imposible. Simplemente implica que no sabes cómo hacerlo, o tu VCS lo dificulta. Hago esto todo el tiempo, incluso tomando un solo compromiso y dividiéndolo después del hecho.
Inútil

Respuestas:

18

OK, ahora hay más cosas aquí de las que son adecuadas para un comentario.

tl; dr

Su intuición sobre lo que debe hacer (refactorizando a medida que avanza) es correcta.

Su dificultad para implementar esto, dado que debe evitar un sistema de revisión de código deficiente, se reduce a su dificultad para manipular su código fuente y VCS. Varias personas han dicho que puede y debe dividir sus cambios (sí, incluso dentro de los archivos) en múltiples confirmaciones, pero parece tener dificultades para creer esto.

Usted realmente puede hacer esto. Eso es realmente lo que estamos sugiriendo. Usted realmente debe aprender a sacar el máximo provecho de sus herramientas de edición, manipulación de la fuente y de control de versiones. Si invierte el tiempo en aprender a usarlos bien, la vida es mucho más fácil.


Flujo de trabajo / cuestiones de política de oficina

Recomendaría esencialmente la misma recomendación que GlenH7 de que cree dos confirmaciones: una con solo refactorizaciones y (demostrable u obviamente) sin cambios funcionales, y otra con los cambios funcionales en cuestión.

Sin embargo, puede ser útil, si encuentra muchos errores, elegir una sola categoría de error para corregir dentro de un solo CR. Luego tiene una confirmación con un comentario como "código de deduplicación", "corregir errores tipo X" o lo que sea. Debido a que esto hace un solo tipo de cambio, presumiblemente en múltiples lugares, debería ser trivial de revisar. Significa que no puede corregir cada error que encuentre, pero puede hacer que sea menos doloroso pasar de contrabando.


Problemas de VCS

Dividir los cambios realizados en su fuente de trabajo en múltiples confirmaciones no debería ser un desafío. No ha dicho lo que está usando, pero los posibles flujos de trabajo son:

  1. si estás usando git, tienes excelentes herramientas para esto

    • puede usar git add -ipara la puesta en escena interactiva desde la línea de comandos
    • puede usar git guiy seleccionar líneas y líneas individuales para organizar (esta es una de las pocas herramientas GUI relacionadas con VCS que en realidad prefiero a la línea de comandos, y la otra es un buen editor de combinación de 3 vías)
    • puede realizar muchas confirmaciones pequeñas (cambios individuales o corregir la misma clase de error en varios lugares) y luego reordenarlas, fusionarlas o dividirlas con rebase -i
  2. si no está usando git, su VCS puede tener herramientas para este tipo de cosas, pero no puedo aconsejarle sin saber qué sistema usa

    Mencionó que está usando TFS, que creo que es compatible con git desde TFS2013. Puede valer la pena experimentar con un clon git local del repositorio para trabajar. Si está deshabilitado o no funciona para usted, aún puede importar la fuente en un repositorio git local, trabajar allí y usarlo para exporta tus confirmaciones finales.

  3. puede hacerlo manualmente en cualquier VCS si tiene acceso a herramientas básicas como diffy patch. Es más doloroso, pero ciertamente posible. El flujo de trabajo básico sería:

    1. realiza todos tus cambios, pruebas, etc.
    2. use diffpara hacer un archivo de parche (contexto o unificado) con todos los cambios desde la última confirmación
    3. particione eso en dos archivos: terminará con un archivo de parche que contiene refactorizaciones, y uno con cambios funcionales
      • Esto no es del todo trivial: las herramientas como el modo emacs diff pueden ayudar
    4. respaldar todo
    5. volver a la última confirmación, usar patchpara reproducir uno de los archivos de revisión, confirmar esos cambios
      • NÓTESE BIEN. Si el parche no se aplicó limpiamente, es posible que deba reparar los trozos fallidos manualmente
    6. repita 5 para el segundo archivo de parche

Ahora tiene dos confirmaciones, con sus cambios particionados adecuadamente.

Tenga en cuenta que probablemente haya herramientas para facilitar esta administración de parches, simplemente no las uso porque git lo hace por mí.

Inútil
fuente
No estoy seguro de seguir: ¿Bullet 3.3 asume que la refactorización y los cambios funcionales residen en diferentes archivos? En cualquier caso, no lo hacen. Quizás la separación por líneas tiene más sentido, pero no creo que tengamos herramientas para esto en nuestro CVS (TFS). En cualquier caso, no funcionaría para muchas (¿la mayoría?) Refactorizaciones donde los cambios funcionales dependen de los cambios refactorizados. Por ejemplo, supongamos que refactorizo ​​el método Foo (que necesito usar como parte de mi cambio funcional) para tomar 3 parámetros en lugar de 2. Ahora el código funcional Imy se basa en el código refactorizador, incluso dividir por línea no ayudará.
t0x1n
1
Diferentes líneas en el mismo archivo están bien en los flujos de trabajo dados. Y dado que las dos confirmaciones serían secuenciales , está perfectamente bien que la segunda confirmación (funcional) dependa de la primera. Ah, y TFS2013 supuestamente admite git.
Inútil
También usamos TFS para el control de origen. Asume que el segundo commit será el funcional, mientras que típicamente sería lo contrario (ya que no puedo decir de antemano qué refactorización debería hacerse). Supongo que podría hacer todo mi trabajo de refactorización funcional +, luego deshacerme de todo lo funcional y agregarlo nuevamente en una confirmación por separado. Solo digo que es una molestia (y tiempo) solo para mantener contentos a un par de revisores. Un enfoque más razonable para mi mente es permitir la refactorización oportunista y, a cambio, aceptar los cambios de CR usted mismo (aceptando la dificultad adicional).
t0x1n
3
Creo que realmente no me estás entendiendo. Editar la fuente y agrupar las ediciones en confirmaciones, sí, incluso las ediciones en el mismo archivo, son actividades lógicamente separadas. Si esto parece difícil, solo necesita aprender mejor las herramientas de administración de código fuente disponibles.
Inútil
1
Sí, su comprensión es correcta, tendría dos confirmaciones secuenciales con la segunda (funcional) dependiendo de la primera (refactorización). El flujo de trabajo diff / patch descrito anteriormente es precisamente una forma de hacerlo que no requiere eliminar manualmente los cambios y luego volver a escribirlos.
Inútil
29

Asumiré que las Solicitudes de cambio son grandes y formales en su empresa. Si no es así, solo realice los cambios (cuando sea posible) en muchas confirmaciones pequeñas (como se supone que debe hacerlo).

¿Alguna idea sobre cómo puedo remediar esta situación?

¿Continuar haciendo lo que estás haciendo?

Quiero decir, todos tus pensamientos y deducciones son completamente correctos. Deberías arreglar las cosas que ves. La gente no planifica refactorizar lo suficiente. Y ese beneficio para todo el grupo es más importante que molestar a algunos.

Lo que puede ayudar es ser menos combativo. Las revisiones de código no deberían ser combativas "¿Por qué hicieron eso?", Deberían ser colaborativas "¡Hola chicos, mientras estuve aquí arreglé todas estas cosas!". Trabajar (con su líder / gerente si es posible) para cambiar esa cultura es difícil, pero bastante vital para crear un equipo de desarrollo de alto funcionamiento.

También puede trabajar (con su líder / gerente si es posible) para avanzar la importancia de estas ideas con sus colegas. Hazlo preguntando "¿por qué no les importa la calidad?" en lugar de preguntar "¿por qué siempre haces estas cosas inútiles?".

Telastyn
fuente
55
Sí, los CR son grandes y formales. Los cambios son CRed, firmados y luego enviados a una cola. Se supone que los pequeños cambios deben agregarse como iteraciones a un CR en curso en lugar de confirmarse por separado. WRT continúa con lo que estoy haciendo, eso puede beneficiar al grupo, pero me temo que no me beneficiará a . Esas personas a las que "incomodo" son probablemente las mismas personas que me calificarían en la revisión anual. El problema con cambiar la cultura es que los grandes jefes creen en ella. Quizás solo necesito ganar más respeto en sus ojos antes de intentar algo así ...
t0x1n
13
@ t0x1n - No me mires. He hecho una carrera haciendo lo correcto frente a las personas que están tercamente obligadas a chupar. Tal vez no sea tan rentable de lo que podría haber sido si hiciera feliz a la gente, pero duermo bien.
Telastyn
Gracias por ser honesto. De hecho, es algo para contemplar.
t0x1n
1
A menudo me encuentro con esto también. Asegurarme de que tengo un parche de "limpieza" y luego un parche de trabajo ayuda mucho. Usualmente peleo dentro del sistema y luego me voy a trabajar a un lugar que es menos estresante. Dicho esto, a veces hay razones válidas para las preocupaciones de tus compañeros de trabajo. Por ejemplo, si el código entra rápidamente en producción y no hay suficientes pruebas. He visto la revisión de código como un intento de evitar las pruebas. No funciona. La revisión de código ayuda a mantener un cuerpo de código uniforme. Hace poco por los errores.
Sean Perry
1
@SeanPerry agregado - pero estoy hablando de circunstancias normales donde existen pruebas, se realizarán
incrustaciones de
14

Tengo mucha simpatía por su situación, pero pocas sugerencias concretas. Por lo menos, quizás te convenza de que, por muy mala que sea la situación, podría ser peor. SIEMPRE puede ser peor. :-)

Primero, creo que tienes (al menos) dos problemas con tu cultura, no solo uno. Un problema es el enfoque de refactorización, pero las revisiones de código parecen ser un problema distinto. Trataré de separar mis pensamientos.

Revisiones de código

Estaba en un grupo que ODIAba las revisiones de código. El grupo se formó fusionando dos grupos de partes separadas de la compañía. Vengo del grupo que había estado haciendo revisiones de código durante varios años, con resultados generalmente buenos. La mayoría de nosotros creía que las revisiones de código eran un buen uso de nuestro tiempo. Nos fusionamos en un grupo más grande y, por lo que podíamos ver, ese "otro" grupo nunca había oído hablar de revisiones de código. Ahora todos estábamos trabajando en "su" base de código.

Las cosas no estaban bien cuando nos fusionamos. Las nuevas características se retrasaron entre 6 y 12 meses, año tras año. La acumulación de errores fue enorme, creciente y agotadora. La propiedad del código era fuerte, especialmente entre los "gurús" más importantes. Las "ramas de características" a veces duraban años y abarcaban algunos lanzamientos; a veces NADIE, pero un solo desarrollador vio el código antes de que llegara a la rama principal. En realidad, la "rama de características" es engañosa, ya que sugiere que el código estaba en algún lugar del repositorio. Más a menudo fue solo en el sistema individual del desarrollador.

La gerencia acordó que necesitábamos "hacer algo" antes de que la calidad se volviera inaceptablemente baja. :-) Su respuesta fue Revisiones de código. Las revisiones de código se convirtieron en un elemento oficial de "Thou Shalt", para preceder a cada registro. La herramienta que utilizamos fue la Junta de Revisión.

¿Cómo funcionó en la cultura que describí? Mejor que nada, pero fue doloroso y tomó más de un año alcanzar una especie de nivel mínimo de cumplimiento. Algunas cosas que observamos:

  1. La herramienta que usa tiende a enfocar las revisiones de código de ciertas maneras. Esto puede ser un problema. Review Board le ofrece diferencias bonitas y coloridas línea por línea y le permite adjuntar comentarios a una línea. Esto hace que la mayoría de los desarrolladores ignoren todas las líneas que no cambiaron. Eso está bien para pequeños cambios aislados. No es tan bueno para grandes cambios, grandes trozos de código nuevo o código que tiene 500 instancias de una función renombrada mezclada con 10 líneas de nueva funcionalidad.
  2. A pesar de que estábamos en una base de código vieja y enferma que en su mayoría nunca antes había sido revisada, se volvió "descortés" que un revisor comentara cualquier cosa que NO fuera una línea de cambio, incluso para señalar un error obvio. "No me molesten, este es un registro importante y no tengo tiempo para corregir errores".
  3. Compre un revisor "fácil". Algunas personas mirarán una revisión de 10 archivos con 2000 líneas cambiadas durante 3 minutos y harán clic en "¡Enviarlo!". Todos aprenden rápidamente quiénes son esas personas. Si realmente no desea que su código sea revisado en primer lugar, envíelo a un revisor "fácil". Su registro no se ralentizará. Puede devolver el favor convirtiéndose en un revisor "fácil" para su código.
  4. Si odia las revisiones de códigos, simplemente ignore los correos electrónicos que recibe de la Junta de Revisión. Ignora los seguimientos de los miembros de tu equipo. Por semanas. Hasta que tenga 3 docenas de reseñas abiertas en su lista y su nombre aparezca en las reuniones de grupo varias veces. Luego conviértase en un revisor "fácil" y borre todas sus revisiones antes de la hora del almuerzo.
  5. Evite enviar su código a un revisor "duro". (El tipo de desarrollador que haría o respondería una pregunta como esta). Todos aprenden rápidamente quiénes son los críticos "duros", así como aprenden los fáciles. Si las revisiones de código son una pérdida de tiempo (™), leer comentarios detallados sobre el código que USTED TIENE es tanto una pérdida de tiempo (™) como un insulto.
  6. Cuando las revisiones de códigos son dolorosas, las personas las posponen y siguen escribiendo más códigos. Obtiene menos revisiones de código, pero cada una es GRANDE. Necesita más revisiones de código más pequeñas, lo que significa que el equipo necesita descubrir cómo hacerlas lo menos dolorosas posible.

Pensé que iba a escribir algunos párrafos sobre Revisiones de Código, pero resulta que estaba escribiendo sobre cultura. Supongo que todo se reduce a esto: las revisiones de código pueden ser buenas para un proyecto, pero un equipo solo obtiene los beneficios que merece.

Refactorización

¿Mi grupo despreciaba la refactorización aún más de lo que odiaba las revisiones de código? ¡Por supuesto! Por todas las razones obvias que ya se han mencionado. ¡Estás perdiendo el tiempo con una revisión de código asquerosa, y ni siquiera estás agregando una función o arreglando un error!

Pero el código todavía necesitaba desesperadamente una refactorización. ¿Cómo proceder?

  1. Nunca mezcle un cambio de refactorización con un cambio funcional. Varias personas han mencionado este punto. Si las revisiones de código son un punto de fricción, no aumente la fricción. Es más trabajo, pero debe planear una revisión por separado y un registro por separado.
  2. Empieza pequeño. Muy pequeña. Incluso más pequeño que eso. Utilice refactorizaciones muy pequeñas para enseñar gradualmente a las personas que la refactorización y las revisiones de código no son pura maldad. Si puede escabullirse en una pequeña refactorización por semana sin demasiado dolor, en un par de meses podría salirse con dos por semana. Y pueden ser un poco más grandes. Mejor que nada.
  3. Básicamente no tuvimos pruebas unitarias. Entonces la refactorización está prohibida, ¿verdad? No necesariamente. Para algunos cambios, el compilador es su prueba de unidad. Concéntrese en refactorizaciones que pueda probar. Evite los cambios si no se pueden probar. Tal vez pase el tiempo escribiendo pruebas unitarias en su lugar.
  4. Algunos desarrolladores tienen miedo de refactorizar porque tienen miedo de TODOS los cambios de código. Me tomó mucho tiempo reconocer este tipo. Escriben un trozo de código, juguetean con él hasta que "funciona", y NUNCA quieren cambiarlo. No necesariamente entienden POR QUÉ funciona. El cambio es arriesgado. No confían en sí mismos para realizar NINGÚN cambio, y ciertamente no confiarán en ti. Se supone que la refactorización se trata de cambios pequeños y seguros que no alteran el comportamiento. Hay desarrolladores para quienes la idea misma de un "cambio que no altera el comportamiento" es inconcebible . No sé qué sugerir en estos casos. Creo que debes tratar de trabajar en áreas que no les importan. Me sorprendió cuando supe que este tipo puede tener mucho tiempo,
GranitoRobert
fuente
1
Esta es una respuesta muy reflexiva, ¡gracias! Estoy especialmente de acuerdo con cómo la herramienta de RC puede afectar el enfoque de la revisión ... línea por línea es la salida fácil, eso no implica comprender realmente lo que estaba sucediendo antes y lo que sucede ahora. Y, por supuesto, el código que no cambió es perfecto, no hay necesidad de mirar eso ...
t0x1n
1
" Podría ser peor. Podría estar lloviendo". Cuando leí su último párrafo (segundo punto # 4) estaba pensando: necesitan más revisión, revisión del codificador . Y algunas refactorizaciones, como en: "yourSalary = 0"
Absolutamente perfecto en MUCHOS frentes, respuesta increíble. Puedo ver totalmente de dónde vienes: estoy exactamente en la misma situación, y es increíblemente frustrante. Usted está en esta lucha constante por la calidad y las buenas prácticas, y no hay cero apoyo no solo de la administración, sino ESPECIALMENTE de otros desarrolladores del equipo.
julealgon
13

¿Por qué no haces ambas cosas, pero con commits separados?

Tus compañeros tienen un punto. Una revisión de código debe evaluar el código que fue cambiado por otra persona. No debe tocar el código que está revisando para otra persona, ya que sesga su rol como revisor.

Pero si ve una serie de problemas evidentes, hay dos opciones que puede seguir.

  1. Si la revisión del código estuvo bien, permita que la parte que revisó se confirme y luego refactorice el código bajo un segundo registro.

  2. Si la revisión del código tuvo problemas que necesitaban corrección, solicite al desarrollador que refactorice según las recomendaciones de Resharper.


fuente
De su respuesta deduzco que usted cree en la Propiedad del Código Fuerte. Te animo a leer los pensamientos de Fowler sobre por qué es una mala idea: martinfowler.com/bliki/CodeOwnership.html . Para abordar sus puntos específicamente: (1) es un mito, ya que la refactorización ocurre mientras realiza el cambio, no antes ni después de una manera separada, limpia y sin relación, como lo requerirían los CR separados. (2) Con la mayoría de los desarrolladores, nunca sucederá. Nunca . La mayoría de los desarrolladores no se preocupan por estas cosas, mucho menos cuando provienen de otro tipo que no es su gerente. Tienen sus propias cosas que quieren hacer.
t0x1n
8
@ t0x1n: Si a sus gerentes no les importa la refactorización, y a sus colegas desarrolladores no les importa la refactorización ... entonces esa compañía se está hundiendo lentamente. ¡Huir! :)
logc
8
@ t0x1n solo porque hagas los cambios juntos, no significa que tengas que comprometerlos juntos. Además, a menudo es útil verificar que su refactorización no haya tenido efectos secundarios inesperados, aparte de verificar que su cambio funcional tuvo el efecto esperado. Obviamente, esto puede no aplicarse a todas las refactorizaciones, pero no es un mal consejo en general.
Inútil
2
@ t0x1n: no recuerdo haber dicho nada sobre la propiedad de códigos fuertes Mi respuesta fue mantener puro tu papel como revisor. Si un revisor introduce cambios, ya no son solo un revisor.
3
@ GlenH7 Quizás hubo algún malentendido aquí: no soy el revisor. Solo estoy codificando lo que necesito y me encuentro con un código que puedo mejorar en el proceso. Mis críticos luego se quejan cuando lo hago.
t0x1n
7

Personalmente odio la idea de las revisiones de código post commit. La revisión del código debería suceder, a medida que realiza los cambios en el código. Lo que, por supuesto, estoy hablando aquí es la programación de pares. Cuando se empareja, obtiene la revisión de forma gratuita y obtiene una mejor revisión del código. Ahora, estoy expresando mi opinión aquí, sé que otros comparten esto, probablemente hay estudios que prueban esto.

Si puede hacer que sus revisores de códigos se emparejen con usted, el elemento combativo de la revisión de códigos debería evaporarse. Cuando comienza a hacer un cambio que no se entiende, la pregunta puede plantearse en el punto de cambio, discutirse y explorar alternativas que pueden conducir a mejores refactorizaciones. Obtendrá una revisión de código de mayor calidad ya que el par podrá comprender el alcance más amplio del cambio y no centrarse tanto en los cambios línea por línea, que es lo que obtiene con la comparación de códigos lado a lado.

Por supuesto, esta no es una respuesta directa al problema de que las refactorizaciones estén fuera del alcance del cambio en el que se está trabajando, pero esperaría que sus pares entiendan mejor el razonamiento detrás de los cambios si estuvieran allí cuando hiciera el cambio.

Por otro lado, suponiendo que esté haciendo TDD o alguna forma de refactor rojo verde, una forma de asegurarse de obtener el compromiso de su compañero es utilizar la técnica de emparejamiento de ping pong. Simplemente se explica que el controlador se gira para cada paso del ciclo RGR, es decir, el par 1 escribe una prueba fallida, el par 2 lo repara, el par 1 refactores, el par 2 escribe una prueba fallida ... y así sucesivamente.

daffers
fuente
Excelentes puntos. Lamentablemente, dudo sinceramente que pueda cambiar "el sistema". A veces, los revisores son de diferentes zonas horarias y ubicaciones geográficas, por lo que, en tales casos, no volará independientemente
t0x1n
6

Probablemente este problema refleje un problema mucho mayor con la cultura de la organización. La gente parece más interesada en hacer "su trabajo" correctamente que en mejorar el producto, probablemente esta compañía tiene una cultura de "culpa" en lugar de una cultura de colaboración y la gente parece más interesada en cubrirse a sí misma que en tener una visión completa del producto / compañía .

En mi opinión, tienes toda la razón, las personas que revisan tu código están completamente equivocadas si se han quejado porque "tocas" cosas fuera de "tu asignación", intenta convencer a esas personas pero nunca estar en contra de tus principios, para mí esto es La cualidad más importante de un verdadero profesional.

Y si hacer lo correcto le da números malos de alguna manera estúpida de corporaciones para evaluar su trabajo, ¿cuál es el problema ?, ¿quién quiere "ganar" este juego de evaluación en una empresa loca ?, ¡trate de cambiarlo !, y si es imposible o estás cansado, encuentras otro lugar, pero nunca, nunca estés en contra de tus principios, es lo mejor que tienes.

AlfredoCasado
fuente
1
Empiezas a querer ganar el juego de evaluación una vez que te das cuenta de que te recompensa con aumentos salariales, bonificaciones y opciones sobre acciones :)
t0x1n
2
No. Estás intercambiando dinero por principios @ t0x1n. Simple como eso. Tiene tres opciones: trabajar para arreglar el sistema, aceptar el sistema, abandonar el sistema. Las opciones 1 y 3 son buenas para el alma.
Sean Perry
2
no solo es malo para el alma, una empresa con principios que se centran en máximas locales es normalmente menos óptima que una empresa que se centra en máximas globales. No solo esto, el trabajo no es solo dinero, usted pasa mucho tiempo cada día en su trabajo, se siente cómodo en su trabajo, siente que está haciendo lo correcto, probablemente sea mucho mejor que un montón de dólares más cada mes.
AlfredoCasado
1
Meh, las almas están sobrevaloradas;)
t0x1n
2
@SeanPerry Realmente intenté arreglar el sistema pero fue muy difícil. (1) Estaba prácticamente solo en esto, y es difícil ir en contra de los grandes jefes (solo soy un desarrollador regular, ni siquiera senior). (2) Estas cosas llevan un tiempo que simplemente no tenía. Hay mucho trabajo y todo el entorno consume mucho tiempo (correos electrónicos, interrupciones, CR, fallas en los ciclos de prueba que necesita corregir, reuniones, etc., etc.). Hago mi mejor esfuerzo para filtrar y ser productivo, pero normalmente apenas puedo terminar mi trabajo "prescrito" a tiempo (tener altos estándares no ayuda aquí), y mucho menos trabajar en cambiar el sistema ...
t0x1n
5

A veces, refactorizar es algo malo. Sin embargo, no por las razones que sus revisores de código están dando; parece que realmente no les importa la calidad del código, y a usted sí le importa, lo cual es increíble. Pero hay dos razones que deberían impedirle refactorizar : 1. No puede probar los cambios que realizó con pruebas automáticas (pruebas unitarias o lo que sea) o 2. Está realizando cambios en algún código que no comprende muy bien. bien; es decir, no tienes el conocimiento específico del dominio para saber qué cambios debes hacer.

1. No refactorice cuando no pueda probar los cambios que realizó con las pruebas automatizadas.

La persona que realiza la revisión de su código debe poder asegurarse de que los cambios que realizó no rompan nada de lo que solía funcionar. Esta es la razón más importante para dejar solo el código de trabajo. Sí, definitivamente sería mejor refactorizar esa función de 1000 líneas de largo (¡eso es realmente una abominación!) En un montón de funciones, pero si no puede automatizar sus pruebas, puede ser realmente difícil convencer a otros de que hizo todo derecho. Definitivamente he cometido este error antes.

Antes de refactorizar, asegúrese de que haya pruebas unitarias. Si no hay pruebas unitarias, ¡escriba algunas! Luego refactorice y sus revisores de código no tendrán excusas (legítimas) para estar molestos.

No refactorice piezas de código que requieran conocimientos específicos del dominio que no tiene.

El lugar donde trabajo tiene muchos códigos de ingeniería química. La base del código utiliza los modismos comunes a los ingenieros químicos. Nunca haga cambios que sean idiomáticos en un campo. Podría parecer una gran idea para cambiar el nombre de una variable llamada xa molarConcentration, pero ¿adivinen qué? En todos los textos de química, la concentración molar se representa con un x. Cambiarle el nombre hace que sea más difícil saber lo que realmente está sucediendo en el código para las personas en ese campo.

En lugar de renombrar variables idiomáticas, simplemente ponga comentarios explicando cuáles son. Si son no idiomática, por favor, cambiar el nombre de ellos. No deje el i, j, k, x, y, etc. flotando cuando mejores nombres funcionarán.

Regla general para las abreviaturas: si, cuando las personas hablan, usan una abreviatura, está bien usar esa abreviatura en el código. Ejemplos de la base del código en el trabajo:

Tenemos los siguientes conceptos que siempre abreviamos cuando hablamos de ellos: "Área de preocupación" se convierte en "AOC", "Explosión de nubes de vapor" se convierte en VCE, cosas así. En nuestra base de código, alguien refactorizó todas las instancias llamadas aoc a AreaOfConcern, VCE a vaporCloudExplosion, nPlanes a confiningPlanesCount ... lo que hizo que el código fuera mucho más difícil de leer para las personas que tenían el conocimiento específico del dominio. También he sido culpable de hacer cosas como esta.


Es posible que esto no se aplique en absoluto a su situación, pero tengo mis pensamientos sobre el tema.

Cody Piersall
fuente
Gracias Cody Con respecto a "sus revisores de código no tendrán una excusa (legítima) para estar molestos", su excusa ya es ilegítima, ya que están molestos por la mayor dificultad de revisar los cambios en lugar de la corrección, la legibilidad o algo así.
t0x1n
2

Esta pregunta ahora tiene dos problemas distintos: la facilidad de revisar los cambios en las revisiones de código y el tiempo dedicado a la refactorización.

Donde trabajo, hay una práctica de desarrollo que causa una gran fricción: la revisión de código (CR). Cada vez que cambio algo que no está dentro del alcance de mi "asignación", mis revisores me reprochan que haga que el cambio sea más difícil de revisar.

Como han dicho otras respuestas, ¿puede separar los registros de refactorización de los registros de cambio de código (no necesariamente como revisiones separadas)? Dependiendo de la herramienta que esté utilizando para hacer la revisión del código, debería poder ver las diferencias solo entre revisiones específicas (el Crucible de Atlassian definitivamente hace esto).

(2) Nadie te mirará favorablemente al liberar CR "refactorizadoras" que no debías hacer. Un CR tiene ciertos gastos generales y su gerente no quiere que "pierda su tiempo" en la refactorización. Cuando se incluye con el cambio que se supone que debes hacer, este problema se minimiza.

Si la refactorización es sencilla y hace que el código sea más fácil de comprender (que es todo lo que debe hacer si solo se refactoriza), entonces la revisión del código no debería tardar mucho en completarse y debería ser una sobrecarga mínima que se recupera en espadas cuando alguien tiene que venir y mirar el código nuevamente en el futuro. Si sus jefes no pueden entender esto, entonces es posible que deba empujarlos suavemente hacia algunos recursos que discutan por qué la Regla Boy Scout conduce a una relación más productiva con su código. Si puede convencerlos de que la "pérdida [de] su tiempo" ahora ahorrará dos, cinco o diez veces más tarde cuando usted / alguien más regrese a trabajar más en ese código, entonces su problema debería desaparecer.

Resharper exacerba el problema, ya que cada archivo nuevo que agrego al cambio (y no puedo saber de antemano exactamente qué archivos terminarían modificados) generalmente está lleno de errores y sugerencias, la mayoría de los cuales son correctos y merecen totalmente fijación.

¿Has intentado llamar la atención de tus colegas sobre estos temas y discutir por qué vale la pena solucionarlos? ¿Y alguno de ellos puede repararse automáticamente (suponiendo que tenga suficiente cobertura de prueba para confirmar que no ha roto cosas con la refactorización automática)? A veces no es "valioso su tiempo" realizar una refactorización si realmente es algo quisquilloso. ¿Todo tu equipo usa ReSharper? Si lo hacen, ¿tiene una política compartida sobre qué advertencias / reglas se aplican? Si no lo hacen, tal vez debería estar demostrando dónde la herramienta lo está ayudando a identificar áreas del código que son posibles fuentes de dolor en el futuro.

Chris Cooper
fuente
Con respecto a la separación CR, he señalado en mi publicación y en varios otros comentarios por qué creo que es imposible.
t0x1n
No estoy hablando de cosas quisquillosas, estoy hablando de problemas de rendimiento y corrección reales, así como de código redundante, código duplicado, etc. Y eso es solo cosas de R #, también hay algún código realmente malo que puedo solucionar fácilmente . Desafortunadamente, no todo mi equipo usa el compartidor, e incluso aquellos que lo hacen no lo toman demasiado en serio. Se necesita un esfuerzo educativo masivo, y tal vez trataré de liderar algo así. Sin embargo, es difícil, ya que apenas tengo suficiente tiempo para el trabajo que tengo que hacer, y mucho menos para proyectos de educación paralelos. Quizás solo necesito esperar un período de inactividad para explotar.
t0x1n
Voy a intervenir y decir que definitivamente no es imposible , ya que lo veo todo el tiempo. Haga el cambio que haría sin la refactorización, verifíquelo, luego refactorice para limpiar, y verifique eso. No es ciencia espacial. Ah, y prepárate para defender por qué consideras que vale la pena haber pasado el tiempo refactorizando y revisando el código refactorizado.
Eric King
@EricKing Supongo que podría hacer eso. Sin embargo: (1) Tendría que trabajar con código feo y mantener notas de lo que quiero mejorar hasta que termine con los cambios funcionales que apestan y en realidad ralentizan mi progreso funcional (2) Una vez que envíe mis cambios funcionales y volver a visitar mis notas para refactorizar, eso solo será la primera iteración y completar la refactorización puede llevar más tiempo, lo cual, como usted sugirió, me resultaría difícil explicar a mis gerentes, ya que mi trabajo ya está "hecho".
t0x1n
2
"Estoy hablando de problemas reales de rendimiento y corrección", entonces esto podría estar estirando la definición de refactorización; si el código es realmente incorrecto, constituiría una corrección de errores. En cuanto a los problemas de rendimiento, eso no es algo que solo deba solucionar como parte de un cambio de características, probablemente sea algo que necesita medición, pruebas exhaustivas y una revisión de código separada.
Chris Cooper
2

Puedo recordar hace 25 años más o menos el código de "limpieza" en el que estaba trabajando para otros fines, principalmente formateando los bloques de comentarios y las alineaciones de tabulación / columna para que el código sea ordenado y, por lo tanto, más fácil de entender (sin cambios funcionales reales) . Me gusta el código ordenado y ordenado. Bueno, los desarrolladores senior estaban furiosos. Resulta que usaron algún tipo de comparación de archivos (diff) para ver qué había cambiado, en comparación con sus copias personales del archivo, y ahora estaba dando todo tipo de falsos positivos. Debo mencionar que nuestra biblioteca de códigos estaba en un mainframe y no tenía control de versión: básicamente sobrescribió lo que había allí, y eso fue todo.

La moraleja de la historia? No se. Supongo que a veces no puedes complacer a nadie excepto a ti mismo. No estaba perdiendo el tiempo (en mis ojos): el código limpiado era más fácil de leer y entender. Es solo que las primitivas herramientas de control de cambios utilizadas por otros ponen un trabajo adicional de una sola vez en ellas. Las herramientas eran demasiado primitivas para ignorar el espacio / tabulación y los comentarios redistribuidos.

Phil Perry
fuente
Sí, también me mandan por espaciado, así como por cosas triviales, como fundición redundante, etc. Lo que mi cambio no exige por completo es "ruido".
t0x1n
2

Si puede dividir el cambio solicitado y el refactor no solicitado en (muchas) confirmaciones separadas, como lo señalaron @Useless, @Telastyn y otros, entonces eso es lo mejor que puede hacer. Seguirá trabajando en un único CR, sin la sobrecarga administrativa de crear uno "refactorizador". Simplemente mantenga sus compromisos pequeños y centrados.

En lugar de darle algunos consejos sobre cómo hacerlo, prefiero indicarle una explicación mucho más amplia (de hecho, un libro) de un especialista. De esta manera, podrás aprender mucho más. Lea Trabajo efectivo con código heredado (por Michael Feathers) . Este libro puede enseñarle cómo refactorizar, entrelazado con los cambios funcionales.

Los temas cubiertos incluyen:

  • Comprender la mecánica del cambio de software: agregar funciones, corregir errores, mejorar el diseño, optimizar el rendimiento
  • Obtener código heredado en un arnés de prueba
  • Escribir exámenes que lo protejan contra la introducción de nuevos problemas
  • Técnicas que se pueden usar con cualquier lenguaje o plataforma, con ejemplos en Java, C ++, C y C #
  • Identificar con precisión dónde deben realizarse cambios en el código
  • Hacer frente a sistemas heredados que no están orientados a objetos
  • Manejo de aplicaciones que no parecen tener ninguna estructura.

Este libro también incluye un catálogo de veinticuatro técnicas de ruptura de dependencia que lo ayudan a trabajar con elementos del programa de manera aislada y hacer cambios más seguros.

Hbas
fuente
2

Yo también soy un gran creyente en The Boyscout Rule y Opportunistic Refactoring. El problema a menudo está en lograr que la gerencia compre. La refactorización conlleva riesgos y costos. Lo que la administración a menudo pasa por alto es que también lo hace la deuda técnica.

Es la naturaleza humana. Estamos acostumbrados a enfrentar problemas reales, no a tratar de evitarlos. Somos reactivos, no proactivos.

El software es intangible y, por lo tanto, es difícil entender que, como un automóvil, necesita mantenimiento. Ninguna persona razonable compraría un automóvil y nunca lo repararía. Aceptamos que tal negligencia disminuiría su longevidad y, a la larga, sería más costosa.

A pesar del hecho de que muchos gerentes entienden esto, son responsables de los cambios en el código de producción. Hay políticas que hacen que sea más fácil dejar las cosas en paz. A menudo, la persona que necesita ser convincente está realmente por encima de su gerente y simplemente no quiere luchar para que sus "grandes ideas" entren en producción.

Para ser honesto, su gerente no siempre está convencido de que su remedio es, de hecho, tan bueno como cree que es. (¿Aceite de serpiente?) Hay ventas. Es su trabajo ayudarlo a ver el beneficio.

La gerencia no comparte sus prioridades. Póngase su sombrero de gestión y vea con ojos de gestión. Lo más probable es que encuentres el ángulo correcto. Se persistente. Efectuará más cambios al no permitir que la primera respuesta negativa lo detenga.

Mario T. Lanza
fuente
1

Si puede dividir el cambio solicitado y el refactor no solicitado en dos solicitudes de cambio separadas, como lo señaló @ GlenH7, entonces eso es lo mejor que puede hacer. Entonces no eres "el tipo que pierde el tiempo" sino "el tipo que trabaja el doble de duro".

Si a menudo se encuentra en una situación en la que no puede dividirlos, porque el trabajo solicitado ahora necesita el refactor no solicitado para compilar, le sugiero que insista en tener herramientas para verificar automáticamente los estándares de codificación, utilizando los argumentos descritos aquí (el Resharper las advertencias por sí solas pueden ser un buen candidato, no estoy familiarizado con eso). Todos esos argumentos son válidos, pero hay uno adicional que puede usar para su ventaja: tener esas pruebas ahora hace que sea su deber aprobar las pruebas en cada confirmación.

Si su empresa no quiere "perder el tiempo refactorizando" (una mala señal de su parte), entonces deben proporcionarle un archivo de "supresión de advertencia" (cada herramienta tiene su propio mecanismo) para que no se moleste más con tales advertencias Digo esto para proporcionarle opciones para diferentes escenarios, incluso en el peor de los casos. También es mejor tener acuerdos claramente establecidos entre usted y su empresa, también sobre la calidad del código esperado, en lugar de suposiciones implícitas en cada lado.

logc
fuente
Esta sería una buena sugerencia para un nuevo proyecto. Sin embargo, nuestra base de código actual es enorme, y Resharper emite muchos errores para la mayoría de los archivos. Simplemente es demasiado tarde para aplicarlo, y suprimir los errores existentes lo hace aún peor: los perderá en su nuevo código. También hay muchos errores, problemas y olores de código que un analizador estático no detectará, solo estaba dando advertencias de Resharper como ejemplo. Una vez más, podría haber redactado la "parte de desperdicio" con demasiada dureza, debería haber dicho algo como "dedicar tiempo a algo que no es una prioridad".
t0x1n
@ t0x1n: la regla de Boy Scout involucra principalmente los módulos que estás tocando. Eso puede ayudarlo a dibujar una primera línea divisoria. Suprimir advertencias no es una buena idea, lo sé, pero desde la perspectiva de la empresa, suprimirlas en un nuevo código es correcto y seguir sus convenciones, bueno, tal vez me estoy
dejando
1
esa es la parte frustrante! Solo toco archivos que hubiera tocado de todos modos como parte de mi tarea, ¡pero recibo quejas de todos modos! Las advertencias de los que estoy hablando son la advertencia no estilo, estoy hablando de rendimiento real y problemas de corrección, así como el código redundante, código duplicado, etc.
t0x1n
@ t0x1n: suena realmente muy frustrante. Tenga en cuenta que no quise decir solo "análisis de código estático" sino también otras recomendaciones, algo equivalente a Nexus. Por supuesto, ninguna herramienta atrapa el 100% de semántica; esto es solo una estrategia de remediación.
logc