"quanto è cattivo" il codice non correlato nel blocco try-catch-finally?

Ho attraversato una situazione molto simile quando ho dovuto gestire un terribile codice Windows Form precedente scritto da sviluppatori che chiaramente non sapevano cosa stavano facendo.

Prima di tutto, non stai esagerando. Questo è un codice cattivo. Come hai detto tu, il blocco delle catture dovrebbe essere sull'abortire e sul prepararsi a fermarsi. Non è il momento di creare oggetti (specialmente pannelli). Non posso nemmeno iniziare a spiegare perché questo è male.

Detto questo ...

Il mio primo consiglio è: se non è rotto, non toccarlo!

Se il tuo lavoro è mantenere il codice devi fare del tuo meglio per non romperlo. So che è doloroso (ci sono stato) ma devi fare del tuo meglio per non rompere ciò che sta già funzionando.

Il mio secondo consiglio è: se devi aggiungere più funzionalità, prova a mantenere il più possibile la struttura del codice esistente in modo da non interrompere il codice.

Esempio: se c'è un'orribile affermazione di caso-interruttore che ritieni possa essere sostituita da un'eredità adeguata, devi stare attento e pensarci due volte prima di decidere di iniziare a spostare le cose.

Troverai sicuramente situazioni in cui un refactoring è l'approccio giusto, ma attenzione: il codice di refactoring ha maggiori probabilità di introdurre bug . Devi prendere quella decisione dal punto di vista dei proprietari delle applicazioni, non dal punto di vista dello sviluppatore. Quindi devi pensare se lo sforzo (denaro) necessario per risolvere il problema valga un refactoring o meno. Ho visto molte volte uno sviluppatore che impiega diversi giorni a sistemare qualcosa che non è realmente rotto solo perché pensa che "il codice sia brutto".

Il mio terzo consiglio è: ti brucerai se rompi il codice, non importa se è colpa tua o meno.

Se sei stato assunto per dare manutenzione, non importa se l'applicazione sta cadendo a pezzi perché qualcun altro ha preso decisioni sbagliate. Dal punto di vista dell'utente stava funzionando prima e ora lo hai rotto. Tu l'hai rotto!

Joel mette molto bene nel suo articolo spiegando diversi motivi per cui non si dovrebbe riscrivere il codice legacy.

link

Quindi dovresti sentirti molto male con quel tipo di codice (e non dovresti mai scrivere qualcosa del genere) ma mantenerlo è un mostro completamente diverso.

Sulla mia esperienza: ho dovuto mantenere il codice per circa 1 anno e alla fine sono stato in grado di riscriverlo da zero ma non tutto in una volta.

Quello che è successo è che il codice era così cattivo che era impossibile implementare nuove funzionalità. L'applicazione esistente presentava gravi problemi di prestazioni e usabilità. Alla fine mi è stato chiesto di fare un cambiamento che mi avrebbe richiesto 3-4 mesi (principalmente perché lavorare con quel codice mi ha portato via più tempo del solito). Ho pensato di poter riscrivere l'intero pezzo (compresa l'implementazione della nuova funzionalità desiderata) in circa 5-6 mesi. Ho portato questa proposta agli stakeholder e sono d'accordo a riscriverla (fortunatamente per me).

Dopo aver riscritto questo pezzo, hanno capito che potevo offrire cose molto migliori di quelle che avevano già. Quindi sono stato in grado di riscrivere l'intera applicazione.

Il mio approccio era di riscriverlo pezzo per pezzo. Per prima cosa ho sostituito l'intera interfaccia utente (Windows Form), quindi ho iniziato a sostituire il livello di comunicazione (chiamate al servizio Web) e infine ho sostituito l'intera implementazione del server (era un tipo di applicazione client / server di spessore).

Un paio di anni più tardi e questa applicazione è diventata un bellissimo strumento chiave utilizzato da tutta l'azienda. Sono sicuro al 100% che non sarebbe mai stato possibile se non avessi riscritto il tutto.

Anche se sono stato in grado di farlo, la parte importante è che le parti interessate lo hanno approvato e sono riuscito a convincerli che valeva la pena. Quindi, mentre devi mantenere l'applicazione esistente fai del tuo meglio per non rompere nulla e se sei in grado di convincere i proprietari del vantaggio di riscriverlo, prova a farlo come Jack the Ripper: a pezzi.

Se non è necessario modificare il codice, lasciarlo così com'è. Ma se devi modificare il codice perché devi correggere qualche bug o modificare alcune funzionalità, dovrai cambiarlo, se ti piace o meno. IMHO è spesso più sicuro e meno soggetto a errori, se lo si prima refactoring in parti più piccole, meglio comprensibili e quindi aggiungere la funzionalità. Quando si dispone di strumenti di refactoring automatico, questo può essere fatto in modo relativamente sicuro, con un rischio molto piccolo di introdurre nuovi bug. E ti consiglio di procurarti una copia del libro di Michael Feathers

link

che ti darà preziosi suggerimenti su come rendere il codice più testabile, aggiungere test unitari ed evitare di infrangere il codice quando aggiungi nuove funzionalità.

Se lo fai bene, il tuo metodo si spera possa diventare abbastanza semplice, il try-catch-end non conterrà più alcun codice non correlato nei posti sbagliati.

Supponiamo di avere sei metodi da chiamare, quattro dei quali dovrebbero essere chiamati solo se il primo si conclude senza errori. Prendi in considerazione due alternative:

try {
     method1();  // throws
     method2();
     method3();
     method4();
     method5();
 } catch(e) {
     // handle error
 }
 method6();

vs

 try {
      method1();  // throws
 }
 catch(e) {
      // handle error
 }
 if(! error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Nel secondo codice, si trattano eccezioni come i codici di ritorno. Concettualmente, questo è identico a:

rc = method1();
if( rc != error ) {
     method2();
     method3();
     method4();
     method5();
 }
 method6();

Se avvolgeremo tutti i metodi che possono generare un'eccezione in un blocco try / catch, qual è il punto di avere eccezioni in una funzionalità linguistica? Non c'è beneficio e c'è più digitazione.

Il motivo per cui abbiamo eccezioni nelle lingue in primo luogo è che nei cattivi vecchi giorni dei codici di ritorno, spesso abbiamo avuto situazioni sopra le quali avevamo diversi metodi che potevano restituire errori, e ogni errore significava interrompere completamente tutti i metodi. All'inizio della mia carriera, nei vecchi C giorni, ho visto un codice come questo dappertutto:

rc = method1();
if( rc != error ) {
     rc = method2();
     if( rc != error ) {
         rc = method3();
         if( rc != error ) {
             rc = method4();
             if(rc != error ) {
                 method5();
             }
         }
     }
 }
 method6();

Questo era uno schema incredibilmente comune, e quello che eccezioni risolveva molto bene consentendo di tornare al primo esempio sopra. Una regola di stile che dice che ogni metodo ha un proprio blocco di eccezioni lo getta completamente fuori dalla finestra. Perché nemmeno preoccuparsi, allora?

Dovresti sempre sforzarti di fare in modo che il blocco di prova si estenda all'insieme di codice che è concettualmente un'unità. Dovrebbe circondare il codice per cui se c'è qualche errore nell'unità di codice, allora non ha senso eseguire altri codici nell'unità di codice.

Tornando allo scopo originale delle eccezioni: ricorda che sono stati creati perché spesso il codice non può facilmente gestire gli errori nel momento in cui si verificano effettivamente. Il punto di eccezioni è quello di consentire di gestire gli errori in modo significativo più avanti nel codice, o più in alto nello stack. La gente dimentica quello e in molti casi li prende localmente quando starebbero meglio semplicemente documentando che il metodo in questione può lanciare questa eccezione. C'è troppo codice al mondo che assomiglia a questo:

void method0() : throws MyNewException 
{
    try {
        method1();  // throws MyOtherException
    }
    catch(e) {
        if(e == MyOtherException)
            throw MyNewException();
    }
    method2();
}

Qui stai solo aggiungendo dei livelli e i livelli aggiungono confusione. Fai questo:

void method0() : throws MyOtherException
{
    method1();
    method2();
}

In aggiunta a ciò, la mia sensazione è che se segui questa regola e ti trovi con più di un paio di tentativi / catture di blocchi nel metodo, dovresti chiedere se il metodo stesso è troppo complesso e deve essere scomposto in più metodi.

Il takeaway: un blocco try dovrebbe contenere il set di codice che dovrebbe essere annullato se si verifica un errore in qualsiasi punto del blocco. Non importa se questo insieme di codici è di una riga o di mille righe. (Anche se hai mille righe in un metodo, hai altri problemi.)