Quale stile è meglio controllare e registrare l'errore?

Penso che in questo caso specifico entrambe le soluzioni siano ugualmente ben leggibili.

Tuttavia, se ci sono altri controlli aggiunti successivamente per verificare che la funzione possa continuare a elaborare, allora solo la prima soluzione sarà facilmente estesa senza rendere difficile il codice da seguire:

bool AClass::myMethod()
{
     if (!SomeCondition())
     {
         Warning("AClass::myMethod: your description");
         return false;
     }

     if (!SomeCondition2())
     {
         Warning("AClass::myMethod: another description");
         return false;
     }

    doSomthing();
    return true;
}

è più chiaro di

bool AClass::myMethod()
{
    if (SomeCondition())
    {
        if(SomeOtherCondition())
        {
            doSomething();
        }
        else
        {
            Warning("AClass::myMethod： another description");
            return false;
        }
    }
    else
    {
        Warning("AClass::myMethod： your description");
        return false;
    }
    return true;
}

Quindi utilizzerei personalmente la prima versione non appena è necessario più di un controllo.

Vorrei fare un passo indietro e dire perché limitarti in questi due casi? Una tecnica più pulita e più difensiva è suggerita in la difesa pragmatica . Seguendo i suggerimenti dell'articolo si finisce in una struttura del genere:

bool AClass::myMethod() throw()
{      
    bool myMethodReturn = false;

    try
    {
       // your preconditions here
       // on failure throw an exception

       // main code...

       // your postconditions here
       // on failure throw an exception

       // everything went fine
       myMethodReturn = true;
    }
    // catch all the possible exceptions
    catch (const std::domain_error& err)
    {
        // log the error
    }
    catch (...) 
    {
        // log the critical error
    }

    return myMethodReturn;
}

Secondo me, la prima alternativa è chiaramente la migliore. Lo scenario normale è quello di restituire true e passare a doSomething, mentre la registrazione degli errori può essere vista come una deviazione dello scenario normale. A mio parere, questi dovrebbero assomigliare ad un'eccezione, in quanto viene lanciata / registrata come prima cosa che accade nella funzione. Quest'ultima alternativa mi sembra un po 'confusa.

Con un metodo così breve, non importa molto.

Se il metodo sarebbe lungo, allora, sarebbe meglio refactor in metodi più brevi, fino al momento in cui, ancora una volta, non importa molto quale delle tue opzioni scegliere.

In realtà, questo potrebbe essere un buon criterio per fermare il refactoring: quando diverse opzioni iniziano a sembrare "difficile da scegliere", "tutte abbastanza buone", è abbastanza sicuro fermarsi qui.

_{Ho imparato a conoscere ciò durante lo studio di CPJ di Doug Lea . Durante la lettura, ho notato che nei frammenti di codice l'autore utilizza uno stile che ho odio (else-after-return) positivamente. Avevo intenzione di indicare questo come un inconveniente nella recensione del libro, ma alla fine ho scoperto di sentirmi a disagio nel puntare il dito su quello. Dato che di solito preferisco essere un vocal riguardo ai problemi di stile, ho pensato a cosa non andava, sono tornato indietro e ho ricontrollato tutti i frammenti "cattivi". In quella retrospettiva, ho notato che tutti i metodi erano piuttosto piccoli (meno di 10 righe o qualcosa del genere), e una particolare preferenza di stile non aveva molta importanza.}

Se lo scopo di questo metodo è quello di attuare una modifica di stato, allora dovrebbe cambiare lo stato o generare un'eccezione. Registra il problema quando viene rilevata l'eccezione. Inoltre, per quanto possibile, evita di scrivere classi che possono trovarsi in uno stato in cui alcune chiamate al metodo non sono valide.

void AClass::myMethod(**args**) {
  if (!SomeCondition(**args**)) { throw new InvalidArguments(); }
  performAction(...);
}

try {
  AClass::myMethod();
}
catch (...) {
  Log(...);
}

Which way is better?

Il meglio è ciò che è più comprensibile.

Secondo me, la prima versione, in cui la condizione non è negata, è migliore. Negare una condizione rende la funzione più complessa, quindi è un po 'più difficile da capire.

Offro umilmente questo come alternativa a 1 e 2:

3.

bool AClass::myMethod()
{
     bool valid = true;
     if (!SomeCondition())
     {
         Warning("AClass::myMethod: your description");
         valid = false;
     }

     if(valid) 
     {
         doSomthing();
     }
     return valid;
}

Questo è forse il più prolisso dei tre, ma penso sia sia chiaro che efficiente, dal momento che in teoria non si dovrebbe tornare nel mezzo di un metodo poiché non è così veloce. Inoltre, è possibile eseguire codice di errore sia specifico che generale eseguendo azioni nella fase di convalida iniziale e nel "else" della clausola if(valid) .

Ancora meglio, convalida separata dall'azione utilizzando due metodi:

3.

bool AClass::isValid() 
{
     bool valid = true;
     if (!SomeCondition())
     {
         Warning("AClass::isValid: your description");
         valid = false;
     }    
     return valid;
}

bool AClass::myMethod()
{
     bool valid = isValid();
     if(valid) 
     {
         doSomthing();
     }
     return valid;
}