Clean Code: Sauber ist anders

von Hubert Schmid vom 2012-09-30

Clean Code ist einerseits eine Initiative, die versucht der Lesbarkeit und Wartbarkeit von Code ein stärkeres Gewicht zu verleihen. Andererseits ist Clean Code auch der Titel eines äußerst ungewöhnlichen Buches, das sich im Grunde das gleiche Ziel gesetzt hat. Ungewöhnlich ist aus meiner Sicht die große Diskrepanz zwischen seiner breiten Beliebtheit und der tatsächlich erreichten Ziele. Denn obwohl viele Empfehlungen in die richtige Richtung gehen, so greifen sie in Summe doch zu kurz und verlieren aufgrund ihrer dogmatischen und übermäßigen Anwendung an Wirkung.

Besonders deutlich wird dies in den zahlreichen Code-Beispielen, die fast durchgehend meine Erwartungen an sauberen Code enttäuschen. An einem Beispiel möchte ich diese Kritikpunkte einmal exemplarisch herausstellen. Aus der großen Menge habe ich mich für die Klasse RowColumnPagePrinter aus Listing 10‑7 entschieden, da sie hinreichend einfach und von der Länge passend ist. Den Code gebe ich im Folgenden stückweise und vollständig wieder – einschließlich aller Kommentare, Zeilenumbrüche und Einrückungen. Beginnen werde ich allerdings mit der main-Methode, aus der heraus die Klasse verwendet wird.

public static void main(String[] args) { final int NUMBER_OF_PRIMES = 1000; int[] primes = PrimeGenerator.generate(NUMBER_OF_PRIMES); final int ROWS_PER_PAGE = 50; final int COLUMNS_PER_PAGE = 4; RowColumnPagePrinter tablePrinter = new RowColumnPagePrinter(ROWS_PER_PAGE, COLUMNS_PER_PAGE, "The First " + NUMBER_OF_PRIMES + " Prime Numbers"); tablePrinter.print(primes); }

Die Methode ruft zunächst eine statische Methode der Klasse PrimeGenerator auf, die die ersten N Primzahlen in einem aufsteigend sortierten Array zurückliefert. Anschließend werden die Primzahlen über eine Instanz der Klasse RowColumnPagePrinter ausgegeben.

Auffallend sind in dieser Methode zwei Dinge: Erstens ist die Schreibweise der lokalen Variablen ungewöhnlich und passt nicht zu den verbreiteten Code Conventions for the Java Programming Language. Denn dort wird diese Schreibweise nur für Klassenkonstanten verwendet – aus technischer Sicht beziehungsweise für universelle Konstanten aus fachlicher Sicht.

Der zweite Punkt betrifft die lokale Variable tablePrinter. Mir ist der Unterschied zwischen einem RowColumnPagePrinter und einem TablePrinter nicht klar – beziehungsweise was aus einem RowColumnPagePrinter einen TablePrinter macht. Soll die unterschiedliche Namensgebung etwas ausdrücken? Ist das ein Artefakt einer nur teilweise durchgeführten Refaktorisierung? Oder hat sich der Autor der Klasse einfach nichts dabei gedacht? Was auch immer der Grund dafür ist: Das Problem daran ist, dass die Namensgebung beim Leser mehr Fragen aufwirft als sie beantwortet – und das anscheinend ganz unnötig.

public class RowColumnPagePrinter { ... }

Am Rahmen der Klasse RowColumnPagePrinter ist nicht viel dran. Die interessante Frage ist: Was fehlt? Es gibt eine Kleinigkeit: Die meisten Klassen sind nicht für weitere Vererbung konzipiert und die Möglichkeit trotzdem von ihnen ableiten zu können schadet meist mehr als es nutzt. Daher ist in Java eine bewährte Praxis, alle Klassen, die nicht explizit für die Vererbung konzipiert wurden, mit dem Modifier final zu deklarieren.

Interessanter ist ein anderer Punkt: Der Entwickler hat sich vermutlich Gedanken dazu gemacht, welchen Sinn und Zweck diese Klasse hat, und welche konkreten oder abstrakten Objekte durch ihre Instanzen repräsentiert werden. Dann hat er sich einen geeigneten Namen für die Klasse überlegt. Wie gut der Name auch immer gewählt ist, er wird in der Regel nicht die ganze Information enthalten. In diesem Fall findet sich die Information im Fließtext wieder:

The RowColumnPagePrinter knows all about how to format a list of numbers into pages with a certain number of rows and columns. If the formatting of the output needed changing, then this is the class that would be affected.

Wäre es nicht schön diese Aussage als Kommentar an der Klasse zu haben, damit die Information an der Stelle steht, an der sie dem Leser hilft?

private int rowsPerPage; private int columnsPerPage; private int numbersPerPage; private String pageHeader; private PrintStream printStream;

Als nächstes kommen die Member-Variablen, wobei ich den PrintStream besser mal ignoriere. Dann bleiben drei unterschiedliche Punkte: Erstens sollten die Member-Variablen final deklariert sein, weil dadurch der Leser sofort sieht, dass die Instanzen keinen veränderlichen Zustand besitzen. Zweitens ist die Member-Variable numbersPerPage – wie sich gleich zeigen wird – berechnet und wird nur an einer Stelle verwendet. Wenn es trotzdem einen sinnvollen Grund gibt, den Wert in einer Member-Variablen zu halten, dann wäre es schön ihn auch zu erfahren. Und drittens der Bruch in der Namensgebung: rowsPerPage und columnsPerPage beschreiben die Struktur wohingegen numbersPerPage den Inhalt beschreibt. Sollte Letzteres nicht besser cellsPerPage heißen?

public RowColumnPagePrinter(int rowsPerPage, int columnsPerPage, String pageHeader) { this.rowsPerPage = rowsPerPage; this.columnsPerPage = columnsPerPage; this.pageHeader = pageHeader; numbersPerPage = rowsPerPage * columnsPerPage; printStream = System.out; }

Der Konstruktor initialisiert lediglich die Member-Variablen. Das sieht einfach aus, ist aber trotzdem nicht ganz sauber. Von Vorne: Die ersten beiden Parameter bieten ein Potential für falsche Verwendung, da beide den gleichen Typ haben und ihre Reihenfolge nicht intuitiv ist, da an den meisten anderen Stellen die horizontale vor der vertikalen Dimension angegeben wird (z.B. java.awt.Dimension). Der nächste Punkt betrifft den Umgang mit gleichbenannten Member-Variablen und Parametern. Besonders unangenehm fällt es in der Zeile numbersPerPage = rowsPerPage * columnsPerPage auf. Auf der linken Seite der Zuweisung steht eine Member-Variable, die im Gegensatz zu den Member-Variablen in den beiden vorhergehenden Zeilen nicht qualifiziert ist, wohingegen auf der rechten Seite zwei Parameter stehen, die gleichbenannte Member-Variablen verdecken. Der letzte Punkt betrifft das System.out, das an dieser Stelle willkürlich und fremd wirkt. Ich würde den Ausgabestrom entweder als Parameter des Konstruktors oder als Parameter der noch folgenden print-Methode sehen.

Man könnte an dieser Stelle noch die Einrückung und den Zeilenumbruch bemängeln, wodurch die Abgrenzung zum Rumpf des Konstruktors verloren geht. Das ist aber ein grundsätzliches Problem der Java Code Conventions und nicht dem Autor der Klasse anzulasten.

public void print(int data[]) { int pageNumber = 1; for (int firstIndexOnPage = 0; firstIndexOnPage < data.length; firstIndexOnPage += numbersPerPage) { int lastIndexOnPage = Math.min(firstIndexOnPage + numbersPerPage - 1, data.length - 1); printPageHeader(pageHeader, pageNumber); printPage(firstIndexOnPage, lastIndexOnPage, data); printStream.println("\f"); pageNumber++; } }

Mit der öffentlichen print-Methode wird es langsam interessanter.

private void printPage(int firstIndexOnPage, int lastIndexOnPage, int[] data) { int firstIndexOfLastRowOnPage = firstIndexOnPage + rowsPerPage - 1; for (int firstIndexInRow = firstIndexOnPage; firstIndexInRow <= firstIndexOfLastRowOnPage; firstIndexInRow++) { printRow(firstIndexInRow, lastIndexOnPage, data); printStream.println(""); } }

Und es geht munter weiter: Wieder wird ein Intervall beidseitig geschlossen statt halb-offen repräsentiert. Die Schleifeniteration ist merkwürdig, denn die Iteration über die Zeilen wäre wesentlich einfacher als die Iteration über den Index der ersten Zelle jeder Zeile. Die Methode gibt immer rowsPerPage Zeilen aus, auch wenn diese leer sind – ohne einen Hinweis zu geben, warum das so ist. Die Argumente von printRow sind kurios, denn es wird nicht der Endindex der auszugebenden Zeile sondern der Endindex der letzten Zeile übergeben. Und schließlich hätte man sich beim println das Argument auch einfach sparen können.

private void printRow(int firstIndexInRow, int lastIndexOnPage, int[] data) { for (int column = 0; column < columnsPerPage; column++) { int index = firstIndexInRow + column * rowsPerPage; if (index <= lastIndexOnPage) printStream.format("%10d", data[index]); } }

Und es wird nicht besser: if-Anweisungen ohne geschweifte Klammern sind in Java eher ungewöhnlich. Die Struktur des Codes gibt die möglichen Ablaufpfade nicht gut wieder. Die Methode gibt entgegen ihrem Namen nicht eine Zeile sondern nur den Inhalt einer Zeile aus. Und: Die Bedingung index <= lastIndexOnPage wirkt ein wenig willkürlich. Wäre index < data.length nicht viel naheliegender?

private void printPageHeader(String pageHeader, int pageNumber) { printStream.println(pageHeader + " --- Page " + pageNumber); printStream.println(""); }

Bald ist es geschafft: Die fest-kodierte Zeichenkette würde ich gern irgendwo anders sehen, und das unnötige Argument des zweiten println-Aufrufs hatten wir oben bereits. Interessanter finde ich mal wieder das Namensproblem: Wenn eine Methode printPageHeader einen Parameter pageHeader hat, dann würde ich erwarten, dass es sich dabei tatsächlich um den Seitenkopf handelt. So ist es aber nicht. Der Seitenkopf wird stattdessen dynamisch zusammengebaut. Besser wäre es den Parameter beispielsweise pageTitle zu nennen. Noch besser: Statt den Fragmenten den Seitenkopf mit Platzhaltern übergeben – beispielsweise in Form eines Formatstrings.

public void setOutput(PrintStream printStream) { this.printStream = printStream; }

Und zu guter Letzt noch ein Setter: Es handelt sich um die einzige Methode, die den Zustand ändert. Andernfalls wären alle Instanzen unveränderbar. Warum kann man gerade den Ausgabestrom ändern? Warum nicht die anderen Eigenschaften? Und warum kann man den Ausgabestrom nicht einfach im Konstruktor übergeben? Welche Gründe es auch immer dafür gibt, der Autor der Klasse wollte anscheinend nicht, dass wir sie erfahren.

Was bleibt abschließend zu sagen? Sicherlich bin ich in meinem Code-Review sehr kleinlich gewesen. Es gibt viele Punkte, über die man leicht hinwegsehen kann. Es sind aber auch einige gewichtigere Punkte dabei. Funktional tut der Code wohl was er soll. Nur: Sauber ist anders.