Zusätzliche Zeile im Block vs. zusätzlicher Parameter in Clean Code

33

Kontext

In Clean Code , Seite 35, heißt es

Dies impliziert, dass die Blöcke in if-Anweisungen, else-Anweisungen, while-Anweisungen usw. eine Zeile lang sein sollten. Wahrscheinlich sollte diese Zeile ein Funktionsaufruf sein. Dies hält nicht nur die umschließende Funktion klein, sondern fügt auch einen dokumentarischen Wert hinzu, da die im Block aufgerufene Funktion einen aussagekräftigen Namen haben kann.

Ich stimme vollkommen zu, das macht sehr viel Sinn.

Später, auf Seite 40, werden Funktionsargumente beschrieben

Die ideale Anzahl von Argumenten für eine Funktion ist Null (Null). Als nächstes kommt eins (monadisch), dicht gefolgt von zwei (dyadisch). Drei Argumente (Triade) sollten nach Möglichkeit vermieden werden. Mehr als drei (polyadisch) erfordern eine besondere Begründung - und sollten dann sowieso nicht verwendet werden. Argumente sind schwer. Sie verbrauchen viel konzeptionelle Kraft.

Ich stimme vollkommen zu, das macht sehr viel Sinn.

Problem

Ich stelle jedoch ziemlich oft fest, dass ich eine Liste aus einer anderen Liste erstelle und mit einem von zwei Übeln leben muss.

Entweder verwende ich zwei Zeilen im Block , eine zum Erstellen des Objekts, eine zum Hinzufügen zum Ergebnis:

    public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            Flurp flurp = CreateFlurp(badaBoom);
            flurps.Add(flurp);
        }
        return flurps;
    }

Oder ich füge der Funktion ein Argument für die Liste hinzu, in die das Objekt eingefügt wird, wodurch es "ein Argument schlechter" wird.

    public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            CreateFlurpInList(badaBoom, flurps);
        }
        return flurps;
    }

Frage

Gibt es (Un-) Vorteile, die ich nicht sehe und die einen generell vorziehen? Oder gibt es solche Vorteile in bestimmten Situationen; Auf was muss ich in diesem Fall achten, wenn ich eine Entscheidung treffe?

R. Schmitz
quelle
58
Was ist los mit flurps.Add(CreateFlurp(badaBoom));?
cmaster
47
Nein, es ist nur eine einzige Aussage. Es ist nur ein trivial verschachtelter Ausdruck (eine einzelne verschachtelte Ebene). Und wenn ein Simple f(g(x))gegen deinen Style-Guide ist, kann ich deinen Style-Guide nicht reparieren. Ich meine, du bist auch nicht sqrt(x*x + y*y)in vier Zeilen aufgeteilt, oder? Und das sind drei (!) Verschachtelte Unterausdrücke auf zwei (!) Inneren Verschachtelungsebenen (keuchen!). Ihr Ziel sollte die Lesbarkeit sein , nicht Aussagen einzelner Bediener. Wenn Sie das später wollen, dann habe ich die perfekte Sprache für Sie: Assembler.
cmaster
6
@cmaster Sogar x86-Assemblys enthalten strikt keine Anweisungen mit einem Operator. Die Speicheradressierungsmodi umfassen viele komplizierte Operationen und können für die Arithmetik verwendet werden. Tatsächlich können Sie einen Turing-vollständigen Computer nur mit x86- movAnweisungen und einer einzigen jmp toStartam Ende erstellen . Jemand hat tatsächlich einen Compiler erstellt, der genau das tut: D
Luaan
5
@Luaan Ganz zu schweigen von der berüchtigten rlwimiAnweisung zum PPC. (Dies steht für "Direktes Einfügen einer Maske für das linke Wort drehen".) Dieser Befehl benötigte nicht weniger als fünf Operanden (zwei Register und drei Direktwerte) und führte die folgenden Vorgänge aus: Ein Registerinhalt wurde durch eine Sofortverschiebung gedreht, eine Maske wurde erstellt mit einem einzigen Lauf von 1 Bits, der von den beiden anderen unmittelbaren Operanden gesteuert wurde, und die Bits, die 1 Bits in dieser Maske im anderen Registeroperanden entsprachen, wurden durch die entsprechenden Bits des gedrehten Registers ersetzt. Sehr coole Anleitung :-)
cmaster
7
@ R.Schmitz "Im Doing General Purpose Programming" - Nein, Sie programmieren für einen bestimmten Zweck (Ich weiß nicht, welchen Zweck, aber ich gehe davon aus, dass Sie dies tun ;-). Es gibt buchstäblich Tausende von Zwecken für die Programmierung, und die optimalen Codierungsstile variieren - was für Sie geeignet ist, ist möglicherweise nicht für andere geeignet und umgekehrt: Häufig ist der Ratschlag hier absolut (" immer tun, X; Y ist schlecht") "etc) zu ignorieren, dass es in einigen Bereichen völlig unpraktisch ist, sich daran zu halten. Deshalb sollten Ratschläge in Büchern wie Clean Code immer mit einer Prise (praktischem) Salz genommen werden :)
psmears

Antworten:

104

Diese Richtlinien sind ein Kompass, keine Karte. Sie weisen Sie in eine vernünftige Richtung . Aber sie können nicht wirklich absolut sagen, welche Lösung „am besten“ ist. Irgendwann müssen Sie nicht mehr in die Richtung gehen, in die Ihr Kompass zeigt, weil Sie an Ihrem Ziel angekommen sind.

Clean Code fordert Sie auf, Ihren Code in sehr kleine, offensichtliche Blöcke zu unterteilen. Das ist eine allgemein gute Richtung. Im Extremfall (wie eine wörtliche Interpretation des zitierten Hinweises nahelegt) haben Sie Ihren Code jedoch in unnötig kleine Teile unterteilt. Nichts tut wirklich etwas, alles delegiert nur. Dies ist im Wesentlichen eine andere Art der Code-Verschleierung.

Es ist Ihre Aufgabe, „kleiner ist besser“ gegen „zu klein ist nutzlos“ abzuwägen. Fragen Sie sich, welche Lösung einfacher ist. Für mich ist das eindeutig die erste Lösung, da sie offensichtlich eine Liste zusammenstellt. Dies ist eine gut verstandene Redewendung. Es ist möglich, diesen Code zu verstehen, ohne eine weitere Funktion anschauen zu müssen.

Wenn es möglich ist, bessere Ergebnisse zu erzielen, können Sie feststellen, dass „alle Elemente von einer Liste in eine andere Liste umwandeln“ ein häufig vorkommendes Muster ist, das häufig mithilfe einer funktionalen map()Operation abstrahiert werden kann . In C #, denke ich, heißt es Select. Etwas wie das:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    return badaBooms.Select(BadaBoom => CreateFlurp(badaBoom)).ToList();
}
amon
quelle
7
Der Code ist immer noch falsch und erfindet das Rad sinnlos neu. Warum anrufen, CreateFlurps(someList)wenn die BCL bereits bietet someList.ConvertAll(CreateFlurp)?
Ben Voigt
44
@BenVoigt Dies ist eine Frage auf Designebene. Die genaue Syntax interessiert mich nicht , zumal ein Whiteboard keinen Compiler hat (und ich habe zuletzt C # in '09 geschrieben). Mein Punkt ist nicht "Ich habe den bestmöglichen Code gezeigt", sondern "beiseite, dies ist ein verbreitetes Muster, das bereits gelöst ist". Linq ist eine Möglichkeit , es, die ConvertAll Sie erwähnen , zu tun , eine andere . Vielen Dank, dass Sie diese Alternative vorgeschlagen haben.
Amon
1
Ihre Antwort ist vernünftig, aber die Tatsache, dass LINQ die Logik abstrahiert und die Aussage auf eine Zeile reduziert, scheint Ihrem Rat zu widersprechen. Als Randnotiz BadaBoom => CreateFlurp(badaBoom)ist überflüssig; Sie können CreateFlurpals Funktion direkt übergeben ( Select(CreateFlurp)). (Soweit ich weiß, war dies immer der Fall.)
jpmc26
2
Beachten Sie, dass dadurch die Methode nicht mehr benötigt wird. Der Name CreateFlurpsist irreführender und schwerer zu verstehen als nur zu sehen badaBooms.Select(CreateFlurp). Letzteres ist vollständig deklarativ - es gibt kein Problem bei der Zerlegung und daher überhaupt keine Notwendigkeit für eine Methode.
Carl Leth
1
@ R.Schmitz Es ist nicht schwer zu verstehen, aber es ist weniger leicht zu verstehen als badaBooms.Select(CreateFlurp). Sie erstellen eine Methode so, dass ihr Name (hohe Ebene) für ihre Implementierung (niedrige Ebene) steht. In diesem Fall sind sie auf dem gleichen Niveau. Um herauszufinden, was genau passiert, muss ich mir die Methode ansehen (anstatt sie inline zu sehen). CreateFlurps(badaBooms)könnte überraschen, badaBooms.Select(CreateFlurp)kann aber nicht. Es ist auch irreführend, weil fälschlicherweise nach einem Liststatt nach einem gefragt wird IEnumerable.
Carl Leth
61

Die ideale Anzahl von Argumenten für eine Funktion ist null (niladisch)

Nein! Die ideale Anzahl von Argumenten für eine Funktion ist eins. Wenn der Wert Null ist, müssen Sie sicherstellen, dass die Funktion auf externe Informationen zugreifen muss, um eine Aktion ausführen zu können. "Onkel" Bob hat das sehr falsch verstanden.

In Bezug auf Ihren Code enthält Ihr erstes Beispiel nur zwei Zeilen im Block, da Sie in der ersten Zeile eine lokale Variable erstellen. Entfernen Sie diese Zuordnung, und halten Sie sich an die folgenden Richtlinien für sauberen Code:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    List<Flurp> flurps = new List<Flurp>();
    foreach (BadaBoom badaBoom in badaBooms)
    {
        flurps.Add(CreateFlurp(badaBoom));
    }
    return flurps;
}

Aber das ist sehr langwieriger (C #) Code. Mach es einfach so:

IEnumerable<Flurp> CreateFlurps(IEnumerable<BadaBoom> badaBooms) =>
    from badaBoom in babaBooms select CreateFlurp(badaBoom);
David Arno
quelle
14
Eine Funktion mit Nullargumenten soll implizieren, dass das Objekt die erforderlichen Daten einkapselt, nicht dass sich Dinge in einem globalen Zustand außerhalb eines Objekts befinden.
Ryathal
19
@Ryathal, zwei Punkte: (1) Wenn Sie Methoden sprechen, wird für die meisten (alle?) OO-Sprachen dieses Objekt als erster Parameter abgeleitet (oder im Fall von Python explizit angegeben). In Java, C # usw. sind alle Methoden Funktionen mit mindestens einem Parameter. Der Compiler verbirgt dieses Detail nur vor Ihnen. (2) Ich habe "global" nie erwähnt. Der Objektzustand ist beispielsweise außerhalb einer Methode.
David Arno
17
Ich bin mir ziemlich sicher, als Onkel Bob "Null" schrieb, meinte er "Null (ohne dies zu zählen)".
Doc Brown
26
@DocBrown, wahrscheinlich weil er ein großer Fan des Mischens von Zustand und Funktionalität in Objekten ist, bezieht er sich mit "Funktion" wahrscheinlich speziell auf Methoden. Und ich bin immer noch nicht einverstanden mit ihm. Es ist weitaus besser, einer Methode nur das zu geben, was sie benötigt, als sie das Objekt durchsuchen zu lassen, um das zu erhalten, was sie will (dh es ist klassisch "sagen, nicht fragen" in Aktion).
David Arno
8
@AlessandroTeruzzi, Das Ideal ist ein Parameter. Null ist zu wenig. Aus diesem Grund verwenden beispielsweise funktionale Sprachen einen Parameter als Anzahl der Parameter für Lehrzwecke (in einigen funktionalen Sprachen haben alle Funktionen genau einen Parameter: nicht mehr, nicht weniger). Mit Null-Parametern zu arbeiten, wäre unsinnig. "Das Ideal ist so wenig wie möglich, Ergo Null ist am besten" ist ein Beispiel für reductio ad absurdum .
David Arno
19

Der Hinweis 'Clean Code' ist völlig falsch.

Verwenden Sie zwei oder mehr Zeilen in Ihrer Schleife. Das Ausblenden derselben zwei Zeilen in einer Funktion ist sinnvoll, wenn es sich um zufällige mathematische Berechnungen handelt, für die eine Beschreibung erforderlich ist. Wenn die Zeilen bereits beschreibend sind, hat dies jedoch keine Auswirkungen. 'Erstellen' und 'Hinzufügen'

Die zweite Methode, die Sie in erwähnen, ist nicht wirklich sinnvoll, da Sie nicht gezwungen sind, ein zweites Argument hinzuzufügen, um die beiden Zeilen zu vermeiden.

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
    {
        List<Flurp> flurps = new List<Flurp>();
        foreach (BadaBoom badaBoom in badaBooms)
        {
            flurps.Add(badaBoom .CreateFlurp());
            //or
            badaBoom.AddToListAsFlurp(flurps);
            //or
            flurps.Add(new Flurp(badaBoom));
            //or
            //make flurps a member of the class
            //use linq.Select()
            //etc
        }
        return flurps;
    }

oder

foreach(var flurp in ConvertToFlurps(badaBooms))...

Wie von anderen angemerkt, wird der Rat, dass die beste Funktion eine Funktion ohne Argumente ist, bestenfalls auf OOP und im schlimmsten Fall auf schlechten Rat verzerrt

Ewan
quelle
Vielleicht möchten Sie diese Antwort bearbeiten, um sie klarer zu machen? Meine Frage war, ob eine Sache die andere unter Clean Code überwiegt. Sie sagen, dass das Ganze falsch ist, und beschreiben dann eine der Optionen, die ich gegeben habe. Im Moment sieht diese Antwort so aus, als würden Sie einer Anti-Clean-Code-Agenda folgen, anstatt tatsächlich zu versuchen, die Frage zu beantworten.
R. Schmitz
Es tut mir leid, dass ich Ihre Frage so interpretiert habe, als ob der erste Weg der "normale" wäre, aber Sie wurden in den zweiten gedrängt. Ich bin im Allgemeinen kein Anti-Clean-Code, aber dieses Zitat ist offensichtlich falsch
Ewan
19
@ R.Schmitz Ich habe "Clean Code" selbst gelesen und folge den meisten Aussagen dieses Buches. Bezüglich der perfekten Funktionsgröße, die so ziemlich eine einzige Aussage ist, ist dies jedoch einfach falsch. Der einzige Effekt ist, dass es Spaghetti-Code in Reis-Code verwandelt. Der Leser verliert sich in der Vielzahl trivialer Funktionen, die nur zusammen eine sinnvolle Bedeutung ergeben. Menschen haben eine begrenzte Arbeitsspeicherkapazität, und Sie können diese entweder mit Anweisungen oder mit Funktionen überladen. Sie müssen ein Gleichgewicht zwischen den beiden herstellen, wenn Sie lesbar sein möchten. Vermeiden Sie die Extreme!
cmaster
@cmaster Die Antwort war nur die ersten 2 Absätze, als ich diesen Kommentar schrieb. Es ist jetzt eine viel bessere Antwort.
R. Schmitz
7
Ehrlich gesagt habe ich meine kürzere Antwort vorgezogen. In den meisten dieser Antworten wird zu viel diplomatisch gesprochen. Der zitierte Rat ist einfach falsch, es ist nicht erforderlich, darüber zu spekulieren, was es wirklich bedeutet, oder sich umzudrehen, um eine gute Interpretation zu finden.
Ewan
15

Zweitens ist es definitiv schlimmer, da es CreateFlurpInListeine Liste akzeptiert und diese modifiziert, wodurch die Funktion nicht rein und schwieriger zu durchdenken ist. Nichts im Methodennamen deutet darauf hin, dass die Methode nur zur Liste hinzugefügt wird.

Und ich biete die dritte, beste Option an:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
    return badaBooms.Select(CreateFlurp).ToList();
}

Und zum Teufel, Sie können diese Methode sofort einbinden, wenn es nur einen Ort gibt, an dem sie verwendet wird, da der Einzeiler für sich selbst klar ist, sodass er nicht durch eine Methode eingekapselt werden muss, um ihm eine Bedeutung zu verleihen.

Euphorisch
quelle
Ich würde mich nicht so sehr darüber beschweren, dass diese Methode "nicht rein und schwer zu überlegen ist" (obwohl sie wahr ist), sondern dass sie eine völlig unnötige Methode zur Behandlung eines Sonderfalls ist. Was ist, wenn ich einen eigenständigen Flurp erstellen möchte, einen zu einem Array hinzugefügten Flurp, ein Wörterbuch, einen Flurp, der dann in einem Wörterbuch nachgeschlagen und der entsprechende Flurp entfernt wird usw.? Mit dem gleichen Argument würde der Flurp-Code auch alle diese Methoden benötigen.
gnasher729
10

Die Version mit einem Argument ist besser, aber nicht in erster Linie wegen der Anzahl der Argumente.

Der wichtigste Grund, warum es besser ist, ist die geringere Kopplung , die es nützlicher macht, einfacher zu überlegen, einfacher zu testen und weniger wahrscheinlich in kopierte + eingefügte Klone umzuwandeln.

Wenn Sie mich mit einem zur Verfügung stellen CreateFlurp(BadaBoom), das kann ich mit jeder Art von Sammelbehälters verwenden: Einfache Flurp[], List<Flurp>, LinkedList<Flurp>, Dictionary<Key, Flurp>, und so weiter. Aber mit a CreateFlurpInList(BadaBoom, List<Flurp>)komme ich morgen auf Sie zurück und CreateFlurpInBindingList(BadaBoom, BindingList<Flurp>)frage, damit mein Ansichtsmodell die Benachrichtigung erhält, dass sich die Liste geändert hat. Yuck!

Als zusätzlicher Vorteil passt die einfachere Signatur eher zu vorhandenen APIs. Sie sagen, Sie haben ein wiederkehrendes Problem

Ziemlich oft erstelle ich eine Liste aus einer anderen Liste

Es ist nur eine Frage der Verwendung der verfügbaren Tools. Die kürzeste, effizienteste und beste Version ist:

var Flurps = badaBooms.ConvertAll(CreateFlurp);

Damit können Sie nicht nur weniger Code schreiben und testen, sondern auch schneller, da Sie List<T>.ConvertAll()genau wissen, dass das Ergebnis die gleiche Anzahl von Elementen enthält wie die Eingabe, und die Ergebnisliste vorab der richtigen Größe zuweisen. Während Ihr Code (beide Versionen) das Erweitern der Liste erforderte.

Ben Voigt
quelle
Nicht verwenden List.ConvertAll. Die idiomatische Methode zum Zuordnen einer Anzahl von Objekten zu verschiedenen Objekten in C # wird aufgerufen Select. Der einzige Grund, der ConvertAllhier überhaupt zur Verfügung steht, ist, dass das OP fälschlicherweise nach einer Listin der Methode fragt - es sollte eine sein IEnumerable.
Carl Leth
6

Behalten Sie das übergeordnete Ziel im Hinterkopf: Einfacheres Lesen und Verwalten des Codes.

Oft ist es möglich, mehrere Zeilen zu einer einzigen sinnvollen Funktion zusammenzufassen. Tun Sie dies in diesen Fällen. Gelegentlich müssen Sie Ihren allgemeinen Ansatz überdenken.

Ersetzen Sie in Ihrem Fall beispielsweise die gesamte Implementierung durch var

flups = badaBooms.Select(bb => new Flurp(bb));

könnte eine Möglichkeit sein. Oder Sie könnten so etwas tun

flups.Add(new Flurp(badaBoom))

Manchmal passt die sauberste und am besten lesbare Lösung einfach nicht in eine Zeile. Sie haben also zwei Zeilen. Machen Sie den Code nicht schwerer zu verstehen, nur um eine willkürliche Regel zu erfüllen.

Ihr zweites Beispiel ist meiner Meinung nach wesentlich schwerer zu verstehen als das erste. Es ist nicht nur so, dass Sie einen zweiten Parameter haben, sondern dass der Parameter durch die Funktion geändert wird. Schauen Sie nach, was Clean Code dazu zu sagen hat. (Ich habe das Buch gerade nicht zur Hand, aber ich bin mir ziemlich sicher, dass es im Grunde genommen "Mach das nicht, wenn du es vermeiden kannst").

doubleYou
quelle