È una grande espressione booleana più leggibile della stessa espressione suddivisa in metodi predicati? [chiuso]

What is easier to understand

Il secondo approccio. Non è solo più facile da capire, ma è più facile scrivere, testare, ridefinire ed estendere. Ogni condizione richiesta può essere tranquillamente disaccoppiata e gestita a modo suo.

it's problematic because you have to read all the methods to understand the code

Non è problematico se i metodi sono nominati correttamente. In effetti sarebbe più facile capire come il nome del metodo descrivesse l'intento della condizione.
Per un onlooker if MatchesDefinitionId() è più esplicativo di if (propVal.PropertyId == context.Definition.Id)

[Personalmente, il primo approccio mi fa male agli occhi.]

Se questo è l'unico posto in cui verranno utilizzate queste funzioni di predicato, puoi anche utilizzare le variabili locali bool :

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

Questi potrebbero anche essere suddivisi ulteriormente e riordinati per renderli più leggibili, ad es. con

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

e quindi sostituendo tutte le istanze di propVal.SecondaryFilter.HasValue . Una cosa che immediatamente emerge è che hasNoSecondaryFilter utilizza AND logiche sulle proprietà HasValue negate, mentre matchesSecondaryFilter usa un AND logico su% co_nonente non negato, quindi non è l'esatto contrario.

In generale, quest'ultimo è preferito.

Rende il sito di chiamata più riutilizzabile. Supporta DRY (significa che hai meno posti da cambiare quando i criteri cambiano, e puoi farlo in modo più affidabile). E molto spesso quei sottocriteri sono cose che saranno riutilizzate indipendentemente altrove, permettendoti di farlo.

Oh, e rende questa roba molto più facile da testare unitamente, dandoti la certezza di averlo fatto correttamente.

Se è tra queste due scelte, allora quest'ultima è migliore. Queste non sono le uniche scelte, comunque! Che ne dici di suddividere la singola funzione in più if? Prova i modi per uscire dalla funzione per evitare test aggiuntivi, emulando approssimativamente un "cortocircuito" in un test a linea singola.

Questo è più facile da leggere (potrebbe essere necessario ricontrollare la logica per il tuo esempio, ma il concetto è vero):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}

Mi piace l'opzione 2, ma suggerirei un cambiamento strutturale. Combina i due controlli sull'ultima riga del condizionale in una singola chiamata.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

La ragione per cui suggerisco questo è che i due controlli sono una singola unità funzionale, e la parentesi di nidificazione in un condizionale è soggetta ad errori: sia dal punto di vista della scrittura iniziale del codice sia dal punto di vista della persona che la legge. Questo è particolarmente vero se i sotto-elementi dell'espressione non seguono lo stesso schema.

Non sono sicuro che MatchesSecondaryFilterIfPresent() sia il nome migliore per la combinazione; ma non mi viene in mente nulla di meglio.

Sebbene in C #, il codice non è molto orientato agli oggetti. Sta usando metodi statici e quelli che sembrano campi statici (ad esempio repo ). In generale si ritiene che la statica renda difficile il refactoring del codice e sia difficile da testare, ostacolando la riusabilità e, alla tua domanda: l'uso statico come questo è meno leggibile & manutenibile rispetto alla costruzione orientata agli oggetti.

Dovresti convertire questo codice in un modulo più orientato agli oggetti. Quando lo fai, scoprirai che ci sono punti sensati per mettere il codice che fa il confronto di oggetti, di campi, ecc. È probabile che tu possa quindi chiedere agli oggetti di confrontarsi, il che ridurrebbe la tua dichiarazione se grande ad un semplice richiesta di confronto (es. if ( a.compareTo (b) ) { } , che potrebbe includere tutti i confronti di campo.)

C # ha un ricco set di interfacce e utilità di sistema per fare confronti sugli oggetti e i loro campi. Oltre il ovvio metodo .Equals , per i principianti, guarda IEqualityComparer , IEquatable e utility come System.Collections.Generic.EqualityComparer.Default .

Quest'ultimo è decisamente preferito, ho visto casi con il primo modo ed è quasi sempre impossibile da leggere. Ho commesso l'errore di farlo nel primo modo e mi è stato chiesto di cambiarlo per prevedere i metodi.

Direi che i due sono all'incirca uguali, SE aggiungi degli spazi bianchi per la leggibilità e alcuni commenti per aiutare il lettore sulle parti più oscure.

Ricorda: un buon commento dice al lettore che cosa stavi pensando quando hai scritto il codice.

Con cambiamenti come quello che ho suggerito, probabilmente andrei con il primo approccio, in quanto è meno disordinato e diffuso. Le chiamate di subroutine sono come note a piè di pagina: forniscono informazioni utili ma interrompono il flusso di lettura. Se i predicati fossero più complessi, li suddividerei in metodi separati in modo che i concetti che incorporano possano essere costruiti in blocchi comprensibili.

Bene, se ci sono parti che potresti voler riutilizzare, separarle in funzioni separate nominate correttamente è ovviamente l'idea migliore.
Anche se potresti mai riutilizzarli, potresti farlo per strutturare meglio le tue condizioni e dare loro un'etichetta che descriva cosa intendi .

Ora, esaminiamo la tua prima opzione e ammettiamo che né la tua rientranza e il loro allineamento sono stati utili, né il condizionale è strutturato così bene:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}

Il primo è assolutamente orribile. Hai usato || per due cose sulla stessa linea; che è un bug nel codice o un intento di offuscare il codice.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

Questo è almeno a metà decentemente formattato (se la formattazione è complicata, perché la condizione if è complicata), e hai almeno una possibilità di capire se c'è qualcosa senza senso. Rispetto alla tua spazzatura formattata se, qualsiasi altra cosa è meglio. Ma sembra che tu sia in grado di fare solo degli estremi: o un disastro completo di un'istruzione if, o quattro metodi completamente inutili.

Si noti che (cond1 & & cond2) || (! cond1 & cond3) può essere scritto come

cond1 ? cond2 : cond3

che ridurrebbe il disordine. Scriverò

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}

Non mi piace nessuna di queste soluzioni, sono entrambe difficili da ragionare e difficili da leggere. L'astrazione in metodi più piccoli solo per metodi più piccoli non sempre risolve il problema.

Idealmente, penso che tu possa confrontare metaprogaticamente le proprietà, quindi non devi definire un nuovo metodo o un ramo ogni volta che vuoi confrontare un nuovo insieme di proprietà.

Non sono sicuro di c #, ma in javascript qualcosa del genere sarebbe MOLTO meglio e potrebbe almeno sostituire MatchesDefinitionId e MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}