Devo refactoring il codice che è contrassegnato come "non cambiare"?

Sembra che tu stia rifattando "nel caso in cui", senza sapere esattamente quali parti del codebase in dettaglio saranno cambiate quando il nuovo sviluppo di funzionalità avrà luogo. Altrimenti, sapresti se c'è un reale bisogno di refactoring dei moduli fragili, o se puoi lasciarli così come sono.

Per dirlo chiaro: penso che questa sia una strategia di refactoring condannata . Stai investendo tempo e denaro della tua azienda per qualcosa in cui nessuno sa se restituirà davvero un vantaggio, e sei sul punto di peggiorare le cose introducendo bug nel codice di lavoro.

Ecco una strategia migliore: usa il tuo tempo per

• aggiungere test automatici (probabilmente non test di unità, ma test di integrazione) ai moduli a rischio. Soprattutto quei moduli fragili che hai citato avranno bisogno di una suite di test completa prima di cambiare qualcosa lì dentro.

• refactoring solo i bit necessari per portare i test in atto. Cerca di minimizzare le modifiche necessarie. L'unica eccezione è quando i tuoi test rivelano bug, quindi li aggiusta immediatamente (e ti aggiusta il livello necessario per farlo in sicurezza).

• insegna ai tuoi colleghi il "principio boy scout" (AKA "refactoring opportunistico" ), quindi quando il il team inizia ad aggiungere nuove funzionalità (o a correggere bug), dovrebbe migliorare e ridefinire esattamente le parti della base di codice di cui hanno bisogno per cambiare, non di meno, non di più.

• ottenere una copia del libro di Feather "Lavorare efficacemente con il codice legacy" per il team.

Quindi quando arriva il momento in cui sai di sicuro hai bisogno di cambiare e ridefinire i moduli fragili (sia a causa del nuovo sviluppo delle funzionalità, sia perché i test che hai aggiunto al passaggio 1 rivelano alcuni bug ), quindi tu e il tuo team siete pronti a rifattorizzare i moduli e ignorare più o meno in modo sicuro questi commenti di avviso.

Come risposta ad alcuni commenti : per essere onesti, se si sospetta che un modulo in un prodotto esistente sia la causa dei problemi su base regolare, specialmente un modulo contrassegnato come "don" 't touch ", sono d'accordo con tutti voi. Dovrebbe essere revisionato, sottoposto a debug e molto probabilmente refactored in quel processo (con il supporto dei test che ho citato, e non necessariamente in questo ordine). I bug sono una strong giustificazione per il cambiamento, spesso più strong delle nuove funzionalità. Tuttavia, questa è una decisione caso per caso. Si deve controllare molto attentamente se vale davvero la pena di cambiare qualcosa in un modulo che è stato contrassegnato come "non toccare".

Sì, dovresti rifattorizzare il codice prima di aggiungere le altre funzionalità.

Il problema con commenti come questi è che dipendono da circostanze particolari dell'ambiente in cui è in esecuzione il code base. Il timeout programmato in un punto specifico potrebbe essere stato veramente necessario quando è stato programmato.

Ma ci sono un certo numero di cose che potrebbero cambiare questa equazione: modifiche hardware, cambiamenti del sistema operativo, modifiche non correlate in un altro componente del sistema, cambiamenti nei tipici volumi di dati, è il nome. Non vi è alcuna garanzia che al giorno d'oggi sia ancora necessario, o che sia ancora sufficiente (l'integrità della cosa che si supponeva dovesse proteggere potrebbe essere stata rotta per un lungo periodo di tempo - senza adeguati test di regressione che non si sarebbero mai notati). Questo è il motivo per cui programmare un ritardo fisso per consentire a un altro componente di terminare è quasi sempre errato e funziona solo accidentalmente.

Nel tuo caso, non conosci le circostanze originali, né puoi chiedere agli autori originali. (Presumibilmente inoltre non hai test di regressione / integrazione adeguati, altrimenti potresti semplicemente andare avanti e lasciare che i tuoi test ti dica se hai rotto qualcosa.)

Potrebbe sembrare un argomento per non cambiare nulla per precauzione; ma tu dici che ci saranno comunque grandi cambiamenti, quindi il pericolo di sconvolgere il delicato equilibrio che si era ottenuto in questo punto è già lì. È molto meglio sconvolgere il carrello di mele ora , quando l'unica cosa che fai è il refactoring, e essere certo che se le cose si rompono è stato il refactoring a causarlo, piuttosto che aspettare fino a te stanno apportando ulteriori modifiche simultaneamente e non si è mai sicuri.

My question is: should I refactor the code when I encounter such warnings from the authors

No, o almeno non ancora. Implica che il livello dei test automatici sia molto basso. Hai bisogno di test prima di poter refactoring con fiducia.

for now we are no longer able to add any feature without breaking something else

In questo momento è necessario concentrarsi sull'aumento della stabilità, non sul refactoring. Potresti fare il refactoring come parte dell'aumento di stabilità, ma è uno strumento per raggiungere il tuo vero obiettivo - una base di codice stabile.

Sembra che questo sia diventato un codebase legacy, quindi devi trattarlo in modo un po 'diverso.

Inizia aggiungendo test di caratterizzazione. Non preoccuparti di nessuna specifica, aggiungi solo un test che asserisce sul comportamento corrente. Ciò consentirà di evitare che nuovi lavori interrompano le funzionalità esistenti.

Ogni volta che risolvi un bug, aggiungi un caso di test che dimostri che il bug è stato corretto. Questo impedisce regressioni dirette.

Quando aggiungi una nuova funzione, aggiungi almeno alcuni test di base che la nuova funzione funzioni come previsto.

Forse ottieni una copia di "Lavorare efficacemente con il codice legacy"?

I was given a few months to refactor existing code.

Inizia facendo salire la copertura del test. Inizia con le aree che si rompono di più. Inizia con le aree che cambiano di più. Quindi, una volta identificati i progetti non validi, sostituiscili uno per uno.

Non hai quasi mai uno da fare un enorme refactoring, ma invece un flusso costante di piccoli refactors ogni settimana. Un grande refactor ha un enorme rischio di rompere le cose, ed è difficile testare bene.

Ricorda recinzione del GK Chesterton : non togliere una recinzione che ostruisce una strada finché non capisci perché è stato costruito.

Puoi trovare l'autore / i del codice e dei commenti in questione e consultarli per ottenere la comprensione. Puoi guardare i messaggi di commit, i thread di email o i documenti se esistono. Poi sarai in grado di refactoring il frammento marcato, o scriverà le tue conoscenze nei commenti in modo che la prossima persona a mantenere questo codice possa prendere una decisione più consapevole.

Il tuo obiettivo è raggiungere una comprensione di ciò che fa il codice, e perché è stato contrassegnato con un avvertimento allora, e cosa accadrebbe se l'avviso fosse ignorato.

Prima non toccarei il codice contrassegnato come "non toccare".

Il codice con commenti come quelli che hai mostrato sarebbe in cima alla mia lista di cose da refactare se, e solo se ho qualche ragione per farlo . Il punto è che il codice fa schifo così male che ne senti l'odore attraverso i commenti. È inutile cercare di mettere in discussione qualsiasi nuova funzionalità in questo codice, tale codice deve morire non appena ha bisogno di cambiare.

Si noti inoltre che i commenti non sono utili in alcun modo: danno solo avvertimento e nessun motivo. Sì, sono i segnali di avvertimento attorno a un serbatoio di squali. Ma se vuoi fare qualcosa nelle vicinanze, ci sono pochi punti per provare a nuotare con gli squali. Imho, dovresti sbarazzarti di quegli squali prima.

Detto questo, hai assolutamente bisogno di buoni casi di test prima di poter osare di lavorare su tale codice. Una volta che hai quei casi di test, assicurati di capire ogni piccola cosa che stai cambiando, per assicurarti di non cambiare davvero il tuo comportamento. Rendi la tua massima priorità per mantenere tutte le peculiarità comportamentali del codice fino a quando non puoi dimostrare che sono senza alcun effetto. Ricorda, hai a che fare con gli squali: devi stare molto attento.

Quindi, nel caso del timeout: lasciatelo agire fino a quando non capisci con precisione che cosa il codice è in attesa, e quindi fissa il motivo per cui è stato introdotto il timeout prima di procedere alla sua rimozione.

Inoltre, assicurati che il tuo capo capisca qual è lo scopo per cui ti stai imbarcando e perché è necessario. E se dicono di no, non farlo. Hai assolutamente bisogno del loro supporto in questo.

Dico di andare avanti e cambialo. Che ci crediate o no, non tutti i programmatori sono un genio e un avvertimento come questo significa che potrebbe essere un punto di miglioramento. Se trovi che l'autore aveva ragione, potresti (senza fiato) DOCUMENTO o SPIEGARE il motivo dell'avvertimento.

Probabilmente non è una buona idea rifattorizzare il codice con tali avvertenze, se le cose funzionano correttamente così come sono.

Ma se devi refactor ...

Per prima cosa, scrivi alcuni test unitari e test di integrazione per verificare le condizioni alle quali gli avvertimenti ti avvisano. Prova a imitare le condizioni di produzione il più possibile . Ciò significa rispecchiare il database di produzione su un server di test, eseguire gli stessi altri servizi sulla macchina, ecc ... qualsiasi cosa tu possa fare. Quindi tenta il tuo refactoring (su un ramo isolato, ovviamente). Quindi esegui i test sul tuo nuovo codice per vedere se è possibile ottenere gli stessi risultati dell'originale. Se puoi, allora è probabilmente OK per implementare il tuo refactoring in produzione. Ma sii pronto a ripristinare i cambiamenti se le cose vanno male.

Sto espandendo i miei commenti in una risposta perché ritengo che alcuni aspetti del problema specifico vengano ignorati o utilizzati per trarre conclusioni sbagliate.

A questo punto, la domanda se il refactoring è prematuro (anche se probabilmente verrà data una risposta a una forma specifica di "sì").

Il problema centrale qui è che (come notato in alcune risposte) i commenti che citi indicano strongmente che il codice ha condizioni di competizione o altri problemi di concorrenza / sincronizzazione, come discusso qui . Questi sono problemi particolarmente difficili, per diversi motivi. Innanzitutto, come è stato rilevato, le modifiche apparentemente non correlate possono innescare il problema (altri bug possono avere questo effetto, ma quasi sempre gli errori di concorrenza fanno). In secondo luogo, sono molto difficili da diagnosticare: l'errore si manifesta spesso in un luogo che è distanti nel tempo o codice dalla causa, e qualsiasi cosa tu faccia per diagnosticare potrebbe farla andare via ( Heisenbugs ). In terzo luogo, i bug di concorrenza sono molto difficili da trovare nei test. In parte, ciò è dovuto all'esplosione combinatoria: è abbastanza grave per il codice sequenziale, ma aggiungendo i possibili intrecci di esecuzione simultanea si arriva al punto in cui il problema sequenziale diventa insignificante al confronto. Inoltre, anche un buon test può solo occasionalmente attivare il problema - Nancy Leveson ha calcolato che uno dei bug letali nel Therac 25 si è verificato in 1 di circa 350 sessioni, ma se non sai qual è il bug, o anche che ce ne sia uno, non sai quante ripetizioni facciano un test efficace. Inoltre, solo su questa scala è possibile eseguire solo test automatici, ed è possibile che il test driver imponga vincoli temporali minimi tali da non attivare mai il bug (Heisenbugs di nuovo).

Ci sono alcuni strumenti per il test della concorrenza in alcuni ambienti, come Helgrind per il codice usando i pthreads POSIX , ma non conosciamo le specifiche qui. I test dovrebbero essere integrati con analisi statiche (o viceversa?), Se esistono strumenti adeguati per il proprio ambiente.

Per aumentare la difficoltà, i compilatori (e persino i processori, in fase di esecuzione) sono spesso liberi di riorganizzare il codice in modi che a volte rendono il ragionamento sulla sicurezza del thread molto contro-intuitivo (forse il caso più noto è il Blocca con doppio controllo , anche se alcuni ambienti (Java, C ++ ...) sono stati modificati per migliorarlo. )

Questo codice potrebbe presentare un semplice problema che causa tutti i sintomi, ma è più probabile che tu abbia un problema sistemico che potrebbe portare i tuoi piani ad aggiungere nuove funzionalità. Spero di averti convinto che potresti avere un problema serio nelle tue mani, forse anche una minaccia esistenziale al tuo prodotto, e la prima cosa da fare è scoprire cosa sta succedendo. Se questo rivela problemi di concorrenza, ti consiglio vivamente di risolverli prima, prima ancora di porre la domanda se devi eseguire un refactoring più generale e prima di provare ad aggiungere altre funzionalità.

I am dealing with a pretty big codebase and I was given a few months to refactor existing code. The refactor process is needed because soon we will need to add many new features to our product [...]

During refactoring, from time to time I encounter the class, method or lines of code which have comments like ["don't touch this!"]

Sì, dovresti refactoring quelle parti specialmente . Questi avvertimenti sono stati collocati lì dagli autori precedenti per indicare "non interferire pigramente con queste cose, è molto intricato e ci sono molte interazioni inaspettate". Poiché la tua azienda prevede di sviluppare ulteriormente il software e aggiungere molte funzionalità, ti ha specificamente incaricato di ripulire questa roba. Quindi non sei ozioso a manometterlo, sei deliberatamente incaricato di ripulirlo.

Scopri cosa stanno facendo quei complicati moduli e suddividili in problemi più piccoli (cosa avrebbero dovuto fare gli autori originali). Per ottenere un codice gestibile da un pasticcio, le parti buone devono essere sottoposte a refactoring e le parti danneggiate devono essere riscritte .

Questa domanda è un'altra variazione sul dibattito su quando / come rifattorizzare e / o ripulire il codice con un trattino di "come ereditare il codice". Ognuno di noi ha esperienze diverse e lavora in diverse organizzazioni con diversi team e culture, quindi non c'è una risposta giusta o sbagliata, eccetto "fai quello che pensi di dover fare, e fallo in un modo che non ti faccia licenziare" .

Non penso che ci siano molte organizzazioni che farebbero bene ad avere un processo di business dalla sua parte perché l'applicazione di supporto aveva bisogno di pulizia del codice o refactoring.

In questo caso specifico, i commenti sul codice stanno alzando il flag che non dovrebbe essere modificato cambiando queste sezioni di codice. Quindi, se procedi, e il business cade dalla sua parte, non solo non hai nulla da mostrare per supportare le tue azioni, esiste in realtà un artefatto che funziona contro la tua decisione.

Quindi, come sempre, dovresti procedere con attenzione e apportare modifiche solo dopo aver compreso ogni aspetto di ciò che stai per cambiare e trovare i modi per testarlo, prestando molta attenzione a capacità e prestazioni e tempismo a causa dei commenti nel codice.

Ma anche in questo caso, il management deve comprendere il rischio insito in ciò che si sta facendo e concordare sul fatto che ciò che si sta facendo ha un valore aziendale che supera il rischio e che si è fatto ciò che si può fare per mitigare tale rischio.

Ora, torniamo tutti al nostro TODO e le cose che sappiamo possono essere migliorate nelle nostre creazioni di codice se solo ci fosse più tempo.

Sì, assolutamente. Queste sono indicazioni chiare che la persona che ha scritto questo codice non era soddisfatta del codice e probabilmente l'ha infastidita fino a quando non ha funzionato. È possibile che non abbiano capito quali fossero i veri problemi o, peggio, li hanno capiti e sono stati troppo pigri per risolverli.

Tuttavia, è un avvertimento che sarà necessario un grande sforzo per risolverli e che tali correzioni avranno dei rischi associati.

Idealmente, sarai in grado di capire qual è il problema e correggerlo correttamente. Ad esempio:

Time out set to give Module A some time to do stuff. If its not timed like this, it will break.

Questo suggerisce strongmente che il modulo A non indica correttamente quando è pronto per essere utilizzato o quando ha terminato l'elaborazione. Forse la persona che ha scritto questo non ha voluto preoccuparsi di fissare il modulo A o non potrebbe per qualche motivo. Questo sembra un disastro in attesa di accadere perché suggerisce una dipendenza temporale gestita dalla fortuna piuttosto che da un corretto sequenziamento. Se l'ho visto, vorrei davvero risolverlo.

Do not change this. Trust me, you will break things.

Questo non ti dice molto. Dipenderebbe da cosa stava facendo il codice. Potrebbe significare che ha quelle ovvie ottimizzazioni che, per una ragione o per l'altra, in realtà infrangeranno il codice. Ad esempio, potrebbe accadere che un loop lasci una variabile su un valore particolare da cui dipende un altro pezzo di codice. Oppure una variabile potrebbe essere testata in un altro thread e cambiare l'ordine degli aggiornamenti delle variabili potrebbe interrompere quell'altro codice.

I know using setTimeout is not a good practice, but in this case I had to use it.

Questo sembra facile. Dovresti essere in grado di vedere cosa sta facendo setTimeout e forse trovare un modo migliore per farlo.

Detto questo, se questi tipi di correzioni non rientrano nell'ambito del refactoring, queste sono indicazioni che provare a eseguire il refactoring all'interno di questo codice potrebbe aumentare significativamente lo scopo del tuo sforzo.

Come minimo, guarda attentamente il codice interessato e vedi se puoi almeno migliorare il commento al punto che spiega più chiaramente qual è il problema. Questo potrebbe salvare la prossima persona da affrontare lo stesso mistero che affronti.

L'autore dei commenti molto probabilmente non ha compreso completamente il codice . In realtà sapevano cosa stavano facendo, avrebbero scritto commenti davvero utili (o non avrebbero introdotto le condizioni di gara in primo luogo). Un commento come " Fidati di me, mi spezzerai le cose. " indica che l'autore prova a cambiare qualcosa che ha causato errori imprevisti che non hanno compreso appieno.

Il codice è probabilmente sviluppato attraverso l'indovinare e il trial-and-error senza una piena comprensione di ciò che accade realmente.

Questo significa:

1. È rischioso modificare il codice. Ci vorrà del tempo per capire, ovviamente, e probabilmente non segue i buoni principi di progettazione e potrebbe avere effetti oscuri e dipendenze. Probabilmente non è sufficientemente testato, e se nessuno comprende appieno il codice, sarà difficile scrivere test per assicurarsi che non vengano introdotti errori o cambiamenti nel comportamento. Le condizioni di gara (come i commenti alludono a) sono particolarmente onerose: questo è un posto dove i test unitari non ti salveranno.

2. È rischioso non modificare il codice. Il codice ha un'alta probabilità di contenere bug oscuri e condizioni di gara. Se viene rilevato un errore critico nel codice o se una modifica del requisito di business ad alta priorità ti obbliga a modificare questo codice con breve preavviso, ti troverai in un problema profondo . Ora hai tutti i problemi delineati in 1, ma con la pressione del tempo! Inoltre, tali "aree oscure" nel codice hanno la tendenza a diffondere e infettare altre parti del codice che tocca.

Un'ulteriore complicazione: I test unitari non ti salveranno . Di solito l'approccio consigliato a tale codice legacy di correzione consiste nell'aggiungere prima i test di unità o integrazione e quindi isolare e refactoring. Ma le condizioni di gara non possono essere catturate dai test automatici. L'unica soluzione è sedersi e pensare attraverso il codice fino a quando non lo si capisce, e quindi riscrivere per evitare le condizioni di gara.

Questo significa che il compito è molto più impegnativo del semplice refactoring di routine. Dovrai programmarlo come un vero compito di sviluppo.

Potresti essere in grado di incapsulare il codice interessato come parte del normale refactoring, quindi almeno il codice pericoloso è isolato.

La domanda che vorrei porre è perché qualcuno ha scritto, NON EDITARE, in primo luogo.

Ho lavorato su un sacco di codice e in parte è brutto e ho impiegato molto tempo e sforzi per lavorare, entro i limiti stabiliti al momento.

Scommetto che in questo caso è successo e la persona che ha scritto il commento ha scoperto che qualcuno l'ha cambiata e quindi ha dovuto ripetere l'intero esercizio e rimetterlo a posto così, per far funzionare la cosa. Dopo questo, per frustrazione, hanno scritto ... NON EDITARE.

In altre parole, non voglio doverlo risolvere di nuovo perché ho cose migliori da fare con la mia vita.

Dicendo NON EDITARE, è un modo per dire che sappiamo tutto ciò che sapremo in questo momento e quindi in futuro non impareremo mai nulla di nuovo.

Ci dovrebbe essere almeno un commento sui motivi della mancata modifica. Come dire "Do not Touch" o "Do Not Enter". Perché non toccare la recinzione? Perché non entrare? Non sarebbe meglio dire "Electric Fence, Do Not Touch!" o "Land Mines! Do Not Enter!". Quindi è ovvio perché, eppure puoi ancora entrare, ma almeno conoscere le conseguenze prima di farlo.

Scommetto anche che il sistema non ha test intorno a questo codice magico e quindi nessuno può confermare che il codice funzionerà correttamente dopo ogni modifica. Posizionare i test di caratterizzazione attorno al codice problema è sempre un primo passo. Consulta "Working with Legacy Code" di Michael Feathers per suggerimenti su come rompere le dipendenze e ottenere il codice sotto test, prima di affrontare la modifica del codice.

Penso che alla fine sia stato avvistato porre restrizioni sul refactoring e lasciare che il prodotto si evolva in modo naturale e organico.