Clean Code und das Principle of Separate Understandability

„Wer Clean Code nicht gelesen hat, ist kein professioneller Entwickler“, hat letztens ein Kollege mal zu mir gemeint. Ich wollte das Buch eh schon seit Ewigkeiten mal lesen, also nehm ich das jetzt einfach mal zum Anlass und lese es. Ganz kommentarlos werde ich das aber nicht tun. Wer mich kennt, weiß, dass ich reflexartig alles in Frage stelle und mir Gedanken dazu mache. Und Kraft meiner Arroganz mache ich da natürlich auch bei Uncle Bob keine Ausnahme.

Ich hab jetzt so in etwa die ersten 50 Seiten gelesen und kann zu meiner Verteidigung sagen, dass ich das meiste einfach nur rundheraus unterstützen kann. Bis auf ein paar wenige Ausnahmen sehe ich das genauso. Aber es sind natürlich die Ausnahmen, die mich zum Nachdenken anregen und die Sache so interessant machen. Deshalb lass ich erstmal die 90% Übereinstimmungen untern Tisch fallen und beschäftige mich mit den 10% Unterschieden. Ich schätze mal in der nächsten Zeit werde ich noch den ein oder anderen Blogpost dazu schreiben. Heute geht es mir um ein Beispiel zum Refactoring.

Wer das Buch hat, kann (und sollte) das Beispiel auf Seite 28f. nachlesen. Konkret geht es um ein Stück Code, das einen String ausgibt, der wohl eine Statistik zu geratenen Buchstaben ausgibt oder so. Die Fachlichkeit ist nicht weiter interessant und wird auch nicht weiter erklärt. Jedenfalls baut der Code Sätze wie „There are 25 ds“. Der Ausgangscode ist folgender (leicht abgewandelt):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
public class Statistics1
{
    public static void main(String...args)
    {
        Statistics1 statistics = new Statistics1();
        statistics.printGuessStatistics('d', 0);
        statistics.printGuessStatistics('d', 1);
        statistics.printGuessStatistics('d', 25);
    }

    private void printGuessStatistics(char candidate, int count)
    {
        String number;
        String verb;
        String pluralModifier;
        if (count == 0)
        {
            number = "no";
            verb = "are";
            pluralModifier = "s";
        }
        else if (count == 1)
        {
            number = "1";
            verb = "is";
            pluralModifier = "";
        }
        else
        {
            number = Integer.toString(count);
            verb = "are";
            pluralModifier = "s";
        }
        String guessMessage = String.format("There %s %s %s%s", verb, number, candidate, pluralModifier);
        System.out.println(guessMessage);
    }
}

Martins Argumente warum der Code schlecht ist, sind in etwa die folgenden:

Das erste Argument ist nachvollziehbar. Wie lang Methoden sein sollten, ist zwar eine Frage, die einen separaten Blog-Post rechtfertigen würde, aber Bob Martin ist ganz klar jemand, der viele sehr kleine Methoden favorisiert. Das zweite ist aber etwas komplizierter. Ja, wenn man die Methode von oben nach unten liest, wird man zuerst die Bedeutung der Variablen nicht verstehen. Und ja, das ist ein Problem. Es macht den Code schwerer verständlich, als er eigentlich sein müsste. Aber ist das Problem tatsächlich fehlender Kontext? Ich sehe das nicht so. Das Problem ist meiner Meinung eher, dass die Variablen nicht aus sich heraus verständlich sind (PSU). Der Sinn der Variablen wird erst klar, wenn man sich ansieht, wie sie benutzt werden. So wird es auch in Clean Code erklärt: Die Variablen sind nur im Kontext deren Benutzung verständlich. Ob der Begriff „Kontext“ hier so eingängig ist, ist eine andere Frage, aber schauen wir uns einmal an, was Martin so refactort:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
public class Statistics2
{
    public static void main(String...args)
    {
        GuessStatisticsMessage statistics = new GuessStatisticsMessage();
        System.out.println(statistics.make('d', 0));
        System.out.println(statistics.make('d', 1));
        System.out.println(statistics.make('d', 25));
    }

    static class GuessStatisticsMessage
    {
        private String number;
        private String verb;
        private String pluralModifier;

        public String make(char candidate, int count)
        {
            createPluralDependentMessageParts(count);
            return String.format("There %s %s %s%s", verb, number, candidate, pluralModifier);
        }

        private void createPluralDependentMessageParts(int count)
        {
            if (count == 0)
            {
                thereAreNoLetters();
            }
            else if (count == 1)
            {
                thereIsOneLetter();
            }
            else
            {
                thereAreManyLetters(count);
            }
        }

        private void thereAreNoLetters()
        {
            number = "no";
            verb = "are";
            pluralModifier = "s";
        }

        private void thereIsOneLetter()
        {
            number = "1";
            verb = "is";
            pluralModifier = "";
        }

        private void thereAreManyLetters(int count)
        {
            number = Integer.toString(count);
            verb = "are";
            pluralModifier = "s";
        }
    }
}

Der Code oben ist auch wieder nur unwesentlich abgewandelt im Vergleich zum Buch, damit das alles in eine kompilierbare java-Datei passt.

Man erkennt hier schön, die Refactoring-Schritte, die gemacht wurden:

  • Extract Method: Die einzelnen Blöcke in der if-Kette sind in separate Methoden ausgelagert worden. Und auch die if-Kette als solche wurde ausgelagert.
  • Als Folge daraus wurden die lokalen Variablen number, verb und pluralModifier zu Attributen.
  • Die Ausgabe der Message wurde vom Zusammenbauen getrennt. (Das ist auch ein Problem, das Martin aber nicht direkt aufgezählt hat. Siehe HC)
  • Extract Class: Die resultierenden Attribute und Methoden wurden in eine neue Klasse GuessStatisticsMessage ausgelagert.
  • Rename Method: printGuessStatistics macht keine Ausgabe mehr. Deshalb stimmt „print“ nicht mehr. Die Methode heißt jetzt make, was im Kontext der Klasse OK ist.

Ist der Code nach dem Refactoring besser? Schauen wir uns das mal an:

  • MIMC: Keine langen Methoden mehr: gut.
  • MIMC: Aber deutlich mehr Methoden und mehr noch: Eine zusätzliche Klasse. Nicht so gut.
  • PSU: Die Methoden thereAreNoLetters, thereIsOneLetter und thereAreManyLetters sind nicht aus sich heraus verständlich. Man muss die gesamte Klasse verstanden haben, um die Implementierung der Methoden zu verstehen. Nicht gut. Auch der Aufruf von createPluralDependentMessageParts zwingt den Leser des Codes dazu, die Implementierung der Methode anzugucken, um zu verstehen, was passiert. Wenn man jemandem nur die Methode make zeigt, wird jeder als nächstes sagen „zeig mir den Code von createPluralDependentMessageParts.“ Auch nicht gut. Der Ganze Sinn des Refactorings war ja unter anderem, den Variablen „mehr Kontext“ zu geben. Oder in meinen Worten: Zu bewirken, dass die Variablen aus sich heraus verständlich sind. Das ist immer noch nicht so. Man muss die gesamte Klasse verstanden haben, um die Attribute zu verstehen. Nicht gut. Man könnte auch sagen: Ziel verfehlt.
  • MP: Die Klasse GuessStatisticsMessage ist eigentlich falsch benannt. Es ist keine Message. Vielmehr ist es eine Abstraktion über Code, der eine Message zusammenbaut. Die Klasse müsste eher GuessStatisticsMessageComposer heißen. Und das ist eine künstliche Klasse. Nicht gut. Klassen sollten nach Möglichkeit mehr sein, als eine Abstraktion über Code. Sie sollten Konzepte kapseln. Wollte man eine Klasse GuessStatisticsMessage machen, die tatsächlich ein Konzept repräsentiert, müsste sie anders aussehen. Ich werd unten ein Beispiel geben.
  • Ich könnte noch mehr Aspekte aufzählen. ML, IAP, … Aber das reicht erstmal.

Kurz: Ich bin von dieser Lösung nicht wirklich überzeugt. Meiner Meinung nach ist das Problem hier, dass das Refactoring zu mechanisch gemacht wurde. Auch, wenn die tollen Refactoring-Features moderner IDEs wirklich ausgesprochen hilfreich sind, so gibt es dennoch sinnvolle Refactorings, die sich darüber nicht abdecken lassen. Und manchmal führen diese zu besseren Ergebnissen.

Was soll denn eigentlich gemacht werden? Wir brauchen:

  1. Den Inhalt der Variablen count in Textform.
  2. Eine Form von „to be“ abhängig vom Numerus („is“ oder „are“).
  3. Keinen Plural-Modifizierer, sondern vielmehr eine deklinierte Form des Buchstabens in der Variablen candidate. Dass das Deklinieren im Plural ein „s“ anfügt, ist Implementierung, nicht das Ziel.

Also machen wir doch einfach mal Methoden, die genau das liefern, was wir brauchen:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
public class Statistics3
{
    enum Number {SINGULAR, PLURAL}

    public static void main(String...args)
    {
        Statistics3 statistics = new Statistics3();
        System.out.println(statistics.composeGuessStatistics('d', 0));
        System.out.println(statistics.composeGuessStatistics('d', 1));
        System.out.println(statistics.composeGuessStatistics('d', 25));
    }

    private String composeGuessStatistics(char candidate, int count)
    {
        Number number = requiresPluralForm(count) ? Number.PLURAL : Number.SINGULAR;
        return "There "
            + thirdFormOfToBe(number) + " "
            + countToString(count) + " "
            + declineLetter(candidate, number));
    }

    private boolean requiresPluralForm(int count)
    {
        return count != 1;
    }

    private String thirdFormOfToBe(Number number)
    {
        return number == Number.SINGULAR ? "is" : "are";
    }

    private String countToString(int count)
    {
        return count == 0 ? "no" : Integer.toString(count);
    }

    private String declineLetter(char letter, Number number)
    {
        return number == Number.SINGULAR ? Character.toString(letter) : letter + "s";
    }
}

Der Code ist einfacher, weil kein Zustand in irgendwelchen privaten Feldern gehalten werden muss (KISS). Weil es die Felder nicht mehr gibt, ist auch keine eigene Klasse mehr nötig (MIMC). Aber vor Allem: Jede Methode ist aus sich heraus verständlich (PSU). Die Implementierung von thirdFormOfToBe(Number number) ist verständlich ohne, dass man von dem ganzen Code drum herum irgend etwas wissen müsste. Ebenso mit den anderen privaten Methoden. Und die Methodenaufrufe sind auch so klar, dass man nicht das Gefühl hat, unbedingt die Implementierung lesen zu müssen. Ein Aufruf wie createPluralDependentMessageParts(count); schreit nach weiterer Erklärung. Ein Aufruf thirdFormOfToBe(number) ist so klar, dass man die Implementierung gar nicht sehen muss und normalerweise auch gar nicht versucht ist, sie zu lesen.

Das einzige, was einem deutschen Leser vielleicht etwas merkwürdig anmutet, ist das Wort „number“. Das bedeutet nämlich nicht nur Anzahl, sondern auch Numerus. Wenn das zu Verwirrung führen sollte, kann man immer noch GrammaticalNumber schreiben.

Generell bin ich jemand, der recht viel Wert auf PSU legt, während das Prinzip ansonsten — so mein Eindruck — ein wenig vernachlässigt wird. Vielleicht weil es lange Zeit keinen Namen hatte. Ich bin mal gespannt wie es mit Clean Code weiter geht. Meine Vermutung ist, dass ich noch ein paar Mal „PSU“ zu rufen versucht bin. Das schmälert den Wert des Buches zwar nicht wirklich. Ich finde den Punkt dennoch nicht ganz unwichtig.

Bei meinem Code oben fällt noch etwas auf: Der Code ist eigentlich funktional. In funktionaler Programmierung schreibt man genau so. Ohne Variablen, ohne Seiteneffekte. Der gleiche Code in Haskell sähe in etwa folgendermaßen aus:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
data Number = Singular | Plural

main = do
    print (composeGuessStatistics 'd' 0)
    print (composeGuessStatistics 'd' 1)
    print (composeGuessStatistics 'd' 25)

composeGuessStatistics candidate count =
    let
        number = if requiresPluralForm count then Plural else Singular
    in
        "There " ++ (thirdFormOfToBe number)
            ++ " " ++ (countToString count)
            ++ " " ++ (declineLetter candidate number)

requiresPluralForm 1 = False
requiresPluralForm _ = True

thirdFormOfToBe Singular = "is"
thirdFormOfToBe Plural = "are"

countToString 0 = "no"
countToString count = show count

declineLetter letter Singular = [letter]
declineLetter letter Plural = [letter] ++ "s"

Mit Pattern Matching ist das nochmal ein wenig schöner als mit dem Ternäroperator. Aber ich glaube, auch, wenn man kein Haskell kann, erkennt man die Parallele zum obigen Java-Code. Nun ist mir die Objektorientierung immer noch näher als das funktionale Paradigma. Dennoch sind manche funktionale Einflüsse und Denkweisen manchmal nicht schlecht. Als ich meinen Studenten in der Uni Haskell beigebracht hatte, gab es vereinzelt Stimmen, dass das ja nicht so praxisrelevant sei. Ich denke das obige Beispiel zeigt so ein bisschen das Gegenteil.

Aber kommen wir zurück zu Java. Was ist nun, wenn wir die Funktionalität trotzdem in eine separate Klasse auslagern wollten, obwohl wir ja nun die Felder alle losgeworden sind? Warum sollten wir so etwas tun wollen? MIMC ist hier wieder das passende Stichwort. Vielleicht wird die Klasse, in der die Methoden enthalten sind zu lang. Es gibt unterschiedliche Meinungen darüber, wann man eine neue Klasse machen sollte und wann nicht. Nach Grady Booch und wahrscheinlich auch nach Uncle Bob lieber früher als später (siehe Add more classes). Ich für meinen Teil mach das nicht ganz so schnell. Wenn ich zu viele Klassen hab, wird das auch bald unübersichtlich und es kostet Zeit, die richtige zu finden. Der Package Explorer zeigt dann bald eine zu lange Liste. Zu große Klassen sind aber natürlich auch nichts und so ist es manchmal ganz gut auch für so „Kleinkram“ Klassen zu machen. Das ist immer eine Abwägungssache zwischen MIMC/KISS), HC und LC.

Gesetzt also den Fall die enthaltene Klasse wird zu groß und die Bindung zu schwach. Dann wollen wir das in einer separaten Klasse haben. Diese sollte aber nach MP eine natürliche Klasse sein und nicht nur Overhead um eine Prozedur sein. Bei Version 2 von oben sieht man recht deutlich, dass man hier quasi Methoden instanziiert:

1
2
3
4
GuessStatisticsMessage statistics = new GuessStatisticsMessage();
System.out.println(statistics.make('d', 0));
System.out.println(statistics.make('d', 1));
System.out.println(statistics.make('d', 25));

GuessStatisticsMessage hat keinen beobachtbaren Zustand und nur eine öffentliche Methode. Es ist wie eine Funktion, die man zuerst instanziieren muss. Weiterhin gibt es einen internen Zustand, der zwischen den Aufrufen keinerlei Bedeutung hat. Sowas kann man als potenzielle Fehlerquelle sehen (ML). GuessStatisticsMessage ist keine Message, sondern eine Methode, die sich als Klasse tarnt.

Man kann aus Version 3 von oben aber auch eine echte Klasse machen. Das sähe dann folgendermaßen aus:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
public class Statistics4
{
    public static void main(String...args)
    {
        GuessStatisticsMessage[] statisticsMessages = {
            new GuessStatisticsMessage('d', 0),
            new GuessStatisticsMessage('d', 1),
            new GuessStatisticsMessage('d', 25)
        };

        for (GuessStatisticsMessage msg : statisticsMessages)
        {
            System.out.println(msg);
        }
    }

    static class GuessStatisticsMessage
    {
        enum Number {SINGULAR, PLURAL}

        private char candidate;
        private int count;

        public GuessStatisticsMessage(char candidate, int count)
        {
            this.candidate = candidate;
            this.count = count;
        }

        @Override
        public String toString()
        {
            Number number = requiresPluralForm(count) ? Number.PLURAL : Number.SINGULAR;
            return "There "
                + thirdFormOfToBe(number) + " "
                + countToString(count) + " "
                + declineLetter(candidate, number));
        }

        private boolean requiresPluralForm(int count)
        {
            return count != 1;
        }

        private String thirdFormOfToBe(Number number)
        {
            return number == Number.SINGULAR ? "is" : "are";
        }

        private String countToString(int count)
        {
            return count == 0 ? "no" : Integer.toString(count);
        }

        private String declineLetter(char letter, Number number)
        {
            return number == Number.SINGULAR ? Character.toString(letter) : letter + "s";
        }
    }
}

Jetzt ist GuessStatisticsMessage tatsächlich konzeptionell eine Message, dabei aber separat verständlich. Klar, die Klasse ist langweilig (wobei das bei Code meist eher ein gutes Zeichen ist), aber sie könnte in Zukunft ja um weitere Funktionalität wachsen. Also auch diese Version halte ich für gut. Jedenfalls für besser als Version 2 aus Clean Code.

Heißt das nun Uncle Bob ist blöd und schreibt keinen sauberen Code? Nein, natürlich nicht. Der Grund für den Code ist wohl ein recht trivialer. In dem Kapitel geht es um „Meaningful Names“. Das Beispiel sollte wohl nur ein Beispiel für eine ganz bestimmte Aussage sein. Eben die Aussage, dass man durch Einführung einer neuen Klasse, Variablennamen in einen bestimmten Kontext setzen kann und das Ganze dadurch lesbarer wird. Die Aussage ist auch richtig so. Nur ist es leider nicht ganz so leicht, wirklich gute Beispiele zu finden. Wer mal ein Tutorial geschrieben oder eine Schulung gehalten hat, weiß, wie schwer es ist, gute Beispiele zu finden. Das ist eines er schwersten Dinge dabei überhaupt. Hier hat das Beispiel gewisse Schwächen. Und da ich ganz gut die Klappe aufreißen kann, hab ich die Schwäche kurzerhand genutzt, um meine Prinzipiensprache vorzuführen und insbesondere PSU zu zeigen. Eigentlich müsste ich jetzt ein besseres Beispiel aus dem Hut zaubern. Aber mein Hut ist leer. Momentan habe ich leider kein tolles Beispiel, das sich hier eignen würde…

6 Kommentare


  1. Hallo Christian,

    countToString 1 = „1“

    Die Zeile ist m.E. überflüssig, ein kurzer Test auf GHCi zeigt:

    >let countToString count = show count
    >countToString 1
    „1“

    Gruss Marco


  2. Nachtrag: ich persönlich löse ähnliche Aufgabenstellungen im Alltag oft mit Dictionaries.
    Also z.B. in etwa so (von der Idee her, die Syntax mag in Java anders sein):

    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    dict[0] = (number = "no",verb = "are",pluralModifier="s");
             dict[1] = (number = "1",verb = "is",pluralModifier="");

             String guessMessage = "";
             if( count in dict )
             {
                var t = dict[count];
                guessMessage = String.format("There %s %s %s%s", t.verb, t.number, candidate, t.pluralModifier);
             }
             else
             {
                 guessMessage = String.format("There %s %s %s%s", "are", Integer.toString(count), candidate, "s");
             }
             System.out.println(guessMessage);

  3. Hallo Marco,

    der Haskell-Code ist korrigiert. Danke.

    Zu deiner Dictionary-Idee: Ja, sowas ist manchmal sinnvoll und macht Code schöner. Ich mach sowas auch manchmal. Hier im konkreten Fall gefällt mir das aber nicht. Das Problem mit der mangelnden separaten Verständlichkeit löst er nicht und durch die zusätzliche Datenstruktur wird es obendrein noch komplexer. Im Vergleich mit der ursprünglichen Version sparst du dir gerade mal ein „else if“. Dafür hast du eine komplexe Datenstruktur mehr (eine Map in nem Array) und das eh schon recht komplexe String.format ist jetzt sogar doppelt drin…

    Viele Grüße

    Christian


  4. Hallo Christian,
    du hast recht, die Dictionary-Methode ist hier „too much“. Dein Ansatz mit der Klasse gefällt mir allerdings auch nicht so richtig, denn auch eine solche Klasse scheint mir „too much“ für ein so einfaches Problem. Warum nicht einfach:

    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    15
    16
    17
    18
    19
    private string getGuessStatistics(char candidate, int count)
            {
                String guessMessage;
               
                if (count == 0)
                {
                    guessMessage = "There are no " + candidate + " 's";
                }
                else if (count == 1)
                {
                    guessMessage = "There is one " + candidate;
                }
                else
                {
                    guessMessage = "There are " + count + " " + candidate + " 's";
                }
                 
                return guessMessage;
            }

    Das ist zugegebenermassen nicht besonders einfallsreich. Ich bin der Meinung, die geringe Redundanz ist hier vertretbar, zugunsten der Lesbarkeit. Gruss und schöne Weihnachten, Marco


  5. Hallo Marco,

    sehr schön! Gefällt mir. Einfach, lesbar, intuitiv. Schön. Das ist Code, wo man sich sagt: „Ja klar. Keine Frage, so muss es sein.“

    Frohe Weihnachten!

    Christian


Schreibe einen Kommentar

Deine E-Mail-Adresse wird nicht veröffentlicht. Erforderliche Felder sind mit * markiert