Refactoring test condizionali verbali [duplicato]

A prima vista, il tuo approccio "chiaro" sembra compatto e pulito, ma se dovessi svelare le affermazioni ...

// PHP?
public function effective()
{
    $nonGroup  = !$this->org->isGroup();
    $hasParent = false;
    if ($nonGroup) {
        $hasParent = ($parent = $this->org->getParentOrg()) instanceof OrgModel;
    }
    $inherits  = false;
    if ($hasParent) {
        $inherits  = $this->org->isParentPropertyInherited();
    }
    $nonRoot   = false;
    if ($inherits) { 
        $nonRoot   = !$parent->isRoot();
    }
    if ($nonRoot) {
        return $parent->expiration()->effective();
    } else {
        return $this->get();
    }
}

L'uso di variabili temporanee qui solo per semplificare l'anti-pattern della freccia si manifesta in un altro problema, che è quello che io definisco come deboli dichiarazioni standalone if che non comunicano chiaramente la logica originale. Se sono letteralmente a leggere questo:

• nonGroup controlla se questa organizzazione non è un gruppo.
• se non di gruppo, hasParent controlla se questo genitore è un'istanza di OrgModel .
• se ha-parent, inherits controlla se la proprietà genitore è ereditata.
• se eredita, nonRoot controlla se genitore non è root.
• se non-root, restituisce la scadenza effettiva del genitore.
• altrimenti, la data di scadenza dell'organizzazione.

Questo significa che devo ricordare quattro variabili temporanee, quando vengono "saltate" (e dato il valore predefinito false ), e sono anche incapace di vedere il collegamento da una condizione all'altra, chiaramente descritto nell'originale documentazione del codice:

• If a non-group org has a parent,
  • inherits the parent's properties, and
    • the parent isn't a root org,
      • return the parent's expiration date.
• Otherwise, return the org's expiration date.

Non hai rivelato l'approccio di fail-early che hai scelto, ma questo è il modo in cui potrei tentare (perdonare le mie abilità PHP estremamente arrugginite):

public function effective() {
    $org = $this->org;
    if ($org->isGroup()) {
        return $this->get();
    }
    $parent = $org->getParentOrg();
    return $parent instanceof OrgModel
            && $org->isParentPropertyInherited() 
            && !$parent->isRoot() ? $parent->expiration()->effective() : $this->get();
}

L'unica variabile temporanea non necessaria che ho creato qui è $org , ma è proprio così che non devo ripetere $this->org .

Un ulteriore perfezionamento della tua base di codice, è per isParentPropertyInherited() fai anche il controllo instanceof internamente, se appropriato . Questo può notevolmente semplificare il corpo a solo:

public function effective() {
    return !($org = $this->org)->isGroup()
            && $org->isParentPropertyInherited() 
            && !($parent = $org->getParentOrg())->isRoot() 
            ? $parent->expiration()->effective() : $this->get();
}

Questo non è inverosimile dal momento che se l'org eredita le proprietà di un genitore, implica che il genitore esiste.

Questo sembra ignorare il bello e semplice:

return complicated_test_1() && complicated_test_2() && complicated_test_3();

Questo è sicuramente più semplice di entrambe le opzioni e presumo che tu l'abbia rifiutato perché:

the complicated_test are not single function calls, but themselves Boolean expressions

Ma questo è un problema autoinflitto! Hai estratto le espressioni booleane in metodi nella tua domanda per verificarne la leggibilità, quindi perché non fare la stessa cosa nel tuo codice reale? Basta applicare un metodo di estrazione di refactoring e scegliere alcuni nomi più espressivi di complicated_test_n , e sei a posto. E non avevamo nemmeno bisogno di monadi!