Ist ein großer boolescher Ausdruck lesbarer als der gleiche Ausdruck, der in Prädikatmethoden unterteilt ist? [geschlossen]

Was ist einfacher zu verstehen, eine große boolesche Anweisung (ziemlich komplex) oder dieselbe Anweisung, die in Prädikatmethoden unterteilt ist (viel zusätzlichen Code zum Lesen)?

Option 1, der große boolesche Ausdruck:

    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.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Option 2: Die Bedingungen sind in Prädikatmethoden unterteilt:

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

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

Ich bevorzuge den zweiten Ansatz, weil ich die Methodennamen als Kommentare sehe, aber ich verstehe, dass dies problematisch ist, weil Sie alle Methoden lesen müssen, um zu verstehen, was der Code tut, damit die Absicht des Codes abstrahiert wird.

Was ist leichter zu verstehen

Letzterer Ansatz. Es ist nicht nur einfacher zu verstehen, sondern auch einfacher zu schreiben, zu testen, umzugestalten und zu erweitern. Jeder erforderliche Zustand kann sicher entkoppelt und auf seine eigene Weise behandelt werden.

Das ist problematisch, weil Sie alle Methoden lesen müssen, um den Code zu verstehen

Es ist nicht problematisch, wenn die Methoden richtig benannt sind. Tatsächlich wäre es einfacher zu verstehen, da der Methodenname die Bedingungsabsicht beschreibt.
Für einen Betrachter if MatchesDefinitionId()ist das eher erklärend alsif (propVal.PropertyId == context.Definition.Id)

[Persönlich wund der erste Ansatz meine Augen.]

Wenn dies der einzige Ort ist, an dem diese Prädikatfunktionen verwendet werden, können Sie boolstattdessen auch lokale Variablen verwenden:

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;
}

Diese könnten auch weiter aufgeschlüsselt und neu angeordnet werden, um sie lesbarer zu machen, z. B. mit

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

und dann Ersetzen aller Instanzen von propVal.SecondaryFilter.HasValue. Eine Sache, die dann sofort auffällt, ist, dass hasNoSecondaryFilterfür die negierten HasValueEigenschaften matchesSecondaryFilterein logisches UND und für die nicht negierten Eigenschaften ein logisches UND verwendet wird HasValue- es ist also nicht das genaue Gegenteil.

Im Allgemeinen ist letzteres bevorzugt.

Dies macht die Call-Site wiederverwendbarer. Es unterstützt DRY (dh Sie haben weniger Änderungsmöglichkeiten, wenn sich die Kriterien ändern, und können dies zuverlässiger tun). Und sehr oft sind diese Unterkriterien Dinge, die unabhängig voneinander an anderer Stelle wiederverwendet werden, sodass Sie dies tun können.

Oh, und es macht dieses Zeug viel einfacher für den Komponententest und gibt Ihnen die Gewissheit, dass Sie es richtig gemacht haben.

Wenn es zwischen diesen beiden Möglichkeiten liegt, ist letzteres besser. Dies sind jedoch nicht die einzigen Möglichkeiten! Wie wäre es, wenn Sie die einzelne Funktion in mehrere Wenns aufteilen? Prüfen Sie, wie Sie die Funktion beenden können, um zusätzliche Tests zu vermeiden, indem Sie einen "Kurzschluss" in einem Test mit einer einzelnen Leitung grob emulieren.

Dies ist einfacher zu lesen (Sie müssen möglicherweise die Logik für Ihr Beispiel überprüfen, aber das Konzept gilt):

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;   
}

Ich mag Option 2 besser, würde aber eine strukturelle Änderung vorschlagen. Kombinieren Sie die beiden Prüfungen in der letzten Zeile der Bedingung zu einem einzigen Aufruf.

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);
}

Der Grund, warum ich dies vorschlage, ist, dass die beiden Prüfungen eine einzige Funktionseinheit sind und das Verschachteln von Klammern in eine Bedingung fehleranfällig ist: Sowohl vom Standpunkt des anfänglichen Schreibens des Codes als auch vom Standpunkt der Person, die ihn liest. Dies ist insbesondere dann der Fall, wenn die Unterelemente des Ausdrucks nicht dem gleichen Muster folgen.

Ich bin mir nicht sicher, ob MatchesSecondaryFilterIfPresent()der beste Name für die Kombination ist. aber nichts Besseres fällt mir sofort ein.

Obwohl in C # der Code nicht sehr objektorientiert ist. Es verwendet statische Methoden und sieht aus wie statische Felder (zB repo). Es wird allgemein davon ausgegangen, dass Statik die Refaktorisierung und das Testen Ihres Codes erschwert und die Wiederverwendbarkeit beeinträchtigt. Außerdem wird die Frage gestellt, ob eine solche statische Verwendung weniger lesbar und wartbar ist als eine objektorientierte Konstruktion.

Sie sollten diesen Code in ein objektorientierteres Formular konvertieren. Wenn Sie dies tun, werden Sie feststellen, dass es sinnvolle Stellen gibt, an denen Code zum Vergleichen von Objekten, Feldern usw. abgelegt werden kann. Es ist wahrscheinlich, dass Sie dann Objekte zum Vergleichen auffordern könnten, was Ihre große if-Anweisung auf a reduzieren würde einfache Vergleichsanforderung (z. B. if ( a.compareTo (b) ) { }die alle Feldvergleiche enthalten könnte)

C # verfügt über zahlreiche Schnittstellen und Systemdienstprogramme zum Vergleichen von Objekten und ihren Feldern. Neben der offensichtlichen .EqualsMethode, für den Anfang, schauen Sie in IEqualityComparer, IEquatableund Dienstprogramme wie System.Collections.Generic.EqualityComparer.Default.

Letzteres ist definitiv vorzuziehen, ich habe Fälle mit dem ersten Weg gesehen und es ist fast immer unmöglich zu lesen. Ich habe den Fehler gemacht, es auf die erste Art und Weise zu tun, und wurde gebeten, es zu ändern, um Methoden zu prädizieren.

Ich würde sagen, dass die zwei ungefähr gleich sind, WENN Sie etwas Leerzeichen für die Lesbarkeit und einige Kommentare hinzufügen, um dem Leser über die undurchsichtigeren Teile zu helfen.

Denken Sie daran: Ein guter Kommentar sagt dem Leser, was Sie gedacht haben, als Sie den Code geschrieben haben.

Bei Änderungen, wie ich sie vorgeschlagen habe, würde ich wahrscheinlich den früheren Ansatz wählen, da er weniger überladen und diffus ist. Unterprogrammaufrufe sind wie Fußnoten: Sie liefern nützliche Informationen, stören aber den Lesefluss. Wenn die Prädikate komplexer wären, würde ich sie in separate Methoden aufteilen, damit die darin enthaltenen Konzepte in verständlichen Stücken aufgebaut werden können.

Nun, wenn es Teile gibt, die Sie wiederverwenden möchten, ist es offensichtlich die beste Idee, sie in separate, ordnungsgemäß benannte Funktionen aufzuteilen.
Selbst wenn Sie sie niemals wiederverwenden, können Sie dadurch Ihre Bedingungen besser strukturieren und ihnen ein Etikett geben, das beschreibt, was sie bedeuten .

Schauen wir uns nun Ihre erste Option an und räumen Sie ein, dass weder Ihre Einrückung und Ihr Zeilenumbruch so nützlich waren, noch dass die Bedingung so gut strukturiert war:

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);
}

Der erste ist absolut schrecklich. Sie haben || verwendet für zwei Dinge auf der gleichen Linie; Das ist entweder ein Fehler in Ihrem Code oder eine Absicht, Ihren Code zu verschleiern.

    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))));

Das ist mindestens halbwegs ordentlich formatiert (wenn die Formatierung kompliziert ist, weil die if-Bedingung kompliziert ist), und Sie haben zumindest die Chance, herauszufinden, ob irgendetwas darin Unsinn ist. Verglichen mit Ihrem Müll, der formatiert wurde, ist alles andere besser. Aber Sie scheinen nur Extreme beherrschen zu können: Entweder ein komplettes Durcheinander einer if-Anweisung oder vier völlig sinnlose Methoden.

Beachten Sie, dass (cond1 && cond2) || (! cond1 && cond3) kann geschrieben werden als

cond1 ? cond2 : cond3

das würde das Durcheinander reduzieren. Ich würde schreiben

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;
}

Ich mag keine dieser Lösungen, beide sind schwer zu überlegen und schwer zu lesen. Die Abstraktion auf kleinere Methoden löst das Problem nicht immer, nur um der kleineren Methoden willen.

Idealerweise würden Sie Eigenschaften metaprogrmatisch vergleichen, sodass Sie keine neue Methode definieren oder jedes Mal verzweigen müssen, wenn Sie einen neuen Satz von Eigenschaften vergleichen möchten.

Ich bin mir bei c # nicht sicher, aber in Javascript wäre so etwas VIEL besser und könnte zumindest MatchesDefinitionId und MatchesParentId ersetzen

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