eo

Developers Shame Day: You've Come a Long Way, Baby

Für heute, den 3. November, hat Cem Derin den Developers Shame Day (oder wie ich sage: DSDS ohne Superstar) angesetzt. An diesem Tag bekommen PHP-Programmierer die Gelegenheit, ein besonders gruseliges Stückchen Code aus der Quarantäne zu holen, um es zum gegenseitigen Amüsement und gewissermaßen als Warnung für kommende Generationen öffentlich im Internet vorzustellen. Klingt verlockend, oder?

Ich frage mich noch immer, was er mit diesem Aufruf zu implizieren versucht.

Kunstpause.

Na ja, jedenfalls hat diese völlig absurde und abwegige Idee in einem vernunft-verspottenden Ausbruch kollektiv-masochistischer Natur dennoch allgemeinen Anklang gefunden, sodass am heutigen Tage allerorts die Codebäume Stilblüten tragen dürften. Ist das schön.

Ich persönlich, der ich nach meiner QBasic-Zeit selbstredend nie wieder auch nur eine einzige Zeile schlechten Code verfasst habe, musste -- in aller Bescheidenheit -- die zurückliegenden Commits von mehreren Tagen durchgehen, um Kandidaten zu finden, die sich in dieses wogende novemberliche Blütenmeer hätten einreihen können. Vorstellen möchte ich allerdings keinen Code von vorgestern, sondern Teile einer Klasse, die laut Repository im Jahre 2005, laut Gefühl irgendwann in den späten 1930ern entstanden ist.

Die Aufgabe dieser Klasse mit dem vielsagenden Namen CParser besteht (natürlich) nicht im Parsen von C-Code, sondern im Umwandeln einer stark BBCode-lastigen Auszeichnunssprache in feinsäuberliches HTML-Markup. Das genaue Vorgehen der Klasse möchte ich dabei zum gegenwärtigen Zeitpunkt als den 700/0-Algorithmus beschreiben, wie in 700 Zeilen Code, gefühlte 0 Zeilen Kommentar.

Den schmutzigeren Details der Implementation kann ich allerdings einen kurzen Lichtblick in Form eines Eingabe/Ausgabe-Beispiels voranstellen.

Eingabe:

[h1]Hallo Welt[/h1]
 
Hier ist ein Absatz.[fn]
 
[fnt]Das hier ist eine Fußnote.[/fnt]
 
Hier ist noch ein [url=http://example.org]Absatz[/url].

Ausgabe:

<h1>Hallo Welt</h1>
 
<p>Hier ist ein Absatz.<a class="footnote" href="#fn0-1" title="Zu Fu&szlig;note 1 springen"><span class="hide">[</span>1<span class="hide">]</span></a></p>
 
<p>Hier ist noch ein <a href="http://example.org" title="Zur Seite &quot;http://example.org&quot; wechseln">Absatz</a>.</p>
 
<div class="footnotes">
  <ul>
 
    <li><a name="fn0-1" class="footnote" id="fn0-1"><span class="hide">[</span>1<span class="hide">]</span></a> Das hier ist eine Fußnote.</li>
  </ul>
</div>

Na, dagegen sehen die WordPresseseses dieser Welt doch alt aus, das haben wir schon viel schlechter gesehen. Die Parser-Klasse fügt selbsttätig <p>-Tags hinzu und verfügt über einen vorzüglichen Fußnoten-Mechanismus, der es gestattet, die Fußnotenposition [fn] an einer anderen Stelle als den tatsächlichen Fußnotentext [fnt] in den Code zu schreiben. Ich für meinen Teil bin von meinem früheren Selbst schwer beeindruckt.

Noch schwerer beeindruckt wäre ich jedoch von ebenjenem früheren Selbst, wenn die Geschichte auch für diejenigen Eingaben funktionieren würde, für die sie dann nämlich in der Tat nicht mehr funktioniert. Das sind leider so ziemlich alle. Folgen die Eingaben nicht einer genau ausbalancierten Reihenfolge oder ist die BBCode-Struktur gar fehlerhaft, läuft irgendwo in den 700 unkommentierten Codezeilen etwas schief und die erzeugte HTML-Ausgabe wird witzig und kunstvoll anzusehen, aber leider nicht richtig.

Daraus lassen sich drei wesentliche Versäumnisse ableiten.

  1. Es wurde versäumt, irgendeine Art von Fehlerbehandlung zu schreiben. Der Code enthält schlicht und ergreifend keine Logik dazu. Es gibt keine Exceptions, es gibt keine Rückgaben von Fehlerwerten. Sobald ein unerwarteter Zustand auftritt, passiert ein klar definiertes Irgendwas.
  2. Es wurde zudem versäumt, den Code sinnvoll zu strukturieren. Die Klasse CParser ist im Grunde ein God object, das nichts weiter tut, als prinzipiell prozeduralen Programmierstil in einem Objekt zu verpacken. Dieses Objekt dient dann strenggenommen nur als Namespace, nicht aber als Teil einer OOP-Hierarchie. Features wie die Fußnoten-Logik wurden nachträglich in den bestehenden Code gehackt und sind entsprechend schwach integriert und blähen die vorhandenen Methoden mit Spezialfällen auf, statt sich irgendeiner definierten Schnittstelle unterzuordnen.
  3. All das macht die Klasse nahezu untestbar, da einzelne Funktionen nicht losgelöst vom Ganzen betrachtet werden können. Einige Methoden sind zudem mit einer Länge von 80 oder sogar 130 Zeilen voller if-Konstrukte ein klarer Indikator dafür, wo Funktionalität in weitere Klassen abstrahiert werden müsste.

Diese drei, mit den bereits erwähnten fehlenden Kommentaren vier, Punkte sind ein sicherer Weg, jeden Code auf Designebene vor die Wand zu fahren, sobald ein gewisser Grad an Komplexität erreicht wird. Die eingesetzten Algorithmen können dabei noch so gut und fehlerfrei sein, irgendwann bricht das Code-Gebilde gewissermaßen unter seinem eigenen Gewicht zusammen, weil die Übersicht verloren geht und weil auftretende Bugs nur noch sehr schwer überhaupt zu lokalisieren, geschweige denn zu beheben sind.

Zur Verdeutlichung des Elends zeige ich beispielhaft die ParseEx-Methode, die den äußeren Wrapper des Markup-Parsers darstellt, in der Originalversion von 1930. Der restliche Code sieht ungefähr genauso aus.

/*
*/
function ParseEx($s) {
    $this->PrepareString($s);
    $ret = "";
    $i = 0;
 
    if ($s == "") return;
 
    $this->GetNextTag($s, $i, $j, $tag);
 
    // Add <p> if source contains no tags or does not start with outline tag
    if ((!($j == 0)) || ($j === FALSE) )$ret .= "\n\n<p>";
    else if ((!($j === FALSE)) && (array_search($this->GetTagName($tag), $this->m_outline_tags) === FALSE))
      $ret .= "\n\n<p>";
 
    while (!($j === FALSE)) {
        $tag_content = "";
        $has_content = FALSE;
        $previous_text = trim($this->FormatString(substr($s, $i, $j - $i)));
        $ret .= $this->FormatString(substr($s, $i, $j - $i));
        if ($this->GetIsTag($tag)) {
            if (!($this->GetIsClosingTag($tag))) {
 
                /*
                    Opening Tag
                */
 
                if (!(array_search($this->GetTagName($tag), $this->m_outline_tags) === FALSE)) {
 
                    // Add </p> if opening outline tag does not follow closing outline tag
                    if ((!($j == 0)) && ((array_search($previous_tag_name, $this->m_outline_tags) === FALSE)||(!($previous_text == ""))))
                    {
                        $ret = trim($ret);
                        $ret .= "</p>\n\n";
                    }
 
                    $tag_content = $this->GetTagContent($s, $j, $tag);
                    $has_content = TRUE;
                    $this->DebugAdd("Tag Content: $tag_content\n");
                }
                elseif ($this->GetTagName($tag) == "fnt") {
                    $tag_content = $this->GetTagContent($s, $j, $tag);
                    $has_content = TRUE;
                    $this->DebugAdd("Tag Content: $tag_content\n");
                }
                elseif (array_search($this->GetTagName($tag), $this->m_single_tags) === FALSE) {
                    $this->AddToStack($tag);
                }
 
                $ret .= $this->AddCode($tag, TRUE, $tag_content);
 
                if (($has_content) && (!($this->GetTagName($tag) == "fnt"))) {
                    /* Add Paragraph: Closing outline tag but not fnt */
 
                    // Get next tag
                    $this->GetNextTag($s, $j, $tag_pos, $next_tag, FALSE);
 
                    $next_tag_name = $this->GetTagName($next_tag);
 
                    if (!($tag_pos === FALSE))
                    {
                        $str_between = substr($s, $j, $tag_pos - $j);
                        $str_trimmed = ltrim($str_between);
 
                        if (  (!($str_trimmed == "")) || (array_search($next_tag_name, $this->m_outline_tags) === FALSE) )
                        {
                            $ret .= "\n\n<p>";
                            $j += strlen($str_between) - strlen($str_trimmed);
                        }
 
                    }
                    else
                    {
                        $str_trimmed = trim(substr($s, $j));
                        if (!($str_trimmed=="")) $ret .= "\n\n<p>";
                    }
                }
 
            } else {
                /*
                    Closing Tag (with closing tag behaviour)
                */
 
                $ret .= $this->AddCode($tag, FALSE);
                $this->RemoveFromStack($tag);
            }
            $previous_tag_name = $this->GetTagName($tag);
        }
        else
          $ret .= $this->FormatString($tag);
 
        if (!$has_content)
          $i = $j + strlen($tag);
        else
          $i = $j;
 
        $this->GetNextTag($s, $i, $j, $tag);
    }
 
    // Restlichen Text anfügen und unter Umständen den letzten Absatz schließen
 
    $str_end = substr($s, $i);
    if ((!($str_end == "")) && ($this->GetIsOutlineTag($previous_tag_name)))
    {
        $i = 0;
        while ((($str_end[$i] == "\n") || ($str_end[$i] == "\r")) && ($i < strlen($str_end)) )
          $i++;
        $str_end = substr($str_end, $i);
    }
 
    if (!trim(($str_end == "")))
    {
        $ret .= $this->FormatString($str_end);
        $ret = trim($ret);
        $ret .= "</p>\n\n";
    }
    else if (!$this->GetIsOutlineTag($previous_tag_name))
    {
        $ret = trim($ret);
        $ret .= "</p>\n\n";
    }
 
    /*
        BAD SOLUTION FOR PARAGRAPH PROBLEM
    */
    $ret = $this->SolveParagraphs($ret);
 
    return $ret;
}

Alles klar, oder?

Ein auf diese Weise organisch gewachsener Code widersetzt sich in aller Regel sehr erfolgreich dem Versuch des Zurechtstutzens und wuchert munter in alle Richtungen weiter. Da hilft meist nur noch die radikale Variante, den entsprechenden Programmteil völlig neu und hoffentlich durchdachter zu konzipieren.

Einige Hinweise dazu:

So, ich werde den Code jetzt in eine Lochkarte stanzen und ans Nixdorf-Museum schicken oder alternativ in den nächsten Ententeich werfen.