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

148

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 funciones nuevas a nuestro producto y, por ahora, ya no podemos agregar ninguna función sin romper otra cosa. En resumen: código desordenado, enorme y con errores, que muchos de nosotros hemos visto en sus carreras.

Durante la refactorización, de vez en cuando encuentro la clase, el método o las líneas de código que tienen comentarios como

Se agotó el tiempo de espera para que el Módulo A tenga tiempo para hacer cosas. Si no está programado así, se romperá.

o

No cambies esto. Confía en mí, romperás las cosas.

o

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

Mi pregunta es: ¿debería refactorizar el código cuando encuentro tales advertencias de los autores (no, no puedo ponerme en contacto con los autores)?

kukis
fuente
157
¡Felicidades! Ahora eres el autor del código.
12
No puede solucionarlo hasta que sepa tanto lo que se supone que debe hacer como lo que realmente hace (necesita conocer esto último para comprender todas las consecuencias de sus cambios). El problema parece estar sincronizado y eliminar tales problemas. debería refactorizar el trabajo n. ° 1, de lo contrario, las cosas empeorarán con cada cambio. La pregunta que debe hacerse es cómo hacer eso. Hay una buena cantidad de dependencia del idioma y la plataforma en esa pregunta. por ejemplo: stackoverflow.com/questions/3099794/finding-threading-bugs
sdenham
14
Continuando con el comentario de @ sdenham: si aún no tiene pruebas unitarias automatizadas que ejercen su código base, le sugiero que pase su tiempo creando tales pruebas unitarias, en lugar de perder el tiempo en una "refactorización de la caza de brujas" con AFAICT sin objetivos claros. en mente. Una vez que tenga un conjunto de pruebas unitarias que ejerciten a fondo su base de código, tendrá una forma objetiva de saber si su código aún funciona si / cuando tiene que reescribir bits de él. La mejor de las suertes.
Bob Jarvis el
17
Si los autores son tan paranoicos que el código se romperá si alguien respira cerca de él, es probable que ya esté roto y que el comportamiento indefinido resulte de la forma en que lo desean en este momento. Ese primero en particular parece que tienen una condición de carrera que han criticado, por lo que funciona la mayor parte del tiempo. Sdenham tiene razón. Averigua qué se supone que debe hacer y no asumas que eso es lo que realmente hace.
Ray
77
@ Ethan Puede que no sea tan simple. Cambiar el código puede romperlo de maneras que no son evidentes de inmediato. A veces se rompe mucho después de que hayas olvidado que este refactor actual podría haberlo causado, por lo que todo lo que tienes es un "nuevo" error con una causa misteriosa. Esto es especialmente probable cuando está relacionado con el tiempo.
Paul Brinkley

Respuestas:

225

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

Para decirlo directamente: creo que esta es una estrategia de refactorización condenada . Está invirtiendo tiempo y dinero de su empresa en algo en lo que nadie sabe si realmente le devolverá 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 esos módulos frágiles que mencionó necesitarán un conjunto de pruebas completo antes de cambiar algo allí.

  • refactorice solo los bits que necesita para poner las pruebas en su lugar. Intenta minimizar cualquiera de los cambios necesarios. La única excepción es cuando sus pruebas revelan errores, luego corríjalos de inmediato (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" ), de modo que cuando el equipo comience a agregar nuevas funciones (o corregir errores), deberían mejorar y refactorizar exactamente las partes de la base de código que necesitan cambiar, No menos, no más.

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

Entonces, cuando llegue el momento en que sepa con certeza que necesita cambiar y refactorizar los módulos quebradizos (ya sea por el desarrollo de nuevas características o porque las pruebas que agregó en el paso 1 revelan algunos errores), entonces usted y su equipo están listos para refactorice los módulos e ignore de manera 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 es la causa de problemas de forma regular, especialmente un módulo que está marcado como "no tocar", estoy de acuerdo con todos tú. Debe revisarse, depurarse y probablemente refactorizarse 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 una más fuerte que las nuevas características. Sin embargo, esta es una decisión caso por caso. Hay que comprobar con mucho cuidado si realmente vale la pena cambiar algo en un módulo que se marcó como "no tocar".

Doc Brown
fuente
70
Estoy tentado a votar en contra, pero me abstendré. He visto que esta mentalidad en múltiples proyectos conduce a un producto mucho peor. Una vez que finalmente ocurren las fallas, a menudo son catastróficas y mucho más difíciles de solucionar debido a su arraigo. Regularmente arreglo el lodo que veo, y parece causar en el peor de los casos tantos problemas como soluciona o, en el mejor de los casos, no deja problemas conocidos; En general, es una gran ventaja. Y el código es mejor para el desarrollo futuro. Una puntada a tiempo ahorra nueve, pero un problema ignorado puede empeorar.
Aaron
98
Yo diría que cualquier código marcado con un comentario "Se agotó el tiempo de espera para que el Módulo A tenga algo de tiempo para hacer cosas. Si no se sincroniza así, se romperá" ya tiene un error. Es solo suerte del sorteo que el error no está siendo golpeado.
17 de 26
10
@ 17of26: no necesariamente es un error. A veces, cuando trabaja con bibliotecas de terceros, se ve obligado a hacer todo tipo de concesiones de "elegancia" con el objetivo de que funcione.
whatsisname
30
@ 17of26: si se sospecha que un módulo en juego tiene errores, agregar pruebas antes de cualquier tipo de refactorización es aún más importante. Y cuando esas pruebas revelan el error, entonces tiene la necesidad de cambiar ese módulo y, por lo tanto, una legitimación para refactorizarlo de inmediato. Si las pruebas no revelan ningún error, mejor deje el código como está.
Doc Brown
17
@ Aaron: No entiendo por qué piensas que agregar pruebas o seguir el principio de boy scout en consecuencia reduciría la calidad del producto.
Doc Brown
142

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

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

Pero hay varias cosas que pueden cambiar esta ecuación: cambios de hardware, cambios de sistema operativo, cambios no relacionados en otro componente del sistema, cambios en los volúmenes de datos típicos, lo que sea. No hay garantía de que en la actualidad todavía sea necesario o de que sea suficiente (la integridad de lo que se suponía que debía proteger podría haberse roto durante mucho tiempo; sin las pruebas de regresión adecuadas, es posible que nunca se note). Esta es la razón por la que programar un retraso fijo para permitir que otro componente termine es casi siempre incorrecto y funciona solo accidentalmente.

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

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

Kilian Foth
fuente
46
La mejor respuesta aquí mismo; Lástima que sea un desvalido. La respuesta aceptada arriba no es un buen consejo, especialmente el énfasis en "condenado". He pasado el último año trabajando en el código más grande (~ 10 ^ 6LOC solo para mi sección), el más antiguo y horrible que he visto, y tengo el hábito de arreglar los restos del tren que veo cuando tengo el tiempo, incluso si no es parte del componente en el que estoy trabajando. Al hacer esto, he encontrado y corregido errores que el equipo de desarrollo ni siquiera sabía que existían (aunque nuestros usuarios finales probablemente sí). De hecho, tengo instrucciones de hacerlo. El producto se mejora como resultado.
Aaron
10
Sin embargo, no llamaría a eso refactorización, sino corrección de errores.
Paŭlo Ebermann
77
Mejorar el diseño de una base de código es refactorizar. Si el rediseño revela errores que pueden corregirse, eso es corregir errores. Mi punto es que el primero a menudo conduce al segundo, y que el segundo a menudo es muy difícil sin el primero.
Kramii
10
@Aaron, tienes suerte de contar con el apoyo de la gerencia / desarrollador para hacerlo. En la mayoría de los entornos, los errores conocidos y desconocidos causados ​​por un diseño y un código deficientes se aceptan como el orden natural de las cosas.
Rudolf Olah
55
@nocomprende Yo diría que si no comprende el sistema lo suficientemente bien como para escribir algunas pruebas, no tiene por qué cambiar cómo funciona.
Patrick M
61

Mi pregunta es: ¿debería refactorizar el código cuando encuentro tales 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. Puede refactorizar como parte del aumento de 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 trabajo nuevo rompa las funciones existentes.

Cada vez que arregle un error, agregue un caso de prueba que pruebe que el error está solucionado. Esto evita 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 eficazmente con código heredado"?

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

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

Casi nunca se puede hacer un gran refactorizador, 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.

Daenyth
fuente
10
+1 para agregar pruebas de caracterización. Es prácticamente la única forma de minimizar las regresiones al modificar el código heredado no probado. Si las pruebas comienzan a fallar una vez que comience a hacer cambios, puede verificar si el comportamiento anterior era realmente correcto dada la especificación, o si el nuevo comportamiento modificado es correcto.
Leo
2
Derecha. O si no hay una especificación utilizable, verifique si el comportamiento anterior es realmente deseado por las personas que pagan o tienen interés en el software.
bdsl
¿Quizás obtenga una copia de "Trabajar eficazmente con código heredado"? ¿Cuántas semanas le llevará leer ese libro? ¿Y cuándo: durante las horas de trabajo en el lugar de trabajo, en la noche cuando todos duermen?
Billal Begueradj
23

Recuerde la valla del GK Chesterton : no derribe una valla que obstruye un camino hasta que comprenda por qué se construyó.

Puede encontrar los autores del código y los comentarios en cuestión, y consultarlos para obtener la comprensión. Puede ver los mensajes de confirmación, hilos de correo electrónico o documentos si existen. Luego, podrá refactorizar el fragmento marcado o escribir su conocimiento en los comentarios para que la próxima persona que mantenga este código pueda tomar una decisión más informada.

Su objetivo es llegar a una comprensión de lo que hace el código, y por qué se marcó con una advertencia en ese momento, y qué sucedería si se ignorara la advertencia.

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

9000
fuente
44
OP dice "no, no puedo ponerme en contacto con los autores".
FrustratedWithFormsDesigner
55
No puedo ponerme en contacto con los autores ni hay documentación.
kukis
21
@kukis: ¿No puede contactar a los autores, a ningún experto en el dominio y no puede encontrar ningún correo electrónico / wiki / docs / casos de prueba relacionados con el código en cuestión? Bueno, entonces es un proyecto de investigación completo en arqueología de software, no una simple tarea de refactorización.
9000
12
No es inusual que el código sea antiguo y que los autores se hayan ido. Si terminaron trabajando para un competidor, discutirlo podría violar el NDA de alguien. En algunos casos, los autores no están disponibles permanentemente: no puedes preguntarle a Phil Katz sobre PKzip porque murió hace años.
pjc50
2
Si reemplaza "encontrar al autor" con "comprender qué problema resolvió el código", entonces esta es la mejor respuesta. A veces, estos fragmentos de código son la solución menos horrible para los errores que tomaron semanas o meses de trabajo para encontrar y rastrear y tratar, a menudo errores que los tipos normales de pruebas unitarias no pueden encontrar. (Aunque a veces son el resultado de programadores que no sabían lo que estaban haciendo. Su primera tarea es comprender cuál es)
grahamparks
6

El código con comentarios como los que mostraste estaría en la parte superior de mi lista de cosas para refactorizar si, y solo si tengo alguna razón para hacerlo . El punto es que el código apesta tan mal que incluso lo hueles a través de los comentarios. No tiene sentido tratar de incluir cualquier nueva funcionalidad en este código, dicho código debe morir tan pronto como necesite cambiar.

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

Dicho esto, absolutamente necesitas buenos casos de prueba antes de que puedas atreverte a trabajar en dicho código. Una vez que tenga esos casos de prueba, asegúrese de comprender cada poquito que está cambiando, para asegurarse de que realmente no está cambiando el comportamiento. Haga que su prioridad sea mantener todas las peculiaridades de comportamiento del código hasta que pueda probar que no tienen ningún efecto. Recuerde, está tratando con tiburones, debe tener mucho cuidado.

Por lo tanto, en el caso del tiempo de espera: déjelo hasta que comprenda exactamente qué está esperando el código y luego arregle la razón por la cual se introdujo el tiempo de espera antes de proceder a eliminarlo.

Además, asegúrese de que su jefe entienda qué esfuerzo emprenderá y por qué es necesario. Y si dicen que no, no lo hagas. Necesitas absolutamente su apoyo en esto.

cmaster
fuente
1
A veces, el código termina siendo un desastre porque debe funcionar correctamente en muestras de silicio de preproducción cuyo comportamiento real no coincide con la documentación. Dependiendo de la naturaleza de las soluciones, reemplazar el código puede ser una buena idea si el código nunca necesita ejecutarse en los chips defectuosos, pero cambiar el código sin el nivel de análisis de ingeniería que se requeriría del nuevo código probablemente no sea una buena idea. idea.
supercat
@supercat Uno de mis puntos fue que la refactorización debe preservar perfectamente el comportamiento. Si no conserva el comportamiento, es más que refactorizar en mi libro (y extremadamente peligroso cuando se trata de ese código heredado).
cmaster
5

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

Pero si debes refactorizar ...

Primero, escriba algunas pruebas unitarias y pruebas de integración para probar las condiciones sobre las que le advierten las advertencias. Intenta imitar las condiciones de producción tanto como sea posible . Esto significa duplicar la base de datos de producción a un servidor de prueba, ejecutar los mismos otros servicios en la máquina, etc., todo lo que pueda hacer. Luego intente refactorizar (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 probablemente esté bien implementar su refactorización en producción. Pero esté listo para revertir los cambios si las cosas salen mal.

FrustratedWithFormsDesigner
fuente
5

Yo digo adelante y cámbialo. 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.

JES
fuente
4

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

En este punto, la cuestión de si refactorizar es prematura (aunque probablemente sea respondida por una forma específica de 'sí').

El problema central aquí es que (como se señala en algunas respuestas) los comentarios que cita indican fuertemente que el código tiene condiciones de carrera u otros problemas de concurrencia / sincronización, como se discute aquí . Estos son problemas particularmente difíciles, por varias razones. En primer lugar, como ha encontrado, 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, eso se debe a la explosión combinatoria: es lo suficientemente malo para el código secuencial, pero agregar los posibles entrelazados de ejecución simultánea lo lleva al 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 Therac 25ocurrió en 1 de aproximadamente 350 ejecuciones, pero si no sabe cuál es el error, o incluso si hay uno, no sabe cuántas repeticiones hacen una prueba efectiva. Además, solo las pruebas automáticas son factibles a esta escala, y es posible que el controlador de prueba imponga restricciones de tiempo sutiles de modo que nunca active el error (Heisenbugs nuevamente).

Hay algunas herramientas para la prueba de concurrencia en algunos entornos, como Helgrind para el código utilizando hilos POSIX, pero no conocemos los detalles aquí. Las pruebas deben complementarse con 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 son libres de reorganizar el código de maneras que a veces hacen que el razonamiento sobre la seguridad de los subprocesos sea muy intuitivo (quizás el caso más conocido es la verificación doble modismo de bloqueo , 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 detener sus planes de agregar nuevas funciones. Espero haberte convencido de que podrías tener un problema grave en tus manos, posiblemente incluso una amenaza existencial para tu producto, y lo primero que debes hacer es descubrir qué está sucediendo. Si esto revela problemas de concurrencia, le recomiendo encarecidamente que los solucione primero, incluso antes de preguntar si debería realizar una refactorización más general, y antes de intentar agregar más funciones.

sdenham
fuente
1
¿Dónde ves "condición de carrera" en los comentarios? ¿Dónde está incluso implícito? Aquí está haciendo una gran cantidad de suposiciones técnicas sin siquiera el beneficio de ver el código.
Robert Harvey
2
@RobertHarvey "Se agotó el tiempo de espera para que el Módulo A tenga tiempo para hacer cosas". - más o menos la definición de una condición de carrera, y los tiempos de espera no son una forma difícil de manejarlos. No soy el único en hacer esta inferencia. Si no hay un problema aquí, está bien, pero el interlocutor debe saberlo, dado que estos comentarios son señales de alerta para una sincronización mal manejada.
sdenham
3

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 encuentro 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 . Esas advertencias fueron colocadas allí por los autores anteriores para que significaran "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 encargado específicamente la limpieza de estas cosas. Por lo tanto, no lo está manipulando ociosamente , está acusado deliberadamente de la tarea de limpiarlo.

Descubra qué están haciendo esos módulos difíciles y divídalos en problemas más pequeños (lo que los autores originales deberían haber hecho). Para sacar el código de mantenimiento de un desastre, las partes buenas deben refactorizarse y las partes malas deben reescribirse .

Daniel deprimido
fuente
2

Esta pregunta es otra variación en el debate sobre cuándo / cómo refactorizar y / o limpiar el código con un guión de "cómo heredar el código". Todos tenemos diferentes experiencias 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 tengan la bondad de que un proceso de negocios caiga de su lado porque la aplicación de soporte necesitaba limpieza o refactorización de código.

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

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

Pero aún así, su gerencia necesita comprender el riesgo inherente a lo que está haciendo y aceptar 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, regresemos a nuestros propios TODO y las cosas que sabemos pueden mejorarse en nuestras propias creaciones de código si solo hubiera más tiempo.

Thomas Carlisle
fuente
1

Si, absolutamente. Estas son indicaciones claras de que la persona que escribió este código no estaba satisfecha con el código y probablemente lo tocó hasta que funcionó. Es posible que no entendieran cuáles eran los problemas reales o, lo que es peor, que los entendieran y que fueran demasiado vagos para solucionarlos.

Sin embargo, es una advertencia de que se necesitará mucho esfuerzo para solucionarlos y que tales soluciones tendrán riesgos asociados con ellos.

Idealmente, podrá descubrir cuál era el problema y solucionarlo correctamente. Por ejemplo:

Se agotó el tiempo de espera para que el Módulo A tenga tiempo para hacer cosas. Si no está programado así, se romperá.

Esto sugiere fuertemente que el Módulo A no indica correctamente cuándo está listo para usarse o cuándo ha terminado el procesamiento. Quizás 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 a la espera de suceder porque sugiere una dependencia del tiempo que se trata por suerte en lugar de una secuencia adecuada. Si lo viera, me gustaría mucho arreglarlo.

No cambies esto. Confía en mí, romperás las cosas.

Esto no te dice mucho. Dependería de lo que estaba haciendo el código. 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 otro fragmento de código. O una variable podría probarse en otro hilo y cambiar el orden de las actualizaciones de las 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 lo que setTimeoutestá haciendo y quizás encontrar una mejor manera de hacerlo.

Dicho esto, si este tipo de correcciones están fuera del alcance de su refactorización, estas son indicaciones de que intentar 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 más claramente cuál es el problema. Eso podría salvar a la siguiente persona de enfrentar el mismo misterio que usted enfrenta.

David Schwartz
fuente
2
Es posible que las condiciones hayan cambiado hasta el punto en que los viejos problemas ya no existan, y los comentarios de advertencia se hayan vuelto como el lugar en los mapas antiguos que dicen: "Aquí hay dragones". Sumérjase y aprenda cuál era la situación, o vea que ha pasado a la historia.
2
En el mundo real, algunos dispositivos no proporcionan una indicación confiable de preparación, sino que especifican un tiempo máximo de no preparación. Las indicaciones de preparación confiables y eficientes son agradables, pero a veces uno está atrapado usando cosas sin ellas.
supercat
1
@supercat Quizás, quizás no. No podemos decir por el comentario aquí. Por lo tanto, vale la pena investigar, si nada más, mejorar el comentario con detalles.
David Schwartz
1
@DavidSchwartz: El comentario ciertamente podría ser más útil, pero a veces no está claro cuánto tiempo debe pasar un programador tratando de mapear todas las formas precisas en que un dispositivo no cumple con sus especificaciones, especialmente si no está claro si se espera el problema ser algo temporal o permanente.
supercat
1

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

El código probablemente se desarrolla 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. Obviamente, llevará tiempo entenderlo, y probablemente no siga buenos principios de diseño y pueda tener efectos y dependencias poco claros. Lo más probable es que no se pruebe lo suficiente, y si nadie comprende completamente lo que hace el código, entonces será difícil escribir pruebas para garantizar que no se introduzcan errores o cambios en el comportamiento. Las condiciones de carrera (como aluden los comentarios) 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 oscuros y condiciones de carrera. Si un error crítico es descubierto en el código, o una alta prioridad fuerzas de cambio requisito de negocio que permite cambiar el código en el corto plazo, se encuentra en lo profundo problema. Ahora tiene 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 extenderse 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 dicho código heredado es agregar primero pruebas unitarias o de integración, y luego aislar y refactorizar. Pero las condiciones de carrera no pueden capturarse mediante pruebas automatizadas. La única solución es sentarse y pensar en el código hasta que lo entienda, y luego reescribirlo para evitar las condiciones de carrera.

Esto significa que la tarea es mucho más exigente que solo la 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.

JacquesB
fuente
-1

La pregunta que haría es por qué alguien escribió, NO EDITE en primer lugar.

He trabajado en un montón de código y algunos de ellos son feos y me tomó mucho tiempo y esfuerzo ponerme a trabajar, dentro de las limitaciones dadas en ese momento.

Apuesto a que en este caso, esto ha sucedido y la persona que escribió el comentario descubrió que alguien lo cambió y luego tuvo que repetir todo el ejercicio y volver a ponerlo de la manera en que estaba para que funcionara. Después de esto, por pura frustración, escribieron ... NO EDITEN.

En otras palabras, no quiero tener que arreglar esto nuevamente ya que tengo mejores cosas que hacer con mi vida.

Al decir NO EDITAR, es una forma de decir que sabemos todo lo que vamos a saber en este momento 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 ingresar". ¿Por qué no tocar la cerca? ¿Por qué no entrar? ¿No sería mejor decir: "Cerca eléctrica, no tocar!" o "¡Minas terrestres! ¡No entren!". 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 es siempre un primer paso. Consulte "Trabajar con código heredado" de Michael Feathers para obtener consejos sobre cómo romper las dependencias y obtener el código bajo prueba, antes de abordar el cambio del código.

Creo que al final, es corto de vista para poner restricciones a la refactorización y dejar que el producto evolucione de una manera natural y orgánica.

BigA
fuente
2
Esto no parece ofrecer nada sustancial sobre los puntos hechos y explicados en las respuestas anteriores 9
mosquito