Ein Schnipsel
Ein Anfang eines CGI-Programms. Die eigentliche Aufgabe ist unbekannt, der Anfang könnte für viele Aufgaben verwendet werden.
Ausgangssituation
Probleme
Die Probleme mit dem Code sind zugegeben nicht vordergründig. Zunächst sind da ein paar dicke Pluspunkte:
- strict, warnings
- CGI.pm
- CGI::Carp - wirkungsvolles Debugging
- Config::IniFiles - korrekte Kapselung von Konfigurationsdaten
Trotzdem gibt es ein paar Ungereimtheiten, die teilweise daraus resultieren, dass wir nicht mehr Code sehen, aber auch ganz objektiv. Bleiben wir also erst einmal objektiv.
Zeile 12: Der Erfolg der Aktion wird nicht geprüft. Das Einlesen der Konfiguration aus einer Datei erfordert zwangsläufig das Öffnen dieser Datei. Damit haben wir eine kritische Aktion, die u.U. erfolglos bleiben kann und dieser Fall sollte berücksichtigt werden.
Zeile 12 nochmal: Die Verwendung eines relativen Pfades ('./setup.ini') in einer CGI-Umgebung ist unter diesen Bedingungen sehr bedenklich, denn das aktuelle Verzeichnis '.' ist im CGI-Kontext keineswegs immer das Verzeichnis, in dem das Programm liegt. Es könnte genausogut das Verzeichnis sein, in dem der Webserver arbeitet...
Zeilen 13 bis 19: Hier werden nach und nach die Konfigurationswerte eingelesen, dabei wird aber nicht sichergestellt, dass die Werte a) vorhanden und b) sinnvoll sind. Ein mögliches Sicherheitsrisiko.
Zeile 47: der Name step_misstake ist für eine Funktion äusserts bedenklich, und zwar aus folgendem Grund: Das Englsiche Wort, das gemeint ist, ist 'mistake'. Das stellt spätestens dann ein Problem dar, wenn ich in Zeile 2001 diese Routine aufrufen will und das intuitiv mit step_mistake( ... ), denn wer soll den damit rechnen, dass die Funktion anders heisst? Ich unterstelle hier einfach mal wohlwollend einen Typo :)
Zeile 54: Die Konstruktion qq~ ... ~ ist vor allem in der Script-Kiddie Fraktion beliebt geworden, weil der wesentlich geeignetere Konstrukt <<ENDE ... ENDE (perldoc perldata) etwas mehr sorgfalt erfordert (perldoc -q here.*document). Es spricht zwar nichts gegen die Verwendung von qq// (perldoc perlop), es ist aber oft ein Signal. An dieser Stelle ist es nämlich eher weniger einleuchtend.
Zeilen 54 bis 57: HTML 4 sollte schon drin sein! :-)
Ein bisschen weniger objektiv, aber doch erwähnenswert: ein bisschen Software-Entwicklungs-Pedanterie:
Zeilen 26 und 43: der Kommentar sub::decide bzw. sub::step_misstake ist aus Perliger Sicht irreführend, denn '::' impliziert eine gewisse package-Spezifikation, d.h. sub::decide ist in meinen Augnen keineswegs "Die Subroutine 'decide'" sondern "Das Symbol 'sub::decide', anscheinend eine Subroutine". Imho durchaus eine Überlegung wert.
Zeilen 35 bis 40: Die namen 'step_1' bis 'step_5' bzw. auch 'step_0' sind ein wenig nichtssagend, aber naja.
Ziele
Ziele sind einfach die Behebung der obigen Fehler.
Zusätzlich habe Ich mir die Mühe gemacht, ein paar Sachen einfach Perl-iger zu erledigen, wie z.B. das Einlesen der Konfiguration. Hier definiert ein Hash, welche Konfigurationswerte in welche Variablen fliessen sollen. Das eigentliche Lesen passiert dann in einer Schleife. Vorteil des Vorgehens ist, dass man jederzeit mit _sehr_ wenig Tipparbeit neue Schlüssel hinzufügen kann.
Die Funktion decide ist vereinfacht worden, indem einfach ein Array die Funktionen referenziert, die je nach der Zahl am Ende des Parameters aufgerufen werden sollen. So wird auch diese Stelle leicht erweiterbar und Perl-iger.
Auch step_mistake ist verbessert. Es spart sich überflüssige Variablen und generiert sauberes HTML via CGI.pm.
Zu Beginn ist der BEGIN Block eingefügt worden, damit das Programm frühzeitig in das Verzeichnis wechselt, in dem es tatsächlich liegt. Näheres zu FindBin natürlich in perldoc FindBin.