Clean Code und das Modellprinzip

1

Drei Monate nach meinem letzten Post zu Clean Code hier nun wieder einer. Und wieder grab ich in den Krümeln und tu so als könnt ich alles besser. Heute behaupte ich mal, ich könnte einen besseren Parser für Kommandozeilenargumente schreiben.

In Kapitel 14 „Successive Refinement“ zeigt Bob Martin sehr detailliert (auf über 50 Seiten) wie man Code schrittweise refactort. Das Kapitel ist wirklich, wirklich lesenswert, weil es die einzelnen Schritte beschreibt und zeigt, wie sich der Code verändert. Das Schwere oder eher: das, was man beim Refactoring nämlich üben muss, ist, wie man alles so kleinschrittig macht, dass die Tests immer durchlaufen. Schnell passiert es, dass man sein System an diversen Stellen ändert und es dann erstmal für Stunden nicht funktioniert, vielleicht nichtmal durch den Compiler geht. Die Kunst ist es nun, die Schritte so zu wählen, dass eben genau das nicht passiert. Mir passiert es immer noch, dass ich Code kaputtändere und ihn dann erst wieder zusammenflicken muss. Dieses Kapitel hat mir geholfen, dass das nun seltener passiert, wenngleich noch ein wenig mehr üben muss.

Aber ich wollte ja mal wieder ein wenig besserwisserisch auftreten und an Uncle Bobs Code rumkritteln. Wenngleich ich bewundere, wie der Mann refactort; das Endergebnis gefällt mir nicht so. Gucken wir uns erstmal an, wie seine Musterlösung denn aussieht:

Die Anforderungen

Aufgabe ist, eine Klasse zu schreiben, die die Kommandozeilenargumente entgehen nimmt und so parst, dass man sie bequem abfragen kann. Dazu kann man spezifizieren, welche Argumente erwartet werden und welchen Typ diese haben. Übergibt man nun eine Argumentliste einer parse()-Methode, kann man die Werte abfragen. Beispiel:

Eine Argumentliste könnte folgendermaßen aussehen:

1
-abc -p test -q 42

a, b und c sind Boolean-Parameter die gesetzt (also true) sind. Gäbe es einen weiteren spezifizierten Boolean-Parameter d, wäre der nicht gesetzt. p ist ein String-Parameter mit dem Wert „test“ und q ein Integer-Parameter mit dem Wert 42. Die Reihenfolge der Argumente spielt keine Rolle und die Klasse soll es ermöglichen, die Werte über einen einzelnen Methodenaufruf ab zu fragen. Unbekannte Argumente, überzählige Parameter, falsche Typen, etc. sollen moniert werden.

Die Lösung aus „Clean Code“

Im Folgenden sehen wir uns auszugsweise die Lösung aus dem Buch an. Meine Kritik folgt jeweils im Anschluss. Der (fast) vollständige Code ist im Buch S. 194ff. zu finden.

Schnittstelle

Zur Benutzung gibt Martin folgendes Codebeispiel zu Veranschaulichung:

1
2
3
4
5
6
7
8
9
10
11
public static void main(String[] args) {
    try {
        Args arg = new Args("l,p#,d*", args);
        boolean logging = arg.getBoolean('l');
        int port = arg.getInt('p');
        String directory = arg.getString('d');
        executeApplication(logging, port, directory);
    } catch (ArgsException e) {
        System.out.printf("Argument error: %s\n", e.errorMessage());
    }
}

Schnittstelle — Kritik

  • PSU: Der erste Parameter des Konstruktors ist die „Schema-Definition“, d.h. die Definition welche Parameter welchen Typs erwartet werden. Das mag zwar schön kurz sein, aber sonderlich intuitiv finde ich das nicht. Dass die Raute (‚#‘) angibt, dass es sich bei p um einen Integer-Parameter handelt, wird nicht klar, ohne, dass man es nachguckt. Zumal man ja nicht gerade täglich Kommandozeilenargumente parst. Ich hab ja mitunter schon Probleme, mir die Formatstrings vom printf()/String.format()/etc. zu merken. Und sowas sieht man ja wirklich alle Nase lang. Zumindest im Vergleich zu dem hier. Ich finde das also schlecht. Uncle Bob ignoriert hier eine seiner eigenen Regeln: „Avoid Encodings“.
  • Mit Hilfe von Generics könnte man das Abfragen von Werten noch etwas schöner machen. Der Code wurde aber wohl vor deren Einführung in Java geschrieben, also will ich das nicht ankreiden.
  • PLS: Normalerweise will man Kommandozeilenargumente nur einmal parsen, weil sich die Parameter ja während der Laufzeit nicht ändern. Trotzdem finde ich es tendenziell unschön, dass der Konstruktor hier quasi die Arbeit macht. Aber das kann man durchaus auch anders sehen.

Implementierung

Sehen wir uns als nächstes die Implementierung an. Die grobe Struktur ist recht einfach. Es gibt eine Args-Klasse, die eine Liste von ArgumentMarshalers hat. Letztere hat wiederum verschiedene Implementierungen:

Robert C. Martins Implementierung eines Parsers for Kommandozeilenargumente
Robert C. Martins Implementierung eines Parsers for Kommandozeilenargumente

Im Code sieht das 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
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
public class Args {
    private Map<Character, ArgumentMarshaler> marshalers;
    private Set<Character> argsFound;
    private ListIterator<String> currentArgument;
    ...
    public Args(String schema, String[] args) throws ArgsException {
        ...
        parseSchema(schema);
        parseArgumentStrings(Arrays.asList(args));
    }
    ...
    private void parseSchemaElement(String element) throws ArgsException {
        char elementId = element.charAt(0);
        String elementTail = element.substring(1);
        ...
        else if (elementTail.equals("*"))
            marshalers.put(elementId, new StringArgumentMarshaler());
        ...
    }

    ...

    private void parseArgumentStrings(List<String> argsList) throws ArgsException {
        for (currentArgument = argsList.listIterator(); currentArgument.hasNext; ) {
            String argString = currenbtArgument.next();
            if (argString.startsWith("-")) {
                parseArgumentCharacters(argString.substring(1));
            } else {
                currentArgument.previous(); // (*)
                break;
            }
        }
    }

    ...

    private void parseArgumentCharacter(char argChar) throws ArgsException {
        ArgumentMarshaler m = marshalers.get(argChar);
        ...
        argsFound.add(argChar);
        ...
        m.set(currentArgument);
        ...
    }

    ...

    public String getString(char arg) {
        return StringArgumentMarshaler.getValue(marshalers.get(arg);
    }

    ...
}
1
2
3
public interface ArgumentMarshaler {
    void set(Iterator<String> currentArgument) throws ArgsException;
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
public class StringArgumentMarshaler implements ArgumentMarshaler {
    private String stringValue = "";

    public void set(Iterator<String> currentArgument) throws ArgsException {
        try {
            stringValue = currentArgument.next();
        } catch (NoSuchElementException e) {
            throw new ArgsException(MISSING_STREING);
        }
    }

    public static String getValue(ArgumentMarshaler am) {
        if (am != null && am instanceof StringArgumentMarshaler)
            return ((StringArgumentMarshaler) am).stringValue;
        else
            return "";
    }
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
public class ArgsException extends Exception {
    private char errorArgumentId = '\0';
    private String errorParameter = null;
    private errorCode = OK;

    public String errorMessage() {
        switch (errorCode) {
            case OK:
                return "TILT: Should not get here.";
            case ...
        }
    }

    public enum ErrorCode {
        OK, ...
    }
}

Der fast vollständige Code findet sich in Clean Code auf den Seiten 194 bis 200.

Implementierung — Kritik

  • MP: Gleich der Hauptkritikpunkt vorweg: Diese „Marshaler“ sind nur eine Gruppierung von Code. Unter einem „ArgumentMarshaler“ kann man sich nichts vorstellen und das, was man sich vielleicht vorstellt, ist zudem noch falsch. to marshal bedeutet so viel wie „ordnen“ oder „aufstellen“. Der Begriff wird meist ähnlich gebraucht wie „serialisieren“. Manche sehen das als synonym an, andere sehen subtile Unterschiede. Fakt ist jedenfalls: Der ArgumentMarshaller hat damit rein gar nichts zu tun. Es ist eine Gruppierung von Code, der sich je nach Argumenttyp unterscheidet. Eine wie auch immer geartete Abstraktion stellt er aber nicht dar. Klassen sollten aber eigentlich Konzepte repräsentieren, und nicht nur Code gruppieren.
  • KISS: Das zweite Problem betrifft auch wieder diese Marshaler. Die Interaktion zwischen Args und Marshaler ist recht komplex. Es wird eine statische Methode aufgerufen, die eine Instanz der Klasse entgegen nimmt und castet? Ehrlich? Eklig! Mal ganz davon abgesehen, dass Casts und instanceof eh Zeichen dafür sind, dass etwas nicht stimmt, ist das alles unnötig kompliziert.
  • HC: Args macht zwei Dinge: Zum einen parst es das Schema und zum zweiten parst es die Argumente. Das sind zwei getrennte Aufgaben, die man eigentlich durch getrennte Klassen realisieren könnte. Ist hier aber nicht so. Die Bindung ist in diesem Punkt schwach.
  • IAP: Daten über die geparsten Argumente werden an zwei Stellen gepflegt: Einmal in den Marshallern und zum zweiten in argsFound. Diese beiden Datentöpfe müssen immer konsistent zueinander sein. Sind sie es nicht, hat man blöde, ggf. schwer zu findende Bugs eingebaut.
  • ML: Ein Iterator, der nicht methoden-lokal ist, sondern private und zudem noch an andere Klassen weitergereicht wird. So ein Iterator ist methoden-lokal ja eine ziemlich tolle Sache. Reicht man ihn aber weiter, hat man viel Potenzial, dass etwas schief läuft, weil der Iterator nachdem man ihn einmal weggegeben hat, vielleicht doch einen anderen Zustand hat, als erwartet. Der Code verlässt sich sogar darauf, dass der Iterator an diversen Stellen vor- oder zurückgesetzt wird. Ganz merkwürdig wird es an der Stelle, die ich oben mit (*) markiert habe. Dort wird der Iterator um eine Stelle zurück gesetzt. Und es ist ziemlich schwer zu verstehen warum. Das hat Auswirkungen auf eine öffentliche Methode, deren Sinn ich momentan nicht verstehe (sie gibt currentArgument.nextIndex() zurück. Vielleicht wird das auch gebraucht um den StringArrayArgumentMarshaler zu bauen, der im Buch nicht abgedruckt ist. Wenn man sich darauf verlässt, dass irgendwo aus nicht direkt ersichtlich Gründen, ein Iterator auf bestimmte Weise manipuliert wird, ist das eher keine gute Idee. Es gibt viel zu viel Fehlerpotenzial.
  • UP, PLS: ArgsException ist eine merkwürdige Exception-Klasse. Normalerweise verwendet man entweder Error-Codes oder Exceptions. Diese Klasse tut beides. Das ist ungewöhnlich. Normalerweise würde man spezialisierte Exception-Klassen ableiten. Warum das hier nicht getan wird? Ich hab keine Ahnung.

Meine Lösung

Ich hab mich dann mal daran gemacht, das besser zu machen. Und das ist dabei rausgekommen:

Schnittstelle

Das erste, was ich mir überlegt hab, ist ein Konzept für die Parameter. Damit meine ich nicht, dass ich mir das komplette Design vorher überlegt hätte. Nein, die Klasse ist konsequent Testgetrieben entwickelt. Vielmehr hab ich mir Gedanken über die Fachlichkeit gemacht: Mir ist aufgefallen, dass es einen fundamentalen Unterschied zwischen zwei Arten von Argumenten gibt. Zum einen gibt es Boolean-Argumente oder „Optionen“. Diese erwarten keinen separaten Wert als nachfolgendes Argument, sondern sind true, wenn sie gesetzt sind, und false, wenn sie nicht auftauchen. Zudem lassen sie sich auch einfach hintereinander notieren also „-abc“ für „-a -b -c“. Zum zweiten gibt es Parameter, die Werte unterschiedlichen Typs erwarten: Strings, Integer, etc. Für mich waren also Option und Parameter zwei fachliche Konzepte, die ich dann nach und nach in Code gegossen habe.

Zur Definition der Parameter hab ich auch nicht dieses merkwürdige Schema benutzt. Vielmehr hab ich einen Builder geschrieben, der so etwas erlaubt:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
public static void main(String... args) {
    try {
        ArgumentParser arg = new ArgumentParserBuilder()
                .withOption('l')
                .andIntegerParameter('p')
                .andStringParameter('d').build();
        arg.parse(args);
        Boolean logging = arg.isOptionSet('l');
        Integer port = arg.getParameter('p');
        String directory = arg.getParameter('d');
        executeApplication(logging, port, directory);
    } catch (ArgumentParsingException e) {
        System.out.println(e.getMessage());
    }
}

Das ist verständlich ohne, dass man irgendetwas auswendig lernen oder nachgucken muss.

Implementierung

Rein strukturell ist meine Implementierung komplexer. Es gibt mehr Klassen (MIMC). Allerdings hat jede ihre klar definierte und — zumindest in meinen Augen — intuitiv verständliche Aufgabe (MP, PSU).

Meine implementierung eines eines Parsers for Kommandozeilenargumente
Meine implementierung eines eines Parsers for Kommandozeilenargumente

Die oben beschriebene fachliche Trennung zwischen Optionen und Parametern spiegelt sich direkt im Design wider:

Meine implementierung eines eines Parsers for Kommandozeilenargumente
Vererbungshierarchie

Im Code sieht das in etwa so aus:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
public class ArgumentParser {
    private final ArgumentList arguments = new ArgumentList();
    ...
    public void parse(String[] args) throws ArgumentParsingException {
        List<String> yetToParse = asList(args);
        while (!yetToParse.isEmpty()) {
            int sizeBefore = yetToParse.size();
            yetToParse = arguments.parse(yetToParse);
            if (!sizeHasChanged(yetToParse, sizeBefore)) {
                if (looksLikeOptions(yetToParse.get(0)))
                    throw new UnknownArgumentException(yetToParse.get(0).charAt(1));
                else
                    throw new UnexpectedArgumentException(yetToParse.get(0));
            }
        }
    }

Das ist so ziemlich die komplizierteste Stelle im gesamten Code. Aber man kann sie verstehen ohne den Rest des Codes ansehen zu müssen (PSU). Was passiert hier?

  • Es gibt eine Liste yetToParse, die — wer hätts gedacht — die Liste der noch zu parsenden Argumente darstellt.
  • Solange wir also noch nicht mit dem Parsen fertig sind, parsen wir die Liste. Das Parsen machen wir nicht selber. Das kann die Liste für uns übernehmen (TdA/IE).
  • Die parse-Methode gibt die Liste der Argumente zurück, die noch nicht fertig geparst wurden. Wenn diese Liste leer ist, sind wir fertig. Wenn sie sich nicht mehr ändert, konnte offensichtlich etwas nicht verarbeitet werden.

Das ist eigentlich schon das ganze Geheimnis. Aber sehen wir uns doch mal die parse-Methode an. Ich hatte ja versprochen, dass es nicht mehr komplizierter wird. Stimmt das?

1
2
3
4
5
6
7
8
public List<String> parse(List<String> yetToParse) throws ArgumentParsingException {
    Argument<T> parameter = getArgumentByListToParse(yetToParse);
    if (parameter == null)
        return yetToParse;
   
    yetToParse = parameter.parse(yetToParse);
    return parse(yetToParse);
}
  • Wir suchen das Argument, das die übergebene Liste verarbeiten kann.
  • Wenn wir nix finden, können wir nix tun.
  • Ansonsten lassen wir das Argument parsen und machen rekursiv mit dem Rest weiter.

Argument sieht so aus:

1
2
3
4
5
6
7
8
9
interface Argument<T> {
    boolean canHandle(List<String> yetToParse);
   
    List<String> parse(List<String> yetToParse) throws ArgumentParsingException;
   
    char getName();
   
    T getValue();
}

Man kann also

  • Ein Argument fragen, ob es eine(n Teil einer) Liste parsen kann.
  • Die Liste parsen lassen
  • Das Argument nach dem Namen fragen.
  • Nach dem Parsen das Argument nach dem Wert fragen.

Das Interface ist aus sich heraus verständlich ohne den Rest des Codes zu kennen (PSU). Und es ist ein abgeschlossenes, verständliches, an der Fachlichkeit orientiertes Konzept (MP).

Insgesamt hat meine Code 409 Zeilen. Die größte Klasse ist 88 Zeilen lang. Das ist nicht länger als die Implementierung aus dem Buch. Ja, ich habe mehr Klassen und eine tiefere Vererbungshierarchie (MIMC). Ja, ich verwende Rekursion (KISS). Insgesamt finde ich meine Lösung aber einfacher und verständlicher.

Der gesamte Code findet sich auf Guthub.

Und wenn sich was ändert?

Ein riesiger Vorteil von Code, der sich am Modellprinzip orientiert: Zukünftige Erweiterungen lassen sich mit höherer Wahrscheinlichkeit ohne allzu große Änderungen bewerkstelligen. Wenn Anforderungen hinzu kommen, ändern diese die Fachlichkeit meist nicht grundlegend. Es ist wahrscheinlicher, dass die bestehenden fachlichen Konzepte im Großen und Ganzen gleich bleiben und nur neue hinzu kommen, als dass sich auf einmal etwas Grundlegendes ändert. Wenn sich jetzt die Codestruktur an der Fachlichkeit orientiert, können höchstwahrscheinlich große Teile des Codes einfach bestehen bleiben. Wenn aber die Codestruktur stark von den fachlichen Konzepten abweicht, kann das schnell anders aussehen.

Kommandozeilenargumente bilden hier keine Ausnahme. Im Grunde genommen, werden sie auch in Zukunft ziemlich ähnlich funktionieren. Überlegen wir uns mal, was so die wahrscheinlichsten zukünftigen Erweiterungen sein werden:

  • Weitere Argument-Typen könnten hinzu kommen.
  • Höchstwahrscheinlich wird man irgendwann auch Langformen der Argumente unterstützen wollen. Also nicht nur -a, sondern auch --all, etc.
  • Typische Argumentparser dieser Sorte bieten auch die Möglichkeit, automatisch eine kleine Hilfe zu generieren, die alle Parameter auflistet und eine dazugehörige Beschreibung anzeigt.

Was würden diese Anforderungen für die beiden Implementierungen bedeuten? Weitere Argument-Typen sind für beide Implementierungen kein sonderliches Problem. Wenn es mit fünf Typen geklappt hat, wird es mit dem sechsten und siebten auch funktionieren.

Die anderen beiden Anforderungen sind aber kniffliger. Das Schema-Konzept der Clean-Code-Implementierung wird entweder abartig hässlich oder es muss etwas Anderem weichen. Der Builder in meiner Implementierung aber würde ein paar mehr Methoden kriegen, mehr nicht. Beide Anforderungen betreffen den gesamten Code. Leider gibt es kaum eine Klasse, die man vollständig in Ruhe lassen könnte. Aber das ist bei beiden Implementierungen so. Bob Martins Lösung wird dahingehend Probleme mit den langen Parametern haben, als dass die Map, die Buchstaben auf Marshaler abbildet, so nicht mehr funktionieren wird. Da müsste ein vollständig neuer Mechanismus her. In meinem Fall sind die Änderungen hingegen nicht nur einfach, sondern intuitiv klar:

  • Fachliche Anforderung: Argumente haben jetzt nicht nur kurze, sondern auch lange Namen. Änderung: Die Argument-Klasse kriegt ein neues Feld „longName“.
  • Fachliche Anforderung: Zu jedem Argument soll eine Beschreibung erfasst und angezeigt werden können. Die Argument-Klasse kriegt ein neues Feld „description“.

Natürlich braucht der Builder neue Methoden und auch der ArgumentParser muss erweitert werden, etc. Im Großen und Ganzen handelt es sich aber um Erweiterungen. Es gibt ein bisschen neuen Code. Bestehender kann so bleiben. Schön ist vor allem, dass im Gegensatz zur anderen Lösung intuitiv klar ist, was zu tun ist. Noch schöner wär nur noch, wenn gar keine bestehenden Klassen angefasst werden müssten und man die Änderungen rein über neue Klassen realisieren könnte. Naja, man kann nicht alles haben. Insgesamt bin ich aber ziemlich zufrieden.

Ausblick

Beitrag und Code sind jetzt über Wochen und Monate gewachsen. Immer mal so nebenher ein bisschen. Ich hoffe, dadurch ist nichts durcheinander geraten. Sagt ggf. Bescheid. Kommentare Gegenmeinungen, Verbesserungsvorschläge und Kritik sind wie immer willkommen.

Auch das wird vermutlich nicht der letzte Blogeintrag zu Clean Code sein. Das Buch hab ich zwar schon seit einiger Zeit fertig gelesen, aber mit dem Bloggen komm ich nicht ganz hinterher. Ob es für den nächsten Post wieder drei Monate braucht, weiß ich nicht. Wir werden sehen…

Ein Kommentar


  1. Schöner Beitrag!

Schreibe einen Kommentar

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