Marc Ermshaus’ avatar

Marc Ermshaus

Now

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

Published on 3 Nov 2010. Tagged with php.

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.