Um das Programm zunächst mit automatischen Tests abzusichern, habe ich einen Testgenerator testgen entwickelt, der nun aus Dateien wie der folgenden
TESTCASE
description:Swiss Eph, geocentric positions
planets:0-13,15-20,21,22,40,43,47
flags:2,258
dates:1.1.1000-31.12.2000,5,randomly # 5 random dates for each object
TESTCASE
description:Swiss Eph, helio/barycentric positions
planets:1-9,14-20,40,43,47
flags:10,266,16642
dates:1.1.1000-31.12.2000,5,randomly # 5 random dates for each object
eine binäre Datei mit einigen Hundert Referenzdaten berechnet - nämlich die Ergebnisse der swe_calc()-Aufrufe durch die unveränderte swedll32.dll (mit den durch die Fixture definierten einzelnen Aufrufen).
Für meine Kopie von sweph.c mit dem Namen test_calc_reduce.c, mit der ich testhalber die Refaktorisierung ausprobieren möchte, habe ich ein eigenes make-Target eingerichtet, an dessen Schlusspunkt der Aufruf der Tests steht (insgesamt sind es 585 Berechnungen verschiedener Objekte zu verschiedenen Zeiten und Koordinatensystemen). Im Fehlerfall weiss ich, dass meine letzten Änderungen etwas zerstört haben müssen. Die umgekehrte Garantie, dass bei Fehlerfreiheit auch alles wie früher funktioniert, habe ich zwar leider nicht. Aber das Streben nach einer 100%igen Testabdeckung würde mich zuviel Zeit kosten. Lieber füge ich in die obige Fixture noch ein paar weitere Testfälle ein, wenn ich auf einen bislang noch unentdeckten Fehler stossen sollte.
Was fällt auf bei der Betrachtung von sweph.c? Die Datei enthält 59 Funktionen bei 6241 Zeilen, also im Schnitt 105 Zeilen pro Funktion. Die Verteilung ist allerdings sehr ungleichförmig: Die Funktion swecalc() hat beispielsweise fast 600 Zeilen! Clean Code-Profis wie Robert C. Martin empfehlen Funktionsgrössen in der Grössenordnung von 10 Zeilen und darunter. Das sind Erfahrungswerte.
Wenn die Zeilenzahlen deutlich über 10 liegen, erschwert das die Lesbarkeit und das Verständnis des Programms. In der Regel sind alle möglichen Übel die Folge: Lokale Variablen werden für aufeinderfolgenden mehrfachen Gebrauch zweckentfremdet, die Verzweigungslogik wird nicht mehr durchschaubar - und, am unangenehmsten: identische Aufgaben werden mehrfach codiert (Verstoss gegen das elementare Prinzip Don't Repeat Yourself).
Die Funktionsnamen sind zum grossen Teil gut gewählt, man versteht, was die Funktion tun soll. Für die Variablen gilt das nur noch zum Teil, und fast vollends ins Unlesbare taucht man bei der Frage nach der Bedeutung der einzelnen Komponenten der Strukturen swe_data ab. Es wäre zu prüfen, welche Komponenten dieser Struktur überhaupt noch benötigt werden.
In sweph.c finde ich verschiedene Dinge, die mich zu Verbesserungen reizen:
- Micro-Wiederholungen, z.B. immer wieder das Kopieren von Vektoren mit einer Mini-for-Schleife,
- Wiederholungen kleinerer Codeblöcke,
- Immer wieder Entscheidungen anhand von Flagleisten, die die Lesbarkeit erschweren.
- Sprünge innerhalb von Funktionen mittels goto - es gibt insgesamt 97 goto's in sweph.c - die immer der deutliche Ruf nach Auslagerung von Code in eine eigene Funktion sind.
- Eine klarere Trennung nach Schichten täte dem Aufbau gut. Wenn man Aufrufe puffern möchte, sollte diese Pufferung möglichst "weit oben" erfolgen – oder "transversal"; jedenfalls so, dass der Teil, der die eigentliche, die ungepufferte Rechnung durchführt, nicht durch die Abfragen nach Pufferung verdunkelt wird.
- Konstanten täte es gut, wenn sie wirklich als const deklariert und somit dem Compiler bekanntgemacht werden. Das bringt eine Menge Vorteile. Das ist aber für den riesigen Haufen an #define's in diesem Projekt wohl nicht (mehr) praktikabel und auch nicht (mehr) sinnvoll. Für einzelne Konstanten und insbesondere für neu eingeführte Konstanten werde ich die Umwandlung jedoch vornehmen.
Es gibt verschiedene Präfixe wie swe_, se_, swi_. Für die Kenntlichmachung der externen Funktionen sehe ich die Nützlichkeit eines solchen Präfixes ein, das nach aussen, im Code des Aufrufers, wie ein "Namensraum" fungiert. Aber für interne Funktionsnamen hat es sich eingebürgert, keine besonderen Präfixe mehr zu verwenden. Dasselbe gilt für Variablennamen. Es empfehlen sich einfach sprechende, ruhig lange Funktionsnamen, die wie ein Kommentar ihres Inhalts funktionieren - ein Kommentar, der diese Funktion immer begleiten wird!
Die dateiinternen Funktionen static zu deklarieren, ist sicher eine gute Idee. Es ist das private des C-Programmierers!
Insgesamt habe ich beim Lesen den Eindruck: "So schwer kann das doch alles nicht sein!" Sicher, die Ephemeridenrechnung ist kein Deckchensticken. Aber auch komplizierteste Probleme lassen sich in kleine Teilaufgaben zerlegen, die jede für sich in klarer, nachvollziehbarer Weise hingeschrieben werden können. "Was man überhaupt sagen kann, das kann man auch klar sagen", sagt Wittgenstein. Das gilt auch für Programme.
Micro-Funktionen werden vom Compiler meist automatisch in den Code hineingeneriert, es entsteht also bei der Ausführung keine neue Stackebene. In C99, das ich verwende, kann man dem Compiler dies mit dem Qualifier inline nahelegen. Solche Funktionen erzeugen keinen Performance-Overhead. Um der Lesbarkeit des Code willens kann man daher ruhig grosszügig solche Funktionen einführen - das Programm wird dadurch um keine Nanosekunde langsamer und gewinnt in der Regel an Lesbarkeit.
Einige Kandidaten für solche Inline-Funktionen wären [1],[2]:
// Utility functions (might move to swephlib)
static inline void* vec_clear( void* x, int size) {
memset((void*)x,0,size*sizeof(double));
return x;
}
static inline void* vec_copy( void* to, void* from, int n) {
memcpy(to,from,n*sizeof(double));
return to;
}
static inline void* vec_add( void* to, void* from, int n) {
double *fto = (double *) to,
*ffrom = (double *) from;
for (int i=0;i<n;i++) fto[i] += ffrom[i];
return fto;
}
static inline void* smul( void* to, double s, int n) {
double *fto = (double *) to;
for (int i=0;i<n;i++) fto[i] *= s;
return fto;
}
static inline char* msg_append(char* serr, char* text) {
int maxLengthAppendable = AS_MAXCH - strlen(serr);
if (maxLengthAppendable > 0) {
if (strlen(text) >= maxLengthAppendable)
*(text+maxLengthAppendable-1) = '\0';
strcat(serr, text);
}
return serr;
}
static inline bool is_set( int iflag, int flags ) {
return iflag & flags;
}
static inline bool is_not_set( int iflag, int flags ) {
return ! (iflag & flags);
}
static inline void set( int* iflag, int flags ) {
(*iflag) |= flags;
}
static void set_ephemeris( int* iflag, int ephemeris) {
*iflag = ( (*iflag) & ~SEFLG_EPHMASK ) |
( ephemeris & SEFLG_EPHMASK )
;
}
[1] Das void* bei den Vektorfunktionen habe ich mit Bedacht gewählt, weil ich überlege, ob der Code an manchen Stellen nicht mit einer geschachtelten Struktur von 2*3 Doubles lesbarer würde, anstelle eines Double-Arrays, etwa so:
// Using named members instead of a double[6]
// might lead to more readable code in some cases
typedef struct {
double x;
double y;
double z;
} rect_coord;
typedef struct {
double l;
double b;
double r;
} polar_coord;
typedef struct {
rect_coord pos;
rect_coord speed;
} rect_data;
typedef struct {
polar_coord pos;
polar_coord speed;
} polar_data;
Ein void*-Argument für die Vektorfunktionen hat den Vorteil, dass man ebensogut auch einen Zeiger auf eine solche Struktur übergeben könnte. Das (compiler- und plattformspezifische) Alignment von Strukturen spuckt uns nicht in die Suppe, weil die Struktur aus lauter double's besteht. Das führt, soweit ich das erforscht habe, auf keiner Plattform zum Padding.
[2] Die Präfixe vec für die Vektorfunktionen widersprechen nicht dem oben über Präfixe Gesagten, denn es sind keine Präfixe aufgrund von generellen Namenskonventionen, sondern Teile des konkreten Namens, genau wie bei set_ephemeris()
Keine Kommentare:
Kommentar veröffentlichen