Programmierstil: Meinung/Verbesserungen?

[Kai345]

[latex]Mae  govannen![/latex]

Wer möchte:

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.

Nun interessieren mich Meinungen und Verbesserungen zum Code, damit ich meinen Stil verbessere und offensichtliche Anfängerfehler und unschöne Konstrukte vermeide. Ganz zufrieden bin ich selber noch nicht (insbesondere bezügl. Zusammenbau der Strings, durch die vielen zu beachtenden Bedingungen sehr zerstückelt) aber bisher weiß ich nicht, wie ich es besser machen soll. Aber ihr werdet mich schon zusammenstauchen ^^

Cü,

Kai

YouTube Video-Tipp: Harmonic Curves
YouTube Video-Tipp: Pipe Dreams
selfcode sh:( fo:| ch:? rl:( br:< n4:# ie:{ mo:| va:) js:) de:> zu:) fl:( ss:| ls:?

[dedlfix]

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";

[Blaubart]

Tach.

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

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

Nur ein kleiner Einwurf: empty("0") liefert auch TRUE.

[Kai345]

[latex]Mae  govannen![/latex]

Gleich vorab: Auf alles kann und will ich nicht eingehen.

Habe ich bei der Größe auch nicht erwartet. Jede Verbesserung hilft.

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.

Da die Klasse bisher immer wieder umgebaut wurde/wird, habe ich mich da bisher eher zurückgehalten. Die Kommentare sind daher bisher eher ein sehr grober Anhaltspunkt. Ok, für 'euch' ist das ungünstig, das sehe ich ein. Ich hatte aber beim Schreiben meines Beitrages auch eher allgemeine Fehler, die unabhängig von der tatsächlichen Funktion der Klasse ins Auge stechen, im Kopf. Einiges hast du diesbezüglich ja auch schon angesprochen.

Eine XSL-Transformation kommt nicht in Frage? Du hast ja XML vorliegen und willst sowas ähnliches auch wieder haben, da bietet sich sowas an.

Oh ja, viel (PHP)Arbeit hätte ich mir sparen können, wenn mein Hoster XSLT anbieten würde. Andererseits: Nur durch Versuche wie mit dieser Klasse lernt man schließlich auch etwas.

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.

Ich habe viel mit Copy&Paste (Nicht zu verwechseln mit Smith&Wesson :> ) gearbeitet, damit hielt sich die Arbeit in Grenzen.
Angefangen hatte ich mit verschachtelten assoziativen Arrays, aber irgendwas hat mich da gestört. Dummerweise weiß ich nicht mehr, was.

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

Ok, das ist eine dumme Angewohnheit aus einer anderen Sprache, wo man sich nicht immer aufs Aufräumen verlassen konnte.

$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.

Ich habe die Exception-Seiten auf php.net auch schon flüchtig angeschaut, bin aber bisher nicht nicht tiefer eingestiegen. Wieder mal: Ich habe mit PHP so viel zu lernen, daß mir eh schon der Schädel brummt.

/* 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 :-)

Man sollte vor Allem eine neue Tastatur besorgen, auf der dann auch noch mehr zu erkennen ist als Q Ü Ö Ä J Y X und V
(alle restlichen Buchstabentasten sind inzwischen blank) :(

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

[...]

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.

Ja, wie im Kommentar angemerkt noch experimentell, funktioniert noch nicht so recht, wie ich es will. Daran arbeite ich zur Zeit noch (d.h. doku lesen) Ich habe es z.B. bisher noch nicht geschafft, die XML-Zeile <irgendwas>a&#160;&amp;&#160;b</irgendwas> sowohl richtig anzeigen zu lassen wie auch valide (XHTML 1.0 strict). Bisher bin ich am & gescheitert. Aber wie gesagt, bin noch nicht so firm in PHP und gerade mit dieser ganzen Umwandelei noch etwas überfordert. Mit meiner etwas seltsamen Funktion habe ich zwar beide Ziele erreicht, aber daß sie so nicht bleiben kann, ist mir klar. Ich muß nur noch auf die richtige(n) Funktion(en) stossen und diese dann ggf. richtig kombinieren...

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.

In der angewendeten XML-Datei für mein Site-Menü sind zumindest Umlaute enthalten. Diese werden zur Zeit leider noch als Entities ausgegeben, aber im XML stehen sie als Umlaute. Ich habe im Vorfeld durchaus überlegt, ob ich zuerst den Beitrag verfasse, mit dem dieser Thread startet, oder einen Beitrag, der sich speziell auf das Problem mit Zeichenumwandlung aus dem XML heraus beschäftigt.

Aber ihr werdet mich schon zusammenstauchen ^^
Jo, das gibt es hier wie üblich gratis.

Das verläßliche Element an diesem Forum ;). So, jetzt mache ich mich mal an die Änderungen, soweit ich sie bereits umsetzen kann.
Danke.

Cü,

Kai

[dedlfix]

echo $begrüßung;

Ich habe es z.B. bisher noch nicht geschafft, die XML-Zeile <irgendwas>a&#160;&amp;&#160;b</irgendwas> sowohl richtig anzeigen zu lassen wie auch valide (XHTML 1.0 strict). Bisher bin ich am & gescheitert.

// Quelltext UTF-8-kodiert
  $xml = simplexml_load_string('<root><irgendwas>ä&#160;&amp;&#160;b</irgendwas></root>');
  var_dump((string)$xml->irgendwas);

Ergibt bei mir

string(8) "ä & b"

also einen ordentlich UTF-8-kodierten String mit je 2 Bytes für das ä und die NBSP, und je eins für & und b. HTML-gerecht kann das mit dem üblichen htmlspecialchars() umgewandelt werden.

echo "$verabschiedung $name";

[Kai345]

[latex]Mae  govannen![/latex]

also einen ordentlich UTF-8-kodierten String mit je 2 Bytes für das ä und die NBSP, und je eins für & und b. HTML-gerecht kann das mit dem üblichen htmlspecialchars() umgewandelt werden.

Danke, funktioniert. Verstehe ich wirklich nicht, ich bin mir ganz sicher, es mit htmlspecialchars erfolglos versucht zu haben. Vielleicht war es der Browsercache.

Cü,

Kai

[Auge]

Hallo

Danke, funktioniert. Verstehe ich wirklich nicht, ich bin mir ganz sicher, es mit htmlspecialchars erfolglos versucht zu haben. Vielleicht war es der Browsercache.

... der in Sachen PHP womit zu tun hat? ;-)

Tschö, Auge

[Christian Seiler]

Hallo,

Eine XSL-Transformation kommt nicht in Frage? Du hast ja XML vorliegen und willst sowas ähnliches auch wieder haben, da bietet sich sowas an.

Oh ja, viel (PHP)Arbeit hätte ich mir sparen können, wenn mein Hoster XSLT anbieten würde. Andererseits: Nur durch Versuche wie mit dieser Klasse lernt man schließlich auch etwas.

PHP hat auch eine XSLT-Erweiterung, die oftmals aktiviert ist, d.h. Du kannst die XSLT-Transformation von PHP aus anstoßen. Oder meintest Du das bereits damit?

Viele Grüße,
Christian

[Kai345]

[latex]Mae  govannen![/latex]

Oh ja, viel (PHP)Arbeit hätte ich mir sparen können, wenn mein Hoster XSLT anbieten würde. Andererseits: Nur durch Versuche wie mit dieser Klasse lernt man schließlich auch etwas.

PHP hat auch eine XSLT-Erweiterung, die oftmals aktiviert ist, d.h. Du kannst die XSLT-Transformation von PHP aus anstoßen. Oder meintest Du das bereits damit?

Ja. Ich habe es allerdings ehrlich gesagt nicht in einem Testszenario ausprobiert, weil bereits in der Ausgabe von phpinfo() nirgendwo einer der Strings xsl / xslt aufgetaucht ist. Mag sein, daß dennoch eine der installierten Erweiterungen diese Aufgabe ebenfalls leisten würde, aber dann kenne ich sie nicht.

Cü,

Kai

[Kai345]

[latex]Mae  govannen![/latex]

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.

<frust>
Ich habe meinen Code gerade speziell auf diese Art von Vergleichen hin abgeklopft und bin nicht unbedingt glücklich mit der durch eine spezifische Auswertung entstehenden Verwirrung. In dem obigen speziellen Fall habe ich nun null verwendet, aber teilweise komme ich mit den verschiedenen Rückgabearten nicht zurecht (was deren Sinn angeht). <grummel>Wieso zum Geier gibt z.B. simplexml_load_file() beim Mißerfolg false zurück? Es ist ein Objekt, da hat verdammt nochmal die Rückgabe null zu sein, wenn das Objekt nicht erzeugt werden konnte und nicht false. Das ist total widersinnig.</grummel> Es widerstrebt mir regelrecht, die Initialisierung mit private $xmlobj = false; durchzuführen, nur weil keine sinnvollen Rückgabewerte verwendet werden. Ich möchte jetzt eigentlich gar nicht mehr wissen, wie das bei anderen Funktionen aussieht...
</frust>

Cü,

Kai

[dedlfix]

echo $begrüßung;

Wieso zum Geier gibt z.B. simplexml_load_file() beim Mißerfolg false zurück? Es ist ein Objekt, da hat verdammt nochmal die Rückgabe null zu sein, wenn das Objekt nicht erzeugt werden konnte und nicht false. Das ist total widersinnig.

simplexml_load_file() ist nur eine einfache Funktion, kein Konstruktor, auch wenn manchmal ein Objekt zurückgegeben wird. Diese Funktion gibt im Fehlerfall genauso ein false zurück, wie das die anderen Funktionen von PHP auch machen. Du solltest immer im Hinterkopf behalten, dass PHP kein objektorientiertes System ist. OOP ist nur ein Zusatz. Deswegen verhält sich PHP in großen Teilen auch nicht wie andere von vornherein objektorintiert geplante Systeme.

Es widerstrebt mir regelrecht, die Initialisierung mit private $xmlobj = false; durchzuführen, nur weil keine sinnvollen Rückgabewerte verwendet werden. Ich möchte jetzt eigentlich gar nicht mehr wissen, wie das bei anderen Funktionen aussieht...

Bleib doch bei null als Zeichen für nicht initialisiert. Dann hast du drei Zustände: ein Objekt im Gutfall, false im Fehlerfall und null wenn die Initialiserung noch nicht abgeschlosse ist.

Vielleicht schaust du dir doch mal die Sache mit den Exceptions an. Im Normalfall tust du als Anwender der Klasse einfach so als ob du ein Objekt bekommst. Im Fehlerfall landest du im catch-Bereich und hast die Fehlerdaten in der Exception. Es existiert dann auch kein Rückgabewert, der dich verwirren kann. Da die Exception die Zuweisung zur Variablen unterbricht, gibt es da auch nichts auszuwerten.

$var = null;
  try {
    $var = FooInitializing();
    // weiter mit $var
  catch (FooException $ex) {
    log($ex->getMessage());
    // Ersatzprogramm für den Anwender
  }

echo "$verabschiedung $name";