Schaltjahr: "sauberer" Code

[paedubucher]

An einer Programmiervorlesung, wo auch Codequalität thematisiert wird, wurde uns Studenten gestern eine Aufgabe gestellt. Es ging darum, eine Methode zu schreiben, die überprüft, ob ein gegebenes Jahr ein Schaltjahr ist, inkl. einiger JUnit-Tests. (Jedes vierte Jahr ist ein Schaltjahr, jedes hundertste jedoch keines, jedes vierhundertste jedoch schon.) Ich habe das ganz kurz und trocken so hingeschrieben:

public static boolean isLeap(int year) {
   return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);
}

Nun meinte der Dozent, dass dieser Code zwar richtig, aber nicht sehr gut lesbar sei. Er schlug vor, eine Methode "isDividableBy" hinzuzufügen:

public static boolean isLeap(int year) {
   return isDividableBy(4, year) && (!isDividableBy(100, year) || isDividableBy(400, year));
}
private static boolean isDividableBy(int what, int x) {
   return x % what == 0;
}

Um das ganze noch lesbarer zu machen, sollten die einzelnen Ergebnisse in boolean-Variablen abgespeichert werden. Leider sind wir nicht mehr dazu gekommen, alle Variablen zu benennen, der Dozent meinte nur, die erste könnte regularLeapYear heissen.

public static boolean isLeap(int year) {
   boolean regularLeapYear = isDividableBy(4, year);
   boolean hundredthYear = isDividableBy(100, year);
   boolean fourHundredthYear = isDividableBy(400, year);
   return regularLeapYear && (!hundredthYear || fourHundredthYear);
}
private static boolean isDividableBy(int what, int x) {
   return x % what == 0;
}

Funktionieren tut alles, die "verbesserte" Lösung überzeugt mich aber keineswegs. Das boolean regularLeapYear suggeriert ja schon, dass es sich um ein Schaltjahr handelt ‒ und um ein "reguläres" Schaltjahr obendrein. Die anderen beiden Variablen sind meines Erachtens schwer zu benennen. Darum würde ich mir die Variablen allesamt sparen, um nicht schwammige Begriffe einführen zu müssen. (Sollte mir jemand drei gute Namen für die boolean-Variablen nennen können, revidiere ich meine Meinung gerne.)
Weiter finde ich auch schon den ersten Schritt unnötig. Wenn ich Code lese und eine Modulo-Operation sehe, deren Ergebnis anschliessend mit 0 verglichen wird, weiss ich, dass auf eine restlose Division geprüft wird. Das Einführen der isDividableBy-Methode führt nur zu einem weiteren Problem: wie benenne ich die beiden Parameter, welcher Parameter wird wofür verwendet?
Grundsätzlich verstehe ich das Ideal, Code hinschreiben zu wollen, der sich wie Prosa liest. Doch warum soll ich Prosa schreiben, wenn eine kurze Formel den Sachverhalt präziser ausdrückt? Dazu kommen weitere Probleme: Sollte die Funktion isDividable eine Eingabe-Prüfung haben? Vielleicht wird die eines Tages jemand ausserhalb meines Kontextes wieder verwenden. Wie benenne ich all die zusätzlichen Variablen?
Vielleicht habe einfach schon zu viel mit C herumgespielt, um für Java-Kreise genehmen Code zu schreiben, mir scheint aber, dass die Verbesserung des Dozenten eher gut gemeint als gut gemacht war. (Bei manchen Programmierern habe ich manchmal den Eindruck, dass die lieber "add(2, 3)" als "2 + 3" hinschreiben würden.)

Ihr denkt euch vielleicht, dass diese ganze Analyse doch etwas heavy sei für so eine primitive Aufgabenstellung. Ich finde aber, dass solche Detailfragen oft grundsätzliche Meinungsverschiedenheiten offenbaren: z.B. die Definition von "sauberem" Code: Erhalte ich diesen durch zusätzliche Indirektion? Ist die ursprüngliche oder die "verbesserte" Lösung eleganter? (Von Performance soll hier mal nicht die Rede sein...)

Danke für eure Meinungen!

 

[Flown]

Vorerst - ich weiß, dass das eine Übung ist aber trotzdem - nimmt man immer eine Funktionalität aus der Standardbibliothek, wenn sie vorhanden ist. Das garantiert, dass viele Augen und Test über die Methode gefahren sind und man sich sicher sein kann, dass die Methode funktioniert. Einfach mit:

/* Seit Java 8 DateTime API */
java.time.Year.isLeap(int n);

Wenn man es selbst schreiben muss, dann unbedingt eine Dokumentation dazuliefern. Damit steckt man die Ein- und Ausgabeparameter ab, welche Werte man zu erwarten hat, bei gewissen Eingaben.

Man sollte immer die Eingaben prüfen, außer eine Methode ist als private deklariert, dann kann man sich auch die Dokumentation sparen, denn diese wird ja nur von der Klasse als Hilfsmethode verwendet und ist nicht für die Öffentlichkeit bestimmt.

"Sauberer" Code sieht, für die meisten professionellen Entwickler, so aus (Auszug aus der Java API):

/**
* Checks if the year is a leap year, according to the ISO proleptic
* calendar system rules.
* <p>
* This method applies the current rules for leap years across the whole time-line.
* In general, a year is a leap year if it is divisible by four without
* remainder. However, years divisible by 100, are not leap years, with
* the exception of years divisible by 400 which are.
* <p>
* For example, 1904 is a leap year it is divisible by 4.
* 1900 was not a leap year as it is divisible by 100, however 2000 was a
* leap year as it is divisible by 400.
* <p>
* The calculation is proleptic - applying the same rules into the far future and far past.
* This is historically inaccurate, but is correct for the ISO-8601 standard.
*
* @param year  the year to check
* @return true if the year is leap, false otherwise
*/
public static boolean isLeap(long year) {
    return ((year & 3) == 0) && ((year % 100) != 0 || (year % 400) == 0);
}

Ich bin immer der Meinung, dass man unnötige Variablen vermeiden sollte, wenn man es auch kurz und prägnant halten kann - wenn Dokumentiert wurde! Darum wüsste ich auch keinen Grund warum ich für den %-Operator einen Namen (isDivisible) erfinden sollte.

ABER HALT. Was du richtig gesehen hast ist, dass es Leute gibt die gerne Sachen mit Variablen versehen und Schritte Namen geben. Das ist auch völlig in Ordnung. Für manche ist das nur eine lästige Leserei, doch für Andere eine große Erleichterung zum Verstehen des Algorithmus.
Also wie du siehst, liegt es wieder am Geschmack der Individuen und du kannst nur Argumentieren, dass es für dich leichter ist, wenn keine störenden Variablen dazwischen sind.

PS: Für Jahrhundert könntest du die Variable: isCentury einführen. Für alle 400 Jahre habe ich leider keinen Namen für dich.

 

[paedubucher]

Vielen Dank für die Erläuterung! Die Java-Jungs machen das ja fast wie ich, mit der Ausnahme, dass % 4 mittels & 3 umgesetzt wird. Der Name isCentury scheint mir durchaus sinnvoll gewählt. Der andere Name könnte isFourthCentury sein.
Ich muss noch anfügen, dass ich die gleiche Frage auch im http://debianforum.de/forum/viewtopic.php?f=34&t=162892 Debianforum bespreche. Der Grundtenor dort deckt sich mit deiner Erläuterung.

 

[mrBrown]

Ich finde Variante zwei besser, Variante 3 würde ich nicht nehmen, Lesbarkeit gewinnt man mMn in dem Fall durch die drei zusätzlichen Variablen nicht wirklich.
Ein isDividableBy(4, 100) ist mMn deutlich Lesbarer als ein ((year &3)==0), auch wenn beides verständlich ist. Bei ersterem hat man auch zusätzliche Doku allein durch den Funktionsnamen.
Clean Code wurde ja im anderem Forum schon angesprochen, die Java-Libs sind leider kein gutes Beispiel dafür...

Bei dem Beispiel mag's überflüssig wirken, aber an so was kleinem kann man es immer am besten demonstrieren. Bei größeren sieht man dann schon eher, wenn es sinnvoll ist.

 

[stg]

Der Name isCentury scheint mir durchaus sinnvoll gewählt.

Ist er nicht. Ein Jahrhundert/Century ist ein Zeitspanne von hundert Jahren, kein Zeitpunkt. Du würdest ja auch nicht erwarten, dass isMonth eigentlich den ersten Tag eines Monats meint.

Zu deinem eigentlichen Thema: Frage 3 Leute und du erhälst 5 unterschiedliche Meinungen.
Mir ist es wichtig, dass ich sofort vestehe, was der Code macht. Notwendig dafür sind etwa kurze Methode, sprechende Bezeichner und eine saubere Struktur des Codes. Ich würde da deine Lösung der aus der Java API vorziehen, da man bei &3 vielleicht doch erst kurz überlegen muss, was da eigentlich passieren soll.

Eine Diskussion hier wird aber nicht wirklich zielführend sein. Ich denke das in diesem Zusammenhang Wichtigste ist ohnehin bei dir angekommen. Objektiv messen lässt sich solche nicht-funktionale Code-Qualität nicht wirklich. Wichtig ist eigentlich, dass man sich dieser Problematik stets bewusst ist und immer versucht "sauberen" Code zu schreiben.

Ich kann dir auch empfehlen "Clean Code" von Onkel Bob Martin zu lesen.