¿Debo refactorizar el código que está marcado como "no cambiar"?

Parece que está refactorizando "por si acaso", sin saber exactamente qué partes del código base en detalle se cambiarán cuando se realice el desarrollo de la nueva característica. De lo contrario, sabría si hay una necesidad real de refactorizar los módulos frágiles, o si puede dejarlos como están.

Para decirlo con claridad: creo que esta es una estrategia de refactorización condenada . Usted está invirtiendo tiempo y dinero de su compañía para algo donde nadie sabe si realmente obtendrá un beneficio, y está a punto de empeorar las cosas al introducir errores en el código de trabajo.

Aquí hay una mejor estrategia: usa tu tiempo para

• agregue pruebas automáticas (probablemente no pruebas unitarias, sino pruebas de integración) a los módulos en riesgo. Especialmente los módulos frágiles que mencionaste necesitarán un conjunto de pruebas completo antes de cambiar algo allí.

• refactorice solo los bits que necesita para llevar a cabo las pruebas. Intenta minimizar cualquiera de los cambios necesarios. La única excepción es cuando sus pruebas revelan errores, luego corríjalos inmediatamente (y refactorice en la medida en que necesite hacerlo de manera segura).

• enseñe a sus colegas el "principio de boy scout" (también conocido como "refactorización oportunista" ), así que cuando El equipo comienza a agregar nuevas funciones (o para corregir errores), deben mejorar y refactorizar exactamente las partes de la base de código que necesitan cambiar, ni menos, ni más.

• obtenga una copia del libro de Feather "Trabajo eficaz con el código heredado" para el equipo.

Entonces, cuando llegue el momento en que sepa con seguridad deberá cambiar y refactorizar los módulos frágiles (ya sea por el desarrollo de nuevas funciones o porque las pruebas que agregó en el paso 1 revelan algunos errores) ), entonces usted y su equipo están listos para refactorizar los módulos, e ignorar de forma más o menos segura esos comentarios de advertencia.

Como respuesta a algunos comentarios : para ser justos, si uno sospecha que un módulo en un producto existente puede ser la causa de los problemas de forma regular, especialmente un módulo que está marcado como "don No toques ", estoy de acuerdo con todos ustedes. Debe revisarse, depurarse y, probablemente, volver a ajustarse en ese proceso (con el apoyo de las pruebas que mencioné, y no necesariamente en ese orden). Los errores son una fuerte justificación para el cambio, a menudo más fuerte que las nuevas características. Sin embargo, esta es una decisión caso por caso. Uno tiene que comprobar con mucho cuidado si realmente vale la pena cambiar algo en un módulo marcado como "no tocar".

Sí, debes refactorizar el código antes de agregar las otras funciones.

El problema con comentarios como estos es que dependen de las circunstancias particulares del entorno en el que se ejecuta el código base. El tiempo de espera programado en un punto específico puede haber sido realmente necesario cuando se programó.

Pero hay varias cosas que podrían cambiar esta ecuación: cambios de hardware, cambios de sistema operativo, cambios no relacionados en otro componente del sistema, cambios en volúmenes de datos típicos, lo que sea. No hay garantía de que en la actualidad aún sea necesario, o de que aún sea suficiente (la integridad de lo que se suponía que debía proteger podría haberse roto durante mucho tiempo, sin pruebas de regresión adecuadas que quizás nunca note). Esta es la razón por la que la programación de un retraso fijo para permitir que otro componente finalice es casi siempre incorrecta y solo funciona de manera accidental.

En su caso, no conoce las circunstancias originales y tampoco puede preguntar a los autores originales. (Es de suponer que tampoco tiene las pruebas de regresión / integración adecuadas, o simplemente puede seguir adelante y dejar que sus pruebas le indiquen si rompió algo).

Esto podría parecer un argumento para no cambiar nada por precaución; pero usted dice que de todos modos tendrá que haber cambios importantes, por lo que ya existe el peligro de alterar el delicado equilibrio que solía lograrse en este lugar. Es mucho mejor molestar el carrito de manzanas ahora , cuando lo único que estás haciendo es refactorizar, y estar seguro de que si las cosas se rompen fue la refactorización lo que lo causó, que esperar hasta que están realizando cambios adicionales simultáneamente y nunca estén seguros.

  

Mi pregunta es: ¿debo refactorizar el código cuando encuentro advertencias de los autores?

No, o al menos no todavía. Usted implica que el nivel de pruebas automatizadas es muy bajo. Necesita pruebas antes de poder refactorizar con confianza.

  

por ahora ya no podemos agregar ninguna función sin romper otra cosa

En este momento, debe centrarse en aumentar la estabilidad, no en refactorizar. Podría hacer refactorización como parte del aumento de la estabilidad, pero es una herramienta para lograr su objetivo real: una base de código estable.

Parece que esto se ha convertido en una base de código heredada, por lo que debe tratarlo de manera un poco diferente.

Comience agregando pruebas de caracterización. No se preocupe por ninguna especificación, solo agregue una prueba que afirme el comportamiento actual. Esto ayudará a evitar que el nuevo trabajo rompa las características existentes.

Cada vez que solucione un error, agregue un caso de prueba que demuestre que el error está solucionado. Esto evita las regresiones directas.

Cuando agrega una nueva función, agregue al menos algunas pruebas básicas de que la nueva función funciona según lo previsto.

¿Quizás obtenga una copia de "Trabajar de manera efectiva con el código heredado"?

  

Me dieron unos meses para refactorizar el código existente.

Comience por obtener cobertura de prueba. Comience con las áreas que más se rompen. Comience con las áreas que más cambian. Luego, una vez que haya identificado los malos diseños, reemplácelos uno por uno.

Usted casi nunca hace un solo refactor, sino un flujo constante de pequeños refactores cada semana. Un gran refactor tiene un gran riesgo de romper cosas y es difícil probarlo bien.

Recuerde la cerca de GK Chesterton : no desmonte una cerca que obstruye un camino hasta que entienda por qué fue construido.

Puede encontrar el (los) autor (es) del código y los comentarios en cuestión, y consultarlos para obtener el acuerdo. Puede consultar los mensajes de confirmación, las hebras de correo electrónico o los documentos, si existen. Entonces podrá refactorizar el fragmento marcado o anotará su conocimiento en los comentarios para que la próxima persona que mantenga este código pueda tomar una decisión más informada.

Tu objetivo es llegar a un entendimiento de lo que hace el código y por qué se marcó con una advertencia en ese entonces, y qué pasaría si se ignorara la advertencia.

Antes de eso, no tocaría el código marcado "no tocar".

El código con comentarios como los que mostraste sería el primero en mi lista de cosas para refactorizar si, y solo si tengo alguna razón para hacerlo . El punto es que el código apesta tanto que incluso lo hueles a través de los comentarios. Es inútil tratar de incorporar cualquier funcionalidad nueva en este código, ya que dicho código debe morir tan pronto como deba cambiarse.

También tenga en cuenta que los comentarios no son útiles en lo más mínimo: solo avisan y no tienen ninguna razón. Sí, son las señales de advertencia alrededor de un tanque de tiburones. Pero si quieres hacer algo en la vecindad, hay pocos puntos para tratar de nadar con los tiburones. Imho, primero debes deshacerte de esos tiburones.

Dicho esto, es absolutamente necesario que haya buenos casos de prueba antes de poder atreverse a trabajar en dicho código. Una vez que tenga esos casos de prueba, asegúrese de comprender cada pequeña parte de lo que está cambiando, para asegurarse de que realmente no está cambiando el comportamiento. Convierta en su máxima prioridad mantener todas las peculiaridades de comportamiento del código hasta que pueda probar que no tienen ningún efecto. Recuerda, estás tratando con tiburones, debes tener mucho cuidado.

Por lo tanto, en el caso del tiempo de espera: déjelo ahí hasta que entienda exactamente lo que espera el código, y luego determine la razón por la que se introdujo el tiempo de espera antes de proceder a eliminarlo.

También, asegúrese de que su jefe entienda qué es un esfuerzo en el que se está embarcando y por qué es necesario. Y si dicen que no, no lo hagas. Absolutamente necesitas su apoyo en esto.

Yo digo adelante, cámbiala. Lo creas o no, no todos los programadores son genios y una advertencia como esta significa que podría ser un lugar para mejorar. Si encuentra que el autor tenía razón, podría (jadear) DOCUMENTO o EXPLICAR el motivo de la advertencia.

Probablemente no sea una buena idea refactorizar el código con tales advertencias, si las cosas funcionan bien como están.

Pero si debe refactorizar ...

Primero, escriba algunas pruebas unitarias y pruebas de integración para probar las condiciones sobre las cuales las advertencias le advierten. Intente imitar las condiciones similares a la producción tanto como sea posible . Esto significa duplicar la base de datos de producción en un servidor de prueba, ejecutar los mismos servicios en la máquina, etc ... todo lo que pueda hacer. Luego intente su refactorización (en una rama aislada, por supuesto). Luego ejecute sus pruebas en su nuevo código para ver si puede obtener los mismos resultados que con el original. Si puede, entonces es probablemente Aceptar implementar su refactorización en producción. Pero prepárate para deshacer los cambios si las cosas van mal.

Estoy ampliando mis comentarios a una respuesta porque creo que algunos aspectos del problema específico se pasan por alto o se usan para sacar conclusiones erróneas.

En este punto, la pregunta de si se debe refactorizar es prematura (aunque probablemente se responderá con una forma específica de "sí").

El problema central aquí es que (como se señaló en algunas respuestas) los comentarios que cita indican claramente que el código tiene condiciones de carrera u otros problemas de concurrencia / sincronización, como se comentó aquí . Estos son problemas particularmente difíciles, por varias razones. En primer lugar, como ha descubierto, los cambios aparentemente no relacionados pueden desencadenar el problema (otros errores también pueden tener este efecto, pero los errores de concurrencia casi siempre lo hacen). En segundo lugar, son muy difíciles de diagnosticar: el error a menudo se manifiesta en un lugar que es distante en el tiempo o el código de la causa, y cualquier cosa que haga para diagnosticarlo puede hacer que desaparezca ( Heisenbugs ). En tercer lugar, los errores de concurrencia son muy difíciles de encontrar en las pruebas. En parte, esto se debe a la explosión combinatoria: es lo suficientemente malo para el código secuencial, pero agregar los posibles entrecruzamientos de la ejecución concurrente lo hace estallar hasta el punto en que el problema secuencial se vuelve insignificante en comparación. Además, incluso un buen caso de prueba solo puede desencadenar el problema ocasionalmente: Nancy Leveson calculó que uno de los errores letales en el Therac 25 ocurrió en 1 de aproximadamente 350 ejecuciones, pero si no sabe cuál es el error o si existe una, no sabe cuántas repeticiones hacen una prueba efectiva. Además, solo es posible realizar pruebas automáticas a esta escala, y es posible que el controlador de prueba imponga restricciones de tiempo sutiles, de modo que nunca vuelva a activar el error (Heisenbugs de nuevo).

Hay algunas herramientas para realizar pruebas de concurrencia en algunos entornos, como Helgrind para el código que usa POSIX pthreads , pero no conocemos los detalles aquí. Las pruebas deben complementarse con un análisis estático (¿o es al revés?), Si existen herramientas adecuadas para su entorno.

Para aumentar la dificultad, los compiladores (e incluso los procesadores, en tiempo de ejecución) a menudo tienen la libertad de reorganizar el código de manera que a veces hacen que el razonamiento sobre la seguridad de sus subprocesos sea muy poco intuitivo (quizás el caso más conocido es el idioma de bloqueo con doble control , aunque algunos entornos (Java, C ++ ...) se han modificado para mejorarlo. )

Este código puede tener un problema simple que está causando todos los síntomas, pero es más probable que tenga un problema sistémico que podría hacer que sus planes para agregar nuevas funciones se detengan. Espero haberlo convencido de que podría tener un problema grave en sus manos, posiblemente incluso una amenaza existencial para su producto, y lo primero que debe hacer es averiguar qué está sucediendo. Si esto revela problemas de concurrencia, le recomiendo encarecidamente que los solucione primero, antes incluso de preguntarse si debería hacer una refactorización más general, y antes de intentar agregar más funciones.

  

Estoy tratando con una base de código bastante grande y me dieron unos meses para refactorizar el código existente. El proceso de refactorización es necesario porque pronto tendremos que agregar muchas características nuevas a nuestro producto [...]

     

Durante la refactorización, de vez en cuando me encuentro con la clase, el método o las líneas de código que tienen comentarios como ["¡no toques esto!"]

Sí, debería refactorizar esas partes especialmente . Esos avisos fueron colocados allí por los autores anteriores para decir "no manipular ociosamente estas cosas, es muy complejo y hay muchas interacciones inesperadas". A medida que su empresa planea desarrollar aún más el software y agregar muchas características, le han asignado específicamente la tarea de limpiar todo esto. Así que no estás ocioso manipulándolo, deliberadamente se encarga de limpiarlo.

Averigüe qué hacen esos módulos complicados y divídalo en problemas más pequeños (lo que deberían haber hecho los autores originales). Para obtener el código de mantenimiento de un desorden, las partes buenas deben ser refactorizadas y las partes malas deben ser reescritas .

Esta pregunta es otra variación en el debate de cuándo / cómo refactorizar y / o limpiar el código con un guión de "cómo heredar el código". Todos tenemos experiencias diferentes y trabajamos en diferentes organizaciones con diferentes equipos y culturas, por lo que no hay una respuesta correcta o incorrecta, excepto "haz lo que creas que debes hacer y hazlo de una manera que no te despida". .

No creo que haya muchas organizaciones que se tomen las molestias de tener un proceso de negocio en su lado porque la aplicación de soporte necesita limpieza de código o refactorización.

En este caso específico, los comentarios de código están activando el indicador de que no se debe cambiar estas secciones de código. Entonces, si continúa y el negocio cae de su lado, no solo no tiene nada que mostrar para respaldar sus acciones, sino que hay un artefacto que funciona en contra de su decisión.

Entonces, como siempre es el caso, debe proceder con cuidado y hacer cambios solo después de comprender cada aspecto de lo que está a punto de cambiar, y encontrar formas de probar el problema prestando mucha atención a la capacidad y el rendimiento y tiempo debido a los comentarios en el código.

Pero aún así, su administración necesita comprender el riesgo inherente a lo que está haciendo y estar de acuerdo en que lo que está haciendo tiene un valor comercial que supera el riesgo y que ha hecho lo que se puede hacer para mitigar ese riesgo.

Ahora, volvamos a nuestros propios TODO y las cosas que sabemos se pueden mejorar en nuestras propias creaciones de código, si solo hubiera más tiempo.

Sí, absolutamente. Estos son claros indicios de que la persona que escribió este código no estaba satisfecha con el código y probablemente lo hurgó hasta que funcionó. Es posible que no entendieran cuáles eran los problemas reales o, peor, si los entendieron y eran demasiado perezosos para solucionarlos.

Sin embargo, es una advertencia de que se necesitará un gran esfuerzo para solucionarlos y que dichas correcciones tendrán riesgos asociados con ellos.

Idealmente, podrás averiguar cuál fue el problema y solucionarlo correctamente. Por ejemplo:

  

Tiempo de espera establecido para dar al Módulo A algo de tiempo para hacer cosas. Si no está cronometrado de esta manera, se romperá.

Esto sugiere fuertemente que el Módulo A no indica correctamente cuándo está listo para ser utilizado o cuando ha terminado de procesarse. Tal vez la persona que escribió esto no quiso molestarse en arreglar el Módulo A o no pudo por alguna razón. Esto parece un desastre en espera de suceder porque sugiere una dependencia de tiempo tratada por la suerte en lugar de una secuencia adecuada. Si viera esto, me gustaría mucho arreglarlo.

  

No cambies esto. Confía en mí, vas a romper cosas.

Esto no te dice mucho. Dependería de lo que el código estaba haciendo. Podría significar que tiene lo que parecen ser optimizaciones obvias que, por una razón u otra, realmente romperán el código. Por ejemplo, puede suceder que un bucle deje una variable en un valor particular del que depende otra pieza de código. O una variable podría probarse en otro hilo y cambiar el orden de las actualizaciones de variables podría romper ese otro código.

  

Sé que usar setTimeout no es una buena práctica, pero en este caso tuve que usarlo.

Esto parece fácil. Debería poder ver qué está haciendo setTimeout y quizás encontrar una mejor manera de hacerlo.

Dicho esto, si este tipo de arreglos están fuera del alcance de su refactor, estas son indicaciones de que tratar de refactorizar dentro de este código puede aumentar significativamente el alcance de su esfuerzo.

Como mínimo, observe detenidamente el código afectado y vea si al menos puede mejorar el comentario hasta el punto de que explique con mayor claridad cuál es el problema. Eso podría evitar que la siguiente persona se enfrente al mismo misterio que usted enfrenta.

El autor de los comentarios muy probablemente no entendió completamente el código por sí mismo . Si realmente supieran lo que estaban haciendo, habrían escrito comentarios realmente útiles (o no habrían introducido las condiciones de las carreras en primer lugar). Un comentario como " Confía en mí, romperás cosas. " para mí indica que el autor intenta cambiar algo que causó errores inesperados que no entendieron completamente.

El código probablemente se desarrolle a través de adivinanzas y prueba y error sin una comprensión completa de lo que realmente sucede.

Esto significa:

1. Es arriesgado cambiar el código. Tomará tiempo entenderlo, obviamente, y probablemente no siga los buenos principios de diseño y puede tener efectos y dependencias oscuros. Lo más probable es que no esté suficientemente probado, y si nadie entiende completamente lo que hace el código, será difícil escribir pruebas para asegurar que no se introduzcan errores o cambios en el comportamiento. Las condiciones de las carreras (como los comentarios aluden a) son especialmente onerosas: este es un lugar donde las pruebas unitarias no te salvarán.

2. Es arriesgado no cambiar el código. El código tiene una alta probabilidad de contener errores ocultos y condiciones de carrera. Si se descubre un error crítico en el código, o si un cambio de requisitos empresariales de alta prioridad lo obliga a cambiar este código con poca antelación, está en un problema profundo . Ahora tienes todos los problemas descritos en 1, ¡pero bajo presión de tiempo! Además, tales "áreas oscuras" en el código tienen una tendencia a propagarse e infectar otras partes del código que toca.

Otra complicación: Las pruebas unitarias no te salvarán . Por lo general, el enfoque recomendado para corregir este código heredado es agregar primero pruebas de unidad o de integración, y luego aislar y refactorizar. Pero las condiciones de carrera no pueden ser capturadas por pruebas automatizadas. La única solución es sentarse y pensar en el código hasta que lo entiendas, y luego volver a escribir para evitar las condiciones de carrera.

Esto significa que la tarea es mucho más exigente que la simple refactorización de rutina. Tendrá que programarlo como una tarea de desarrollo real.

Es posible que pueda encapsular el código afectado como parte de la refactorización regular, por lo que al menos el código peligroso está aislado.

La pregunta que me gustaría hacer es por qué alguien escribió, NO EDITAR en primer lugar.

He trabajado en un montón de código y parte de él es feo y tomó mucho tiempo y esfuerzo para trabajar, dentro de las restricciones dadas en ese momento.

Apuesto a que en este caso, esto sucedió y la persona que escribió el comentario encontró que alguien lo cambió y luego tuvo que repetir todo el ejercicio y volver a ponerlo de la manera correcta para que funcionara. Después de esto, por frustración de corte, escribieron ... NO EDITAR.

En otras palabras, no quiero tener que arreglar esto de nuevo porque tengo mejores cosas que hacer con mi vida.

Al decir NO EDITAR, es una forma de decir que sabemos todo lo que sabremos ahora y, por lo tanto, en el futuro nunca aprenderemos nada nuevo.

Debe haber al menos un comentario sobre las razones para no editar. Como decir "No tocar" o "No entrar". ¿Por qué no tocar la valla? ¿Por qué no entrar? ¿No sería mejor decir "Cerca eléctrica, no tocar"? o "¡Minas terrestres! ¡No entrar!". Entonces es obvio por qué, y aún así puedes ingresar, pero al menos conocer las consecuencias antes de hacerlo.

También apuesto a que el sistema no tiene pruebas en torno a este código mágico y, por lo tanto, nadie puede confirmar que el código funcionará correctamente después de cualquier cambio. Colocar pruebas de caracterización alrededor del código del problema siempre es un primer paso. Consulte "Trabajar con código heredado" por Michael Feathers para obtener sugerencias sobre cómo romper las dependencias y obtener el código bajo prueba, antes de abordar el cambio de código.

Creo que al final, se acortó para poner restricciones a la refactorización y dejar que el producto evolucione de forma natural y orgánica.