Best practice su if / return

L'esempio 2 è noto come blocco di guardia. È più adatto a restituire / generare eccezioni in anticipo se qualcosa è andato storto (parametro errato o stato non valido). Nel normale flusso logico è meglio usare l'Esempio 1

Il mio stile personale è utilizzare il singolo if per i blocchi di guardia e if / else nel codice di elaborazione del metodo effettivo.

In questo caso, stai utilizzando myString == null come condizione di guardia, quindi tenderei a utilizzare il singolo pattern if .

Considera il codice un po 'più complicato:

Esempio 1:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }
    else{
        //processing block
        myString = escapedString(myString);

        if (myString == "foo"){
            //some processing here
            return false;
        }
        else{
            myString = "Name " + myString;
            //other stuff
            return true;
        }
    }
}

Esempio 2:

public bool MyFunction(myString: string){

    //guard block
    if (myString == null){
        return false;
    }

    //processing block
    myString = escapedString(myString);

    if (myString == "foo"){
        //some processing here
        return false;
    }
    else{
        myString = "Name " + myString;
        //other stuff
        return true;
    }
}

Nell'Esempio 1, sia la guardia che il resto del metodo sono nel formato if / else . Confrontalo con l'Esempio 2, in cui il blocco di guardia si trova nella singola forma if , mentre il resto del metodo usa il formato if / else . Personalmente, trovo che l'esempio 2 sia più facile da capire, mentre l'esempio 1 sembra disordinato e troppo indentato.

Si noti che questo è un esempio forzato e che è possibile utilizzare le istruzioni else if per ripulirlo, ma sto mirando a mostrare la differenza tra i blocchi di guardia e il codice di elaborazione della funzione effettiva.

Un compilatore decente dovrebbe comunque generare lo stesso risultato per entrambi. L'unica ragione per utilizzare l'una o l'altra è la preferenza personale o per conformarsi allo stile del codice esistente.

personalmente, preferisco il secondo metodo. Mi sento come se fosse più corto, ha meno rientranze e è più facile da leggere.

La mia pratica personale è la seguente:

• Non mi piacciono le funzioni con diversi punti di uscita, ho trovato difficile mantenere e seguire, le modifiche al codice a volte interrompono la logica interna perché è intrinsecamente un po 'sciatta. Quando si tratta di un calcolo complesso, creo un valore di ritorno all'inizio e lo restituisco alla fine. Questo mi obbliga a seguire attentamente ogni percorso if-else, switch, etc, impostare il valore correttamente nelle posizioni corrette. Passo anche un po 'di tempo a decidere se impostare un valore di ritorno predefinito o lasciarlo non inizializzato all'inizio. Questo metodo aiuta anche quando la logica o il tipo di valore restituito o il significato cambia.

Ad esempio:

public bool myFunction()
{
   // First parameter loading
   String myString = getString();

   // Location of "quick exits", see the second example
   // ...

   // declaration of external resources that MUST be released whatever happens
   // ...

   // the return variable (should think about giving it a default value or not) 
   // if you have no default value, declare it final! You will get compiler 
   // error when you try to set it multiple times or leave uninitialized!
   bool didSomething = false;

   try {
     if (myString != null)
     {
       myString = "Name " + myString;
       // Do something more here...

       didSomething = true;
     } else {
       // get other parameters and data
       if ( other conditions apply ) {
         // do something else
         didSomething = true;
       }
     }

     // Edit: previously forgot the most important advantage of this version
     // *** HOUSEKEEPING!!! ***

   } finally {

     // this is the common place to release all resources, reset all state variables

     // Yes, if you use try-finally, you will get here from any internal returns too.
     // As I said, it is only my taste that I like to have one, straightforward path 
     // leading here, and this works even if you don't use the try-finally version.

   }

   return didSomething;
}

• L'unica eccezione: "uscita rapida" all'inizio (o in rari casi, all'interno del processo). Se la logica di calcolo reale non è in grado di gestire una determinata combinazione di parametri di input e stati interni o ha una soluzione semplice senza eseguire l'algoritmo, non è utile avere tutto il codice incapsulato in blocchi (a volte profondi). Questo è uno "stato eccezionale", non parte della logica principale, quindi devo uscire dal calcolo non appena ho rilevato. In questo caso non c'è altro ramo, in condizioni normali l'esecuzione continua semplicemente. (Ovviamente, lo "stato eccezionale" è meglio espresso facendo un'eccezione, ma a volte è eccessivo).

Ad esempio:

public bool myFunction()
{
   String myString = getString();

   if (null == myString)
   {
     // there is nothing to do if myString is null
     return false;
   } 

   myString = "Name " + myString;
   // Do something more here...

   // not using return value variable now, because the operation is straightforward.
   // if the operation is complex, use the variable as the previous example.

   return true;
}

La regola "una uscita" aiuta anche quando il calcolo richiede risorse esterne che devi rilasciare, o indica che devi reimpostare prima di lasciare la funzione; a volte vengono aggiunti in seguito durante lo sviluppo. Con più uscite all'interno dell'algoritmo, è molto più difficile estendere tutti i rami correttamente. (E se si verificano eccezioni, il rilascio / ripristino dovrebbe essere inserito in un blocco finale, per evitare effetti collaterali in rari casi eccezionali ...).

Il tuo caso sembra rientrare nella categoria "uscita rapida prima del lavoro reale" e lo scriverei come la tua versione di esempio 2.

Preferisco utilizzare i blocchi di guardie dove posso, per due motivi:

1. Consentono un'uscita rapida in base ad alcune condizioni specifiche.
2. Elimina la necessità di istruzioni se necessario complesse e non necessarie più avanti nel codice.

In generale preferisco vedere i metodi in cui la funzionalità principale del metodo è chiara e minima. I blocchi di guardia aiutano a rendere visivamente possibile ciò.

Mi piace l'approccio "Fall Through":

public bool MyFunction()
{
   string myString = GetString();

   if (myString != null)
   {
     myString = "Name " + myString;
     return true;
    }
    return false;
}

L'azione ha una condizione specifica, qualsiasi altra cosa è solo il ritorno predefinito "fall through".

Se ho una condizione singola se non spenderei troppo tempo a rimuginare sullo stile. Ma se ho più condizioni di guardia preferire style2

Picuture questo. Supponiamo che i test siano complessi e non vuoi legarli in una singola condizione se redatta per evitare la complessità:

//Style1
if (this1 != Right)
{ 
    return;
}
else if(this2 != right2)
{
    return;
}
else if(this3 != right2)
{
    return;
}
else
{
    //everything is right
    //do something
    return;
}

vs

//Style 2
if (this1 != Right)
{ 
   return;
}
if(this2 != right2)
{
    return;
}
if(this3 != right2)
{
    return;
}


//everything is right
//do something
return;

Qui ci sono due vantaggi principali

1. Stai separando il codice in una singola funzione in due blocchi visivamente logici: un blocco superiore di convalide (guardia condizioni) e un blocco inferiore di codice eseguibile.

2. Se devi aggiungere / rimuovere una condizione, riduci le tue possibilità di incasinare l'intero ladder if-elseif-else.

Un altro piccolo vantaggio è che hai un paio di set di parentesi da curare.

Questo dovrebbe essere un aspetto del suono migliore.

If condition
  do something
else
  do somethingelse

si esprime meglio di allora

if condition
  do something
do somethingelse

per piccoli metodi non sarà molto diverso, ma per metodi composti più grandi può renderlo più difficile da capire in quanto non sarà correttamente separato

Trovo che l'esempio 1 sia irritante, perché l'istruzione di ritorno mancante alla fine di una funzione di ritorno valore attiva immediatamente "Aspetta, c'è qualcosa di sbagliato" -flag. Quindi in casi come questo, vorrei andare con l'esempio 2.

Tuttavia, di solito c'è più coinvolgimento, a seconda dello scopo della funzione, ad es. gestione degli errori, registrazione, ecc. Quindi sono con la risposta di Lorand Kedves su questo, e di solito ho un punto di uscita alla fine, anche a costo di una variabile di flag aggiuntiva. Rende la manutenzione e le estensioni successive più facili nella maggior parte dei casi.

Quando l'istruzione if restituisce sempre, non vi è alcun motivo per utilizzare un altro per il codice rimanente nella funzione. In questo modo si aggiungono linee extra e rientri extra. L'aggiunta di codice non necessario rende più difficile la lettura e tutti sanno che leggere il codice è difficile .

Nel tuo esempio, else è ovviamente non necessario, MA ...

Quando si sfoglia velocemente attraverso linee di codice, gli occhi di solito guardano le parentesi e le rientranze per avere un'idea del flusso del codice; prima che si stabiliscano effettivamente sul codice stesso. Pertanto, scrivendo il else e inserendo parentesi e rientranze attorno al secondo blocco di codice, in realtà è più veloce per il lettore vedere che questa è una situazione "A o B", in cui un blocco o l'altro verrà eseguito. Cioè, il lettore vede le parentesi e il rientro PRIMA che vedano il return dopo l'iniziale if .

Secondo me, questo è uno dei rari casi in cui l'aggiunta di qualcosa di ridondante al codice lo rende PIÙ leggibile, non inferiore.

Preferirei l'esempio 2 perché è immediatamente evidente che la funzione restituisce qualcosa . Ma anche più di questo, preferirei tornare da un posto, come questo:

public bool MyFunction()
{
    bool result = false;

    string myString = GetString();

    if (myString != nil) {
        myString = "Name " + myString;

        result = true;
    }

    return result;
}

Con questo stile posso:

1. Guarda subito che sto restituendo qualcosa.

2. Catch il risultato di ogni invocazione della funzione con un solo breakpoint.

Il mio stile personale tende ad andare

function doQuery(string) {
    if (!query(string)) {
        query("ABORT");
        return false;
    } // else
    if(!anotherquery(string) {
        query("ABORT");
        return false;
    } // else
    return true;
}

Utilizzare le istruzioni else commentate per indicare il flusso del programma e mantenerlo leggibile, ma evitando i massicci rientri che possono facilmente raggiungere lo schermo se sono coinvolti molti passaggi.

Vorrei scrivere

public bool SetUserId(int id)
{
   // Get user name from id
   string userName = GetNameById(id);

   if (userName != null)
   {
       // update local ID and userName
       _id = id;
       _userNameLabelText = "Name: " + userName;
   }

   return userName != null;
}

Preferisco questo stile perché è ovvio che restituirei userName != null .

La mia regola empirica è fare un if-return senza altro (principalmente per errori) o fare una catena completa se-else-if su tutte le possibilità.

In questo modo evito di cercare di essere troppo fantasioso con la mia ramificazione, poiché ogni volta che finisco per farlo codifico la logica dell'applicazione nei miei if, rendendoli più difficili da mantenere (Ad esempio, se un enum OK o ERR e io scriviamo tutti i miei ifs sfruttano il fatto che! OK < - > ERR diventa un rompicoglioni per aggiungere una terza opzione all'enum la prossima volta.)

Nel tuo caso, ad esempio, userei un semplice se, poiché "return if null" è sicuramente un pattern comune e non è probabile che dovrai preoccuparti di una terza possibilità (oltre a null / non null) in futuro.

Tuttavia, se il test è stato qualcosa che ha fatto un'ispezione più approfondita rispetto ai dati, avrei invece commesso un errore su un full if-else.

Penso che ci siano molte insidie nell'esempio 2, che in fondo alla strada potrebbero portare a codice non intenzionale. La prima messa a fuoco qui è basata sulla logica che circonda la variabile "myString". Pertanto, per essere espliciti, tutti i test delle condizioni dovrebbero avvenire in un blocco di codice che tiene conto della logica conosciuta e predefinita / sconosciuta .

Che cosa succede se il codice successivo è stato introdotto involontariamente all'esempio 2 che ha significativamente modificato l'output:

   if (myString == null)
   {
      return false;
   }

   //add some v1 update code here...
   myString = "And the winner is: ";
   //add some v2 update code here...
   //buried in v2 updates the following line was added
   myString = null;
   //add some v3 update code here...
   //Well technically this should not be hit because myString = null
   //but we already passed that logic
   myString = "Name " + myString;
   // Do something more here...
   return true;

Penso che con il blocco else che segue immediatamente il controllo di un null avrebbero reso i programmatori che hanno aggiunto i miglioramenti alle versioni future aggiungono tutta la logica insieme perché ora abbiamo una stringa di logica non voluta per la regola originale (restituisce se il valore è nullo).

Ho prestato la mia convinzione in questo senso ad alcune delle linee guida C # su Codeplex (link a questo qui: link ) che seguenti:

"Aggiungi un commento descrittivo se il blocco predefinito (altrimenti) dovrebbe essere vuoto, inoltre, se non si prevede che quel blocco venga raggiunto, lancia un InvalidOperationException per rilevare le modifiche future che potrebbero ricadere nei casi esistenti. Questo garantisce un codice migliore, poiché tutti i percorsi che il codice può viaggiare è stato pensato. "

Penso che sia una buona pratica di programmazione quando si usano blocchi logici come questo per avere sempre un blocco predefinito aggiunto (if-else, case: default) per tenere conto di tutti i percorsi di codice esplicitamente e non lasciare il codice aperto a conseguenze logiche non intenzionali.