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

63

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.

willem
quelle
13
Option 2 ähnelt dem, was Martin Fowler in seinem Refactoring-Buch empfiehlt. Plus Ihre Methodennamen dienen als die Absicht aller zufälligen Ausdrücke, der Inhalt der Methoden sind nur die Implementierungsdetails, die sich im Laufe der Zeit ändern können.
Programmierer
2
Ist es wirklich der gleiche Ausdruck? "Oder" hat eine geringere Priorität als "Und". Wie auch immer, die zweite sagt Ihre Absicht, die andere (erste) ist technisch.
Thepacker
3
Was @thepacker sagt. Die Tatsache, dass Sie beim ersten Mal einen Fehler gemacht haben, ist ein ziemlich guter Hinweis darauf, dass der erste Weg für einen sehr wichtigen Sektor Ihrer Zielgruppe nicht leicht verständlich ist. Dich selber!
Steve Jessop
3
Option 3: Ich mag keine. Der zweite ist lächerlich wortreich, der erste ist nicht gleichbedeutend mit dem zweiten. Klammern helfen.
David Hammen
3
Das mag umständlich sein, aber Sie haben in keinem der if Codeblöcke Anweisungen. Ihre Frage bezieht sich auf Boolesche Ausdrücke .
Kyle Strand

Antworten:

88

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.]

Wunderglocke
quelle
12
Wenn die Methodennamen gut sind, ist es auch einfacher zu verstehen.
BЈовић
Und bitte machen Sie sie (Methodennamen) aussagekräftig und kurz. Mehr als 20 Zeichen Methodennamen haben meine Augen verletzt. MatchesDefinitionId()ist grenzwertig.
Mindwin
2
@Mindwin Wenn es darum geht, Methodennamen kurz und aussagekräftig zu halten, nehme ich jedes Mal die letzteren. Kurz ist gut, geht aber nicht zu Lasten der Lesbarkeit.
Ajedi32
@ Ajedi32 man muss keinen Aufsatz darüber schreiben, was die Methode mit dem Methodennamen macht, oder grammatisch fundierte Methodennamen haben. Wenn man die Abkürzungsstandards klar hält (über die Arbeitsgruppe oder Organisation hinweg), ist dies kein Problem mit Kurznamen und Lesbarkeit.
Mindwin
Verwenden Sie das Zipf-Gesetz: Machen Sie die Dinge ausführlicher, um ihre Verwendung zu verhindern.
hoosierEE
44

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.

Simon Richter
quelle
3
Diese Lösung ist ziemlich gut und ich habe sicherlich viele ähnliche Codes geschrieben. Es ist sehr gut lesbar. Der Nachteil im Vergleich zu der von mir veröffentlichten Lösung ist die Geschwindigkeit. Mit dieser Methode führen Sie eine Reihe von bedingten Tests durch, egal was passiert. In meiner Lösung können die Vorgänge basierend auf den verarbeiteten Werten drastisch reduziert werden.
BuvinJ
5
@BuvinJ Tests wie die hier gezeigten sollten ziemlich billig sein. Wenn ich also nicht weiß, dass einige der Bedingungen teuer sind oder es sich nicht um äußerst leistungsempfindlichen Code handelt, würde ich mich für die besser lesbare Version entscheiden.
Svick
1
@svick Zweifellos ist es unwahrscheinlich, dass dies die meiste Zeit zu einem Leistungsproblem führt. Wenn Sie die Anzahl der Vorgänge verringern können, ohne die Lesbarkeit zu verlieren, warum dann nicht? Ich bin nicht davon überzeugt, dass dies viel lesbarer ist als meine Lösung. Es gibt selbstdokumentierende "Namen" für die Tests - was schön ist ... Ich denke, es kommt auf den spezifischen Anwendungsfall an und wie verständlich die Tests für sich sind.
BuvinJ
Das Hinzufügen von Kommentaren kann auch die Lesbarkeit
verbessern
@BuvinJ Was ich an dieser Lösung wirklich mag, ist, dass ich schnell verstehen kann, was sie tut, wenn ich alles außer der letzten Zeile ignoriere. Ich denke, das ist besser lesbar.
Svick
42

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.

Telastyn
quelle
1
Ja, Ihre Antwort sollte sich jedoch auch mit der Festlegung der Verwendung von befassen, bei der es sich um repoein statisches Feld / eine statische Eigenschaft handelt, dh um eine globale Variable. Statische Methoden sollten deterministisch sein und keine globalen Variablen verwenden.
David Arno
3
@DavidArno - das ist zwar nicht so toll, scheint aber tangential zur vorliegenden Frage zu sein. Und ohne weiteren Code ist es plausibel, dass es einen vorübergehenden Grund gibt, warum das Design so funktioniert.
Telastyn
1
Ja, egal Repo. Ich musste den Code ein wenig verschleiern, wollte den Client-Code nicht so wie er ist in den Interwebs teilen :)
willem
23

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;   
}
BuvinJ
quelle
3
Warum habe ich innerhalb von Sekunden nach dem Posten eine Gegenstimme erhalten? Bitte geben Sie einen Kommentar ein, wenn Sie abstimmen! Diese Antwort funktioniert genauso schnell und ist leichter zu lesen. Also, was ist das Problem?
BuvinJ
2
@BuvinJ: Absolut nichts falsch daran. Wie der ursprüngliche Code, außer dass Sie nicht mit einem Dutzend Klammern und einer einzelnen Zeile kämpfen müssen, die über das Ende des Bildschirms hinausreicht. Ich kann diesen Code von oben nach unten lesen und sofort verstehen. WTF count = 0.
gnasher729
1
Wenn Sie einen anderen Code als am Ende der Funktion zurückgeben, wird der Code weniger lesbar und nicht mehr lesbar, IMO. Ich bevorzuge einen einzelnen Austrittspunkt. Einige gute Argumente in beide Richtungen unter diesem Link. stackoverflow.com/questions/36707/…
Brad Thomas
5
@Brad thomas Ich kann dem einzelnen Austrittspunkt nicht zustimmen. Dies führt normalerweise zu tief verschachtelten Klammern. Die Rückkehr beendet den Weg, so ist für mich viel leichter zu lesen.
Borjab
1
@BradThomas Ich stimme Borjab voll und ganz zu. Das Vermeiden tiefer Verschachtelungen ist eigentlich der Grund, warum ich diesen Stil häufiger verwende, als lange bedingte Anweisungen aufzulösen. Ich finde mich dabei, Code mit Tonnen von Verschachtelungen zu schreiben. Dann begann ich nach Wegen zu suchen, um kaum mehr als ein oder zwei Schachtelungen tief zu gehen, und mein Code wurde dadurch VIEL einfacher zu lesen und zu warten. Wenn Sie eine Möglichkeit finden, Ihre Funktion zu beenden, tun Sie dies so bald wie möglich! Wenn Sie einen Weg finden, tiefe Verschachtelungen und lange Bedingungen zu vermeiden, tun Sie dies!
BuvinJ
10

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.

Dan Neely
quelle
Sehr schön, zu erklären, was in den Methoden gemacht wird, ist eigentlich besser als nur Aufrufe umzustrukturieren.
klaar
2

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.

Erik Eidt
quelle
0

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.

Schnüffeln
quelle
0

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.

Mark Wood
quelle
Verdient eine +1. Guter Anlass zum Nachdenken, auch wenn die anderen Antworten keine allgemein verbreitete Meinung sind. Danke :)
willem
1
@willem Nein, es hat +1 nicht verdient. Zwei Ansätze sind nicht dasselbe. Die zusätzlichen Kommentare sind dumm und unnötig.
BЈови at
2
Ein guter Code hängt NIEMALS von Kommentaren ab, um verständlich zu sein. Tatsächlich sind die Kommentare das schlimmste Durcheinander, das ein Code haben könnte. Der Code sollte für sich selbst sprechen. Außerdem können die beiden Ansätze, die OP evaluieren möchte, niemals "ungefähr gleich" sein, egal wie viele Leerzeichen man hinzufügt.
WonderBell
Es ist besser, einen aussagekräftigen Funktionsnamen zu haben, als den Kommentar lesen zu müssen. Wie im Buch "Clean Code" angegeben, ist ein Kommentar ein Fehler beim Ausdrücken von Wurfcode. Warum erklären Sie, was Sie tun, wenn die Funktion dies deutlicher hätte ausdrücken können?
Borjab
0

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);
}
Deduplizierer
quelle
0

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;
}
gnasher729
quelle
-4

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
}
user1152226
quelle
1
Sollte kein Problem sein, so etwas in C # zu implementieren.
Snoop
Ich verstehe nicht, wie eine boolesche Kombination von ~ 5 Aufrufen compareContextProp(propVal, "PropertyId", context.Definition.Id)leichter zu lesen wäre als die boolesche Kombination von ~ 5 Vergleichen des Formulars im OP propVal.PropertyId == context.Definition.Id. Es ist bedeutend länger und fügt eine zusätzliche Ebene hinzu, ohne die Komplexität wirklich vor der Anrufstelle zu verbergen. (wenn es darauf ankommt, habe ich nicht abgelehnt)
Ixrec