Extract Method with continue

Question

Stiamo rifattorizzando un metodo lungo; contiene un lungo ciclo for con molte dichiarazioni continue. Vorrei semplicemente usare il refactoring del metodo di estrazione del metodo , ma quello automatizzato di Eclipse non sa come gestire il ramo condizionale. Nemmeno io.

La nostra strategia attuale è quella di introdurre un keepGoing bandiera (una variabile di istanza dal momento che stiamo andando a voler metodo estratto), impostarla su false nella parte superiore del ciclo, e sostituire ogni continuare con l'impostazione della bandierina true, quindi avvolge tutte le seguenti cose (a diversi livelli di nidificazione) all'interno di una clausola if (keepGoing). Quindi eseguire le varie estrazioni, quindi sostituire gli assegnamenti keepGoing con i ritorni anticipati dai metodi estratti, quindi eliminare la bandiera.

C'è un modo migliore?

Aggiornamento: In risposta ai commenti - non posso condividere il codice, ma ecco un estratto anonimo:

private static void foo(C1 a, C2 b, C3 c, List<C2> list, boolean flag1) throws Exception { 
    for (int i = 0; i < 1; i++) { 
     C4 d = null; 
     Integer e = null; 
     boolean flag2 = false; 
     boolean flag3 = findFlag3(a, c); 
     blahblahblah(); 
     if (e == null) { 
      if (flag1) { 
       if (test1(c)) { 
        if (test2(a, c)) { 
         Integer f = getF1(b, c); 
         if (f != null) 
          e = getE1(a, f); 
         if (e == null) { 
          if (d == null) { 
           list.add(b); 
           continue; 
          } 
          e = findE(d); 
         } 
        } else { 
         Integer f = getF2(b, c); 
         if (f != null) 
          e = getE2(a, f); 
         if (e == null) { 
          if (d == null) { 
           list.add(b); 
           continue; 
          } 
          e = findE(d); 
         } 
         flag2 = true; 
        } 
       } else { 
        if (test3(a, c)) { 
         Integer f = getF2(b, c); 
         if (f != null) 
          e = getE2(a, f); 
         if (e == null) { 
          if (d == null) { 
           list.add(b); 
           continue; 
          } 
          e = findE(d); 
         } 
         flag2 = true; 
        } else { 
         if (d == null) { 
          list.add(b); 
          continue; 
         } 
         e = findE(d); 
         flag2 = true; 
        } 
       } 
      } 
      if (!flag1) { 
       if (d == null) { 
        list.add(b); 
        continue; 
       } 
       e = findE(d); 
      } 
     } 
     if (e == null) { 
      list.add(b); 
      continue; 
     } 
     List<C2> list2 = blahblahblah(b, list, flag1); 
     if (list2.size() != 0 && flag1) { 
      blahblahblah(); 
      if (!otherTest()) { 
       if (yetAnotherTest()) { 
        list.add(b); 
        continue; 
       } 
       blahblahblah(); 
      } 
     } 
    } 
} 

Answer 1

Questo è uno di quelli divertenti in cui nessun modello singolo ti porterà lì.

Vorrei lavorarci iterativamente.

Per prima cosa, proverei a vedere se non potrei usare un primo aggiornamento per rimuovere uno di questi livelli di if. È un codice molto più chiaro per controllare una condizione e tornare presto (o nel tuo caso continuare) piuttosto che avere if if profondamente annidati.

Avanti Penso che prenderei alcuni dei pezzi interni e vedere se non potevano essere estratti in un metodo separato. Sembra che i primi due grandi blocchi (all'interno di "if (test2 (a, c)) {" e la sua altra istruzione) siano molto simili. C'è una logica taglia e incolla che dovrebbe essere la stessa.

Infine, dopo aver risolto il problema, è possibile iniziare a esaminare il problema reale: sono necessarie più classi. Questa intera affermazione è probabilmente un metodo polimorfico a tre linee in 3-5 classi di fratello.

È molto vicino a buttare via e riscrivere il codice, una volta identificate le classi attuali, questo intero metodo svanirà e sarà sostituito con qualcosa di così semplice da far male. Solo il fatto che sia un metodo di utilità statica dovrebbe dirti qualcosa - non vuoi uno di quelli in questo tipo di codice.

Modifica (dopo aver cercato un po 'di più): C'è così tanto qui che sarebbe davvero divertente da passare. Ricorda che quando hai finito non vuoi duplicare il codice - e sono abbastanza sicuro che questa intera cosa possa essere scritta senza un singolo se - penso che tutti i tuoi if siano casi che potrebbero/dovrebbero essere facilmente gestiti dal polimorfismo.

Oh, e come risposta alla tua domanda di eclissi non volendo farlo - NON PROVARE anche il refactoring automatico con questo, basta farlo a mano. Il materiale all'interno di quel primo if() deve essere estratto in un metodo perché è praticamente identico alla clausola nel suo else()!

Quando faccio qualcosa del genere, di solito creo un nuovo metodo, sposta il codice dal se nel nuovo metodo (lasciando solo una chiamata al nuovo metodo all'interno del if), quindi eseguo un test e assicurati di non ha rotto nulla.

quindi andare riga per riga e verificare che non ci siano differenze tra il se e il suo altro codice. Se c'è, compensalo passando la differenza come una nuova variabile al metodo. Dopo che sei sicuro che tutto sia identico, sostituisci la clausola else con una chiamata. Prova di nuovo È probabile che a questo punto alcune ottimizzazioni aggiuntive diventeranno ovvie, molto probabilmente perderai l'intero se combinando la sua logica con la variabile che hai passato per differenziare le due chiamate.

Continua a fare cose del genere e iterando. Il trucco con il refactoring consiste nell'usare Passi molto piccoli e testare ogni passaggio per garantire che nulla sia cambiato.

Answer 2

continue è fondamentalmente un analogo di un rientro anticipato, giusto?

for (...) { 
    doSomething(...); 
} 

private void doSomething(...) { 
    ... 
    if (...) 
     return; // was "continue;" 
    ... 
    if (!doSomethingElse(...)) 
     return; 
    ... 
} 

private boolean doSomethingElse(...) { 
    ... 
    if (...) 
     return false; // was a continue from a nested operation 
    ... 
    return true; 
} 

Ora devo ammettere che non ho seguito la tua strategia attuale, quindi avrei potuto semplicemente ripetere ciò che hai detto. Se è così, allora la mia risposta è che non riesco a pensare a un modo migliore.

Answer 3

Se mi trovassi di fronte alla tua situazione, considererei l'utilizzo di altre tecniche di refactoring come "sostituire il condizionale con il polimorfismo". Detto questo si dovrebbe sempre fare una cosa alla volta, quindi se si vuole prima di estrarre il metodo si hanno due opzioni:

1. aggiungere il flag "keepGoing"
2. un'eccezione dal metodo

Di queste due opzioni, penso che il flag keepGoing sia migliore. Non smetterei di refactoring dopo aver estratto il metodo. Sono sicuro che una volta che avrai un metodo più piccolo troverai un modo per rimuovere questo flag e avere una logica più pulita.

Answer 4

Riassumo le risposte qui, accettando la risposta di Bill K come la più completa. Ma tutti avevano qualcosa di buono da offrire, e potrei usare uno qualsiasi di questi approcci la prossima volta che mi trovo di fronte a questo tipo di situazione.

---

mmyers: Tagliare il corpo del ciclo, incollarlo in un nuovo metodo e sostituire tutti i continue s con return s. Questo ha funzionato molto bene, anche se avrebbe avuto problemi se ci fossero altre istruzioni sul flusso di controllo, come interruzione e ritorno, all'interno del ciclo.

---

Bill K: prendere in giro a parte iterativo; cercare la duplicazione ed eliminarla. Approfitta delle classi polimorfiche per sostituire il comportamento condizionale. Usa passaggi molto piccoli. Sì; questo è tutto un buon consiglio, con una più ampia applicabilità rispetto a questo caso specifico.

---

Aaron: o utilizzare il flag keepGoing per sostituire il continue o lancerà un'eccezione. Non l'ho provato, ma penso che l'opzione Eccezione sia un'alternativa molto bella e che non avevo considerato.