Come eseguire un refactoring pulito di un codice If Else senza lasciare blocchi liberi? [duplicare]

12
if(condition1)
{
   Statement1A;
   Statement1B;
}
else if(condition2)
{
  Statement2;
}
else if(condition3)
{
  Statement3;
}
else 
{
   Statement1A;
   Statement1B;
}

   return;

Mi piacerebbe refactoring quel codice in modo che io non duplicare dichiarazioni. I sempre devono controllare la condizione1 prima di qualsiasi altra condizione. (Quindi non posso solo cambiare l'ordine delle condizioni). Inoltre, non voglio scrivere &&!condition1 dopo ogni altra condizione.

L'ho risolto in questo modo

if(condition1)
{
}
else if(condition2)
{
  Statement2;
  return;
}
else if(condition3)
{
  Statement3;
  return;
}

Statement1A;
Statement1B;
return;

Tuttavia non penso che una condizione vuota sia facilmente comprensibile da altri (anche da parte mia dopo un po 'di tempo).

Qual è un approccio migliore?

    
posta M.C. 10.12.2013 - 14:26
fonte

8 risposte

31
notCondition2And3 = !condition2 & !condition3; 
// in place of notCondition2And3 should be some meaningful name
// representing what it actually MEANS that neither condition2 nor condition3 were true

E ora:

if (condition1 || notCondition2And3)
{
   Statement1A;
   Statement1B;
   return;
}
if (condition2)
{
   Statement2;
   return;
}
if (condition3)
{
   Statement3;
   return;
}

Come ho scritto nel mio commento alla risposta di Kieveli , non vedo nulla di sbagliato in più return s in un metodo, se non ci sono considerazioni sulla gestione della memoria (come potrebbe essere il caso in C o C ++ in cui è necessario rilasciare tutte le risorse manualmente prima di partire).

O ancora un altro approccio. Ecco la matrice delle decisioni in modo da non rovinarlo:

F F F - 1
---------
F F T - 3
---------    
F T F - 2
F T T - 2
---------    
T F F - 1
T F T - 1
T T F - 1
T T T - 1

T s e F s rappresentano i valori di condition1 , condition2 e condition3 (rispettivamente). I numeri rappresentano i risultati.

Rende chiaro che è anche possibile scrivere il codice come:

if (!condition1 && condition2) // outcome 2 only possible for FTF or FTT, condition3 irrelevant
{
   Statement2;
   return;
}
if (!condition1 && !condition2 && condition3)  // outcome 3 only when FFT
{
   Statement3;
   return;
}
// and for the remaining 5 combinations...
Statement1A;
Statement1B;

Ora se abbiamo estratto !condition1 (che è presente in ifs ), avremmo:

if (!condition1)
{
    if (condition2) // outcome 2 only possible for FTF or FTT, condition3 irrelevant
    {
       Statement2;
       return;
    }
    if (condition3)  // outcome 3 only when FFT
    {
       Statement3;
       return;
    }
}
// and for the remaining 5 combinations...
Statement1A;
Statement1B;

Che è quasi esattamente quello che ha suggerito Kieveli, solo il suo disprezzo per l'iniziale return s ha causato la sua implementazione di errori (come ha notato lui stesso), perché non avrebbe fatto nulla se tutte e tre le condizioni fossero false.

Oppure potremmo ripristinarlo in questo modo (questo probabilmente non funzionerebbe in tutte le lingue - funziona in C #, per esempio, dato che C # consente il confronto dell'uguaglianza tra più variabili), ora siamo praticamente tornati al primo uno:

// note that "condition1" on the right side of || is actually redundant and can be removed, 
// because if the expression on the right side of || gets evaluated at all,
// it means that condition1 must have been false anyway:

if (condition1 || (condition1 == condition2 == condition3 == false)) // takes care of all 4 x T** plus FFF (the one special case). 
{
    Statement1A;
    Statement1B;
    return;
}
// and now it's nice and clean
if (condition2) 
{
    Statement2;
    return; // or "else" if you prefer
}
if (condition3)
{
    Statement3;
    return; // if necessary
}
    
risposta data 10.12.2013 - 15:46
fonte
10

Non sono un fan delle tue dichiarazioni vuote di if e return. Diventa complicato da seguire, ed è generalmente disapprovato di avere più dichiarazioni di ritorno nel corpo del codice. Dati i tuoi obiettivi, vorrei fare qualcosa del genere:

if ( ! condition1 )
{
   if ( condition2 )
      Statement2;
   else if ( condition3 )
      Statement3;
}
else
{
   Statement1A;
   Statement1B;
}

Nota: questa soluzione è sbagliata! Considera! Condition1 & & ! condition2 & & ! Condition3

    
risposta data 10.12.2013 - 14:38
fonte
6

Le dichiarazioni ripetute sono solo un problema in quanto stai replicando un aspetto del tuo codice - in questo caso, "fai Statement1A, quindi fai Statement1B".

Un re-factoring più pulito è spostare ognuno di questi blocchi in metodi separati, anche se l'altro metodo è solo a due righe.

void _statement1() {
  Statement1A;
  Statement1B;
}

Avere un metodo del genere ti consente di fare semplicemente riferimento in luoghi in cui ti avresti ripetuto.

if(condition1) 
{
  _statement1()
} 
else if(condition2) 
{
  Statement2;
}
else if(condition3)
{
  Statement3;
}
else 
{
  _statement1()
}

Ovviamente, questo solleva la domanda di follow-up di "quando dovrei usare un metodo invece di ripetere me stesso?", che ha delle considerazioni tutte sue. (Preferisco "quando possibile", ma anche violando in modo flagrante quella regola a seconda della lingua.)

    
risposta data 10.12.2013 - 15:22
fonte
4

Mi piace in questo modo:

        if (!condition1)
        {
            if (condition2)
            {
                Statement2();
                return;
            }
            else if (condition3)
            {
                Statement3();
                return;
            }
        }

        Statement1A();
        Statement1B();
        return;

Modifica :

Mi piace ancora di più in questo modo perché ho solo 1 return e 2 istruzioni IF:

        if (!condition1 && condition2)
            Statement2();
        else if (!condition1 && condition3)
            Statement3();
        else
        {
            Statement1A();
            Statement1B();
        }
        return;
    
risposta data 10.12.2013 - 18:47
fonte
2

Forse sono solo io, ma trovo che una condizione come if (!a && b) sia relativamente difficile da leggere e generalmente preferisco if (b && !a) , quando possibile (cioè, a meno che non dipenda dalla valutazione di cortocircuito in modo che b solo essere valutato se a era falso).

In base a ciò, probabilmente lo scrivere come:

if (condition2 && !condition1)
   statement2;
else if (condition3 && !condition1) 
   statement3;
else {
    statement1A;
    statement1B;
}

Come ho detto, anche se questo va bene se ti interessa solo quale statement s viene valutato, non funziona se ti interessa anche quale condition s viene valutata.

    
risposta data 10.12.2013 - 20:12
fonte
2

C'è un altro modo. Preferirei mantenere il flusso di controllo originale. Quindi, il lavoro che viene svolto due volte, può essere messo in una funzione, e quindi invochiamo la funzione da due posti diversi:

function X()
{  
  if(condition1)
  {
     HandleDefault();
     return;
  }

  if(condition2)
  {
     Statement2;
     return;
  }

  if(condition3)
  {
    Statement3;
    return;
  }

  HandleDefault();
  return;

}
/// 


function HandleDefault(   )
{
    Statement1A;
    Statement1B;
}

Spesso, se ci sono blocchi di istruzioni che vanno insieme, è possibile dare alla funzione un nome significativo, aumentando ancora di più la leggibilità del codice. Ma se non hai un nome significativo, puoi comunque creare una funzione privata con un nome meno descrittivo (come ho fatto nel mio esempio).

La ragione per cui ritengo che questa sia la soluzione migliore perché mantiene l'ordine dei condizionali, mentre realizza che hai una sola copia di Statement1A e Statement1B da mantenere, e lo fa senza introdurre alcun costrutto di annidamento o variabile temporanea . Questo riduce la quantità di cose di cui hai bisogno per tenere traccia mentalmente di quando leggi questo codice.

Ora, ci sono linguaggi (in particolare C) in cui i ritorni anticipati non possono sempre essere usati, ma in qualsiasi altro linguaggio tradizionale posso pensare di poterlo fare.

    
risposta data 10.12.2013 - 20:35
fonte
0

Due approcci che non sono ancora stati menzionati sono di riscrivere le affermazioni come espressioni (se sono suscettibili a tale) o (oserei dire) usando una parolina che inizia con g . Ci sono casi in cui considererei ciascuno di questi l'alternativa migliore.

Nel caso di espressioni, il codice sarebbe qualcosa come:

if (condition1 ||
     condition2 ? (action2(),1) :
     condition3 ? (action3(),1) :
     1)
  action1;

Piuttosto antipatico, e non generalmente qualcosa che userei, ma può essere ragionevole nei casi in cui condition2 e action2 sono entrambi incapsulati in un metodo che tenta un'operazione e indica se ha successo e similar2 condizione2 e azione3. In tal caso, potrebbe diventare:

if (cantTryAnything || (
     tryFirstThing() != FIRST_TRY_SUCCESS &&
     trySecondThing() != SECOND_TRY_SUCCESS
)
{
  default actions here
}

Esegui le azioni predefinite se non riesco a provare nulla o tutto ciò che ho provato, fallito.

Si noti che semplicemente copiare le azioni o fattenerle su un metodo diverso generalmente sarebbe preferibile allo stile di logica precedente o a g___ . ma se il codice in "azioni predefinite qui" avrebbe bisogno di manipolare le variabili locali e il suo comportamento esatto potrebbe cambiare, incapsulare il codice in un metodo esterno potrebbe essere difficile e duplicare il codice potrebbe comportare il rischio che le versioni duplicate possano essere modificate per non abbinare più perfettamente. Inoltre, la nozione secondo cui una certa affermazione del flusso di controllo è dannosa è incentrata sul fatto che i più problemi del flusso di controllo del mondo reale possono essere mappati correttamente in costrutti di linguaggio comuni e che quando un problema del mondo reale si adatta bene a un costrutto linguistico che si dovrebbe usare. Se il particolare flusso di programma richiesto da un'applicazione richiede un comportamento identico in più combinazioni di condizioni, direi che è meglio forzare esplicitamente l'esecuzione a un punto comune in tutte queste condizioni, piuttosto che fudge attorno al problema.

    
risposta data 10.12.2013 - 23:46
fonte
0

Mi vengono in mente due cose:

switch-case

sebbene non sempre applicabile, può rendere il codice più facile da leggere e conservare. In pratica, acquisisci le diverse condizioni in un enum e crei una funzione per analizzare il contesto per produrre il valore enum corretto, quindi cambialo ed esegui il codice appropriato.

otterrai due cose:

  • separa il codice logico dall'esecuzione. Questo isolamento faciliterà la correzione dei bug in ogni parte.
  • flusso di controllo di facile lettura. La struttura dell'interruttore è piuttosto intuitiva, specialmente se si fa un nome con attenzione ai valori enum.

Funzioni

Cattura ogni blocco di codice in una funzione e, all'inizio della funzione, metti un codice per decidere se devi eseguire o meno.

Qui non si ottiene la separazione sopra menzionata ma si interrompe la struttura del flusso del codice IF-ELSE in più bit maneggevoli. Inoltre, se si verifica un errore, sarà più veloce da correggere in quanto verrà isolato su un singolo blocco di codice.

Il più grande vantaggio è che un cambiamento in una di queste funzioni non influirà su altre parti del codice.

Le funzioni hanno un vantaggio rispetto al caso degli interruttori: possono essere aggiunte dinamicamente. Un'estensione naturale sarebbe qualcosa di simile al modello di comando in cui una prima parte del codice analizza le azioni di contesto e di coda in una lista, quindi una seconda parte esegue semplicemente queste azioni in modo sequenziale.

Indipendentemente dalla strategia scelta CONSERVARE SEMPLICEMENTE a volte una serie di if-else è sufficiente e dovrebbe rimanere tale. Come regola generale se ritorno nello stesso codice per risolvere quello che sembra lo stesso problema o se mi ritrovo spesso a grattarmi la testa per aggiungere ancora una nuova condizione allora forse è il momento di cercare alternative come quelle menzionate sopra.

    
risposta data 13.12.2013 - 20:50
fonte

Leggi altre domande sui tag