Modi eleganti per gestire if (if else) else

Estrailo in una funzione separata (metodo) e usa l'istruzione return :

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

O, forse meglio, separare i contenuti e il loro trattamento:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

UPD:

Perché non eccezioni, perché OpenFile non lancia eccezioni IO:
Penso che sia una domanda davvero generica, piuttosto che una domanda sul file IO. Nomi come FileExists , OpenFile possono confondere, ma se sostituirli con Foo , Bar , ecc. - sarebbe più chiaro che DefaultAction può essere chiamato tutte le volte che DoSomething , quindi può essere un caso non eccezionale. Péter Török ha scritto su questo alla fine della sua risposta

Perché esiste un operatore condizionale ternario nella 2a variante:
Se ci fosse il tag [C ++], avrei scritto if statement con dichiarazione di contents nella sua parte condition:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Ma per altre lingue (simili a C), if(contents) ...; else ...; è esattamente la stessa di un'espressione con operatore condizionale ternario, ma più lunga. Poiché la parte principale del codice era la funzione get_contents , ho appena usato la versione più breve (e omesso anche il tipo contents ). Ad ogni modo, è oltre questa domanda.

Se il linguaggio di programmazione stai usando (0) cortocircuiti binari confronti (cioè se non chiama SomeTest se FileExists restituisce false) e (1) assegnazione restituisce un valore (il risultato di OpenFile è assegnato a contents e poi quel valore è passato come argomento a SomeTest ), puoi usare qualcosa come il seguente, ma ti consigliamo comunque di commentare il codice notando che il singolo = è intenzionale .

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

A seconda di quanto convoluto sia il if, potrebbe essere meglio avere una variabile flag (che separa il test delle condizioni di successo / fallimento con il codice che gestisce l'errore DefaultAction in questo caso)

Più seriamente della ripetizione della chiamata a DefaultAction è lo stile stesso perché il codice è scritto non ortogonale (vedi questa risposta per buoni motivi per scrivere ortogonalmente).

Per mostrare perché il codice non ortogonale è sbagliato, considera l'esempio originale, quando un nuovo requisito che non dovremmo aprire il file se è memorizzato su un disco di rete è stato introdotto. Bene, allora potremmo semplicemente aggiornare il codice a il seguente:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Ma poi arriva anche il requisito che non dovremmo aprire file di grandi dimensioni oltre 2 Gb. Bene, ci limitiamo ad aggiornare di nuovo:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Dovrebbe essere molto chiaro che tale stile di codice sarà un enorme problema di manutenzione.

Tra le risposte qui che sono state scritte correttamente ortogonalmente secondo esempio Abyx e La risposta di Jan Hudec , quindi non lo ripeterò, solo sottolineare che aggiungere i due requisiti in quelle risposte sarebbe solo

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(o goto notexists; anziché return null; ), non influisce su nessun altro codice rispetto a quelle aggiunte . Per esempio. ortogonale.

Durante i test, la regola generale dovrebbe essere quella di eccezioni di test, non il caso normale .

Ovviamente:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Hai detto di essere aperto anche a soluzioni malvagie, quindi usare il conteggio del goto del male, no?

In effetti, a seconda del contesto, questa soluzione potrebbe essere meno malvagia di entrambi i malvagi che eseguono l'azione due volte o una variabile extra malvagia. L'ho avvolto in una funzione, perché non sarebbe sicuramente OK nel mezzo di una lunga funzione (non ultimo a causa del ritorno nel mezzo). Ma la funzione lunga non è OK, punto.

Quando hai delle eccezioni, saranno più facili da leggere, specialmente se puoi avere OpenFile e DoSomething semplicemente lanciare un'eccezione se le condizioni non sono soddisfatte, quindi non hai bisogno di controlli espliciti. D'altra parte in C ++, Java e C # lanciare un'eccezione è un'operazione lenta, quindi dal punto di performance, il goto è ancora preferibile.

Nota su "male": Domande frequenti su C ++ 6.15 definisce " male "come:

It means such and such is something you should avoid most of the time, but not something you should avoid all the time. For example, you will end up using these "evil" things whenever they are "the least evil of the evil alternatives."

E questo vale per goto in questo contesto. I costrutti di controllo del flusso strutturato sono migliori il più delle volte, ma quando entri nella situazione in cui accumulano troppi mali, come il compito in condizioni, annidando più di 3 livelli in profondità, duplicando codice o condizioni lunghe, goto potrebbe semplicemente finire per essere meno cattivo.

function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Una possibilità:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Ovviamente, questo rende il codice leggermente più complesso in un modo diverso. Quindi è in gran parte una domanda di stile.

Un approccio diverso utilizzerebbe le eccezioni, ad esempio:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Sembra più semplice, tuttavia è applicabile solo se

• sappiamo esattamente quale tipo di eccezioni aspettarci e DefaultAction() si adatta a ciascuna
• ci aspettiamo che l'elaborazione dei file abbia successo, e un file mancante o un SomeTest() non riuscito è chiaramente una condizione errata, quindi è adatto a lanciare un'eccezione su di esso.

Questo è ad un livello più alto di astrazione:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

E questo riempie i dettagli.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}

Functions should do one thing. They should do it well. They should do it only.
— Robert Martin in Clean Code

Alcune persone trovano questo approccio un po 'estremo, ma è anche molto pulito. Consentitemi di illustrare in Python:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Quando dice che le funzioni dovrebbero fare una cosa, significa una cosa. processFile() sceglie un'azione basata sul risultato di un test, e questo è tutto ciò che fa. fileMeetsTest() combina tutte le condizioni del test, e questo è tutto ciò che fa. contentsTest() trasferisce la prima riga a firstLineTest() , e questo è tutto ciò che fa.

Sembra un sacco di funzioni, ma si legge praticamente come in inglese diretto:

To process the file, check if it meets the test. If it does, then do something. Otherwise, take the default action. The file meets the test if it exists and passes the contents test. To test the contents, open the file and test the first line. The test for the first line ...

Certo, è un po 'prolisso, ma nota che se un manutentore non si preoccupa dei dettagli, può smettere di leggere dopo solo le 4 righe di codice in processFile() , e avrà ancora una buona conoscenza di alto livello di cosa fa la funzione.

Riguardo a ciò che viene chiamato, può facilmente svilupparsi nel modello di punta della freccia man mano che il codice cresce per gestire più requisiti (come mostrato dalla risposta fornita a link ) e quindi cade nella trappola di avere enormi sezioni di codici con istruzioni condizionali nidificate che assomigliano a una freccia.

Vedi Collegamenti come;

link

link

C'è molto altro su questo e altri anti pattern da trovare su Google.

Alcuni ottimi consigli che Jeff fornisce sul suo blog riguardo a questo sono:

1) Replace conditions with guard clauses.

2) Decompose conditional blocks into seperate functions.

3) Convert negative checks into positive checks

4) Always opportunistically return as soon as possible from the function.

Vedi alcuni dei commenti sul blog di Jeff riguardo ai suggerimenti di Steve McConnells anche sui primi ritorni;

"Use a return when it enhances readability: In certain routines, once you know the answer, you want to return it to the calling routine immediately. If the routine is defined in such a way that it doesn't require any further cleanup once it detects an error, not returning immediately means that you have to write more code."

...

"Minimize the number of returns in each routine: It's harder to understand a routine when, reading it at the bottom, you're unaware of the possibility that it returned somewhere abouve. For that reason, use returns judiciously--only when they improve readability. "

Mi sono sempre iscritto alla teoria 1 entrata / uscita per funzione, a causa di ciò che mi è stato insegnato 15 anni fa. Sento che questo rende il codice molto più facile da leggere e come si menziona più gestibile

Questo è conforme alle regole DRY, no-goto e no-multiple-return, è scalabile e leggibile secondo me:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

Lo estraperei in un metodo separato e poi:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

che consente anche

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

quindi probabilmente potresti rimuovere le chiamate DefaultAction e lasciare l'esecuzione di DefaultAction per il chiamante:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

Mi piace l'approccio di Jeanne Pindar .

Per questo caso particolare, la risposta è abbastanza semplice ...

C'è una condizione di competizione tra FileExists e OpenFile : cosa succede se il file viene rimosso?

L'unico modo corretto per affrontare questo caso particolare è saltare FileExists :

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Questo risolve in modo chiaro questo problema e rende il codice più pulito.

In generale: Cerca di ripensare il problema e ideare un'altra soluzione che eviti completamente il problema.

Un'altra possibilità se non ti piace vederne troppe è abbandonare del tutto l'uso di else e inserire una dichiarazione di ritorno extra. Else è una sorta di superfluo a meno che non sia necessaria una logica più complessa per determinare se ci sono più di due sole possibilità di azione.

Quindi il tuo esempio potrebbe diventare:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Personalmente non mi dispiace usare la clausola else in quanto afferma esplicitamente come dovrebbe funzionare la logica, e quindi migliora la leggibilità del tuo codice. Alcuni strumenti di abbellimento del codice preferiscono tuttavia semplificare una singola istruzione if per scoraggiare la logica di nidificazione.

Il caso illustrato nel codice di esempio può essere generalmente ridotto a una singola istruzione if . Su molti sistemi, la funzione di apertura file restituirà un valore non valido se il file non esiste già. A volte questo è il comportamento predefinito; altre volte, deve essere specificato tramite un argomento. Ciò significa che il test FileExists può essere eliminato, il che può anche aiutare con le condizioni di competizione risultanti dalla cancellazione di file tra il test di esistenza e l'apertura del file.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Questo non affronta direttamente il problema del mixaggio a livello di astrazione poiché elude completamente il problema di test multipli e non vincolabili, anche se eliminare il test di esistenza del file non è incompatibile con la separazione dei livelli di astrazione. Supponendo che gli handle di file non validi siano equivalenti a "false" e che gli handle di file si chiudano quando escono dall'ambito di applicazione:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}

Sono d'accordo con frozenkoi, comunque, per C # comunque, ho pensato che sarebbe stato utile seguire la sintassi dei metodi TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();

bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}

Il tuo codice è brutto perché stai facendo troppo in una singola funzione. Desideri elaborare il file o eseguire l'azione predefinita, quindi inizia dicendo che:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

I programmatori Perl e Ruby scrivono processFile(file) || defaultAction()

Ora vai su Process File:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;

Naturalmente puoi solo andare così lontano in scenari come questi, ma ecco una strada da percorrere:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Potresti volere filtri aggiuntivi. Quindi fai questo:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Anche se questo potrebbe avere un buon senso:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Qual è il migliore? Dipende dal problema del mondo reale che stai affrontando.
Ma la cosa da togliere è: puoi fare molto con la composizione e il polimorfismo.

Cosa c'è di sbagliato nell'ovvio

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

Mi sembra piuttosto normale? Per quel tipo di grande procedura in cui devono accadere molte piccole cose, il fallimento di una qualsiasi delle quali impedirebbe il secondo. Le eccezioni lo rendono un po 'più pulito se questa è un'opzione.

Mi rendo conto che questa è una vecchia domanda, ma ho notato uno schema che non è stato menzionato; principalmente, impostando una variabile per determinare in seguito il / i metodo / i che si desidera chiamare (al di fuori del if ... else ...).

Questo è un altro angolo da considerare per rendere il codice più semplice da utilizzare. Permette anche quando potresti voler aggiungere un altro metodo per essere chiamato o cambiare il metodo appropriato che deve essere chiamato in determinate situazioni.

Piuttosto che dover sostituire tutte le menzioni del metodo (e magari perdere alcuni scenari), sono tutte elencate alla fine del blocco if ... else ... e sono più semplici da leggere e modificare. Tendo ad usarlo quando, ad esempio, si possono chiamare diversi metodi, ma all'interno del nidificato se ... altrimenti ... un metodo può essere chiamato in più corrispondenze.

Se imposti una variabile che definisce lo stato, potresti avere molte opzioni profondamente annidate e aggiornare lo stato quando qualcosa deve essere (o non essere) eseguito.

Questo potrebbe essere usato come nell'esempio chiesto nella domanda in cui stiamo verificando se "DoSomething" si è verificato, e in caso contrario, eseguire l'azione predefinita. Oppure potresti avere lo stato per ogni metodo che potresti voler chiamare, impostare quando applicabile, quindi chiamare il metodo applicabile al di fuori del if ... else ...

Alla fine delle istruzioni nidificate if ... else ..., controlli lo stato e agisci di conseguenza. Ciò significa che è sufficiente una sola menzione di un metodo invece di tutte le posizioni che dovrebbe essere applicato.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}

Per ridurre IF annidato:

1 / ritorno anticipato;

2 / espressione composta (in cortocircuito)

Quindi, il tuo esempio potrebbe essere refactored in questo modo:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();

Ho visto molti esempi con "return" che uso anch'io ma a volte voglio evitare di creare nuove funzioni e usare invece un loop:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Se vuoi scrivere meno righe o odi loop infiniti come me, puoi cambiare il tipo di loop in "do ... while (0)" ed evitare l'ultima "interruzione".

Che ne dici di questa soluzione:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Ho assunto il presupposto che OpenFile restituisca un puntatore, ma questo potrebbe funzionare anche con il ritorno del tipo di valore specificando un valore predefinito non restituibile (codici di errore o qualcosa del genere).

Naturalmente non mi aspetto alcuna azione possibile tramite il metodo SomeTest sul puntatore NULL (ma non si sa mai), quindi questo potrebbe anche essere visto come un controllo extra per il puntatore NULL per la chiamata SomeTest (contenuti).

Chiaramente, la soluzione più elegante e concisa è usare una macro per il preprocessore.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Che ti permette di scrivere codice bello come questo:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Potrebbe essere difficile fare affidamento sulla formattazione automatica se si utilizza questa tecnica spesso, e alcuni IDE potrebbero urlare un po 'su ciò che erroneamente suppone sia malformato. E come dice il proverbio, tutto è un compromesso, ma suppongo che non sia un cattivo prezzo da pagare per evitare i mali del codice ripetuto.

Dato che hai chiesto per curiosità e la tua domanda non è contrassegnata da una lingua specifica (anche se è chiaro che hai in mente dei linguaggi imperativi), potrebbe valere la pena aggiungere che le lingue che supportano la valutazione pigra consentono un approccio completamente diverso. In quei linguaggi, le espressioni vengono valutate solo quando necessario, quindi è possibile definire "variabili" e usarle solo quando ha senso farlo. Ad esempio, in un linguaggio fittizio con strutture lazy let / in ti dimentichi del controllo di flusso e scrivi:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()