dedlfix: Programmierstil: Meinung/Verbesserungen?

Beitrag lesen

echo $begrüßung;

Ich habe noch ziemlich wenig Ahnung von sowohl PHP wie auch Klassen/OOP. Daher habe ich einfach mal eine Klasse geschrieben, mit der ich eine verschachtelte, unsortierte Liste rekursiv aus eine XML-Datei erzeugen lasse.

Gleich vorab: Auf alles kann und will ich nicht eingehen. Besonders wenig Lust habe ich immer, mich in fachliche Fragen einzuarbeiten, wenn mich das Thema nicht wirklich interessiert und, so wie in deinem Fall, zu wenig erläuternde Kommentare enthalten sind. Kommentare sollen erklären, was mit dem nächsten Codeabschnitt erreicht werden soll. Im Idealfall kann durch eine "Übersetzung" des Kommentars in Code auch ein fachfremder Programmierer die Richtigkeit des Codes beurteilen.

Eine XSL-Transformation kommt nicht in Frage? Du hast ja XML vorliegen und willst sowas ähnliches auch wieder haben, da bietet sich sowas an. Ansonsten erscheint es mir recht aufwendig, erst solch einen umfangreichen XML-Code zu erzeugen. Das ist alles Fleißarbeit beim Tippen. Ich hätte wohl eher eine Notation in einer PHP-Datenstruktur gewählt. Die kommt mit weniger Overhead-Zeichen aus und könnte dadurch auch verständlicher und weniger erschlagend wirken, wenn man sie erblickt.

class MENU {

In Großbuchstaben schreibt man eigentlich nur Konstantennamen. Ich hätte das als "Menu" geschrieben.

public $current_id = 'current_page'; /* ID für Hervorhebung der aktuellen Seite */

Für die Kommentierung von Klassen, deren Eigenschaften und Methodensignaturen hat sich die PHPDoc-Syntax bewährt. Es gibt IDEs, die diese Art der Kommentierung interpretieren und beim Verwenden des Codes ebenjene Kommentare anzeigen können. Das ist hilfreich, um nicht Dinge wie Reihenfolge und Bedeutung von Funktionsparametern nachschlagen zu müssen.

public function __construct($file = '', $fallback = '', $xmlstring = false) {
        if (!empty($fallback) && is_string($fallback)) {

Wenn Parameter wie $file und $fallback optional sind, dann ist es günstiger, ihnen Werte wie null zuzuweisen. Die kann man besser von Leerstrings unterscheiden, die möglicherweise durchaus gewollt sein können.

/* Aufräumen */
    public function __destruct() {
        $this->menu = $this->xmlobj = $this->fallback = '';
        unset ($this->menu, $this->xmlobj, $this->fallback, $this->err_code, $this->indent);
    }

Das ist komplett überflüssig. Variablen bereinigt PHP von selbst. Der Destruktor wäre sinnvoller angewendet, wenn externe Ressourcen geschlossen werden müssen.

private function openXML($file) {
        if (file_exists($file)) {
            return simplexml_load_file($file);
        }

Es gibt noch andere Situationen, in denen die Datei zwar existiert, aber doch nicht gelesen werden kann. Öffne sie einfach und reagiere auf dabei auftretende Fehler.

$this->set_ec(self::MENU_FILE_NOT_FOUND);

Exceptions wäre auch eine zu überlegende Alternative. Dann muss der anwendende Code nicht erst nachfragen, welcher Fehler aufgetreten ist, sondern kann sich diese Information gleich aus den Exception-Daten holen.

if ($this->xmlobj) {

Wenn ich sowas sehe (und das ist nur einer von vielen ähnlichen Konstrukten), denke ich zuerst, du willst da was boolsches auswerten. Es ist nicht ersichtlich, dass du stattdessen auf Leerstring mit der Bedeutung "noch nicht initialisiert" prüfst. Nimm doch lieber null und mach die Prüfung mit is_null() oder === null.

Ab jetzt überfliege ich den Code nur noch und greife mit ein paar Dinge raus, die mir dabei noch auffallen.

/* Vergleicht die aktuelle Seite mit dem gerade bearbeitetetn Menüpunkt. Wenn duese gleich sind, wird
       true zurückgegeben, sonst false */

Man sollte auch Kommentare korrekturlesen :-)

/* Experimentell - HTML-Enities und numerische Zeichencodes, die sich im XML befinden, umwandeln */

Wohin umwandeln, und warum?

private function content_convert($str) {

Auch der Name der Funktion ist nichtssagend.

return (!empty($str)) ? htmlentities(utf8_decode(html_entity_decode($str))) : '';

Mit dieser Zeile gehen dir zuerst die Nicht-ISO-8859-1-Zeichen verloren. Anschließend hast du einen Mischmasch aus den UTF-8-kodierten Daten aus dem XML und den nach ISO-8859-1 umgewandelten Entitys vorliegen, den du von UTF-8 nach ISO-8859-1 umzukodieren versuchst, um dann das was noch übrig geblieben ist, unnötigerweise in Entitys umzuwandeln.

Die ganze Schose bei leerem String abzublasen, bringt auch nicht wirklich Pluspunkte. Diese Prüfung tät ich weglassen.

Du berücksichtigst die charset-Parameter der Funktionen nicht. Außerdem solltest du berücksichtigen, dass fast alle XML-Tools UTF-8 haben wollen. Selbst wenn du ihnen was anderes vorsetzt und ihnen das so mitteilst, werden sie dir das Ergebnis als UTF-8 liefern (wollen). Manchmal kann man das auch nicht umkonfigurieren. Es ist jedoch immer eine gute Idee, auch mal Nicht-ASCII-Zeichen beim Testen zu verarbeiten. Solche vermisse ich in deiner testmenu.xml.

Ebenfalls vermisse ich die kontextgerechte Behandlung der Werte beim Einfügen in den HTML-Code. Füge doch auch noch ein paar <>"& in deine Testdaten ein.

Aber ihr werdet mich schon zusammenstauchen ^^

Jo, das gibt es hier wie üblich gratis.

echo "$verabschiedung $name";