Artikelformat

Developers Shame Day

Heute ist der 3.11. und laut dem php hacker ist heute der „Developers Shame Day„. Heute will ich daher einen kleinen Codeabschnitt mit kurzer Analyse zeigen, der nicht gerade der Hit ist.

Der folgende Code ist ca 7 Jahre alt. Ich hätte gerne einen noch älteren Code hervorgeholt, leider konnte ich keine Backups des entsprechenden Projekts finden. Der Code wäre dann ca 11 Jahre alt.

1
2
3
4
5
6
7
8
9
10
11
function isSpecialRight() {
    $query = "select special from benutzer where id='" . $this->user_id . "'";
    $erg = mysql_query($query, $this->DBLink);
    $out = false;
 
    while ($item = mysql_fetch_array($erg)) {
        $out = $item['special'];
    }
 
    return $out;
}

negativ:

  • Die Methode hat keinen Kommentar. Da der die Benamung auch nicht besonders ist, kann ich nicht mehr sagen, welches besondere Recht hier geprüft wird.
  • Weiter hat die Methode keinen Modifier. Man könnte sagen, dass die Methode eben public ist. Aus dem Zusammenhang heraus wäre private aber eher angebracht.
  • Sehr ungeschickt ist auch die While-Schleife zu benutzen. Die user_id sollte ein Primärschlüssel sein und von daher sowieso nur einmal existieren.
  • Die Datenbank-Kommunikation würde ich heute entweder per PDO oder mit einem ORM-System (wie bspw Doctrine) realisieren.

positiv:

  • Die Verbindung zur Datenbank wird aus einer Klassen-Variable bezogen. Und somit hat man eine Art Dependency Injection für Arme. Bei einem Unit Test könnte man diese Variable mit einem Mock überschreiben.
  • Der Rückgabe Wert wird sinnvoll initialisiert. Immerhin das passt schon mal 🙂

Sicher gibt es noch einige Kommentare und ich hoffe nicht schon alle Punkte vorweg genommen zu haben. Insgesamt finde ich den „developers shame day“ eine interessante Sache.

7 Kommentare

  1. Schön finde ich auch den deutschen Tabellennamen und den deutschen Variablennamen ($erg). Oder machst du das auch heute noch bei privaten Kleinprojekten, wo man nicht im Team arbeitet und davon ausgeht, es nie Open Source zu machen oder zu verkaufen? Kann ja sein, ist ja (rein technisch) auch nichts dran auszusetzen, die Mischung englisch/deutsch sieht nur komisch aus 😉

    Antworten
    • Ohja 🙂 ist natürlich auch ganz schick.

      Inzwischen nutze ich da eher englische Tabellen- und Variablennamen. Die Methoden natürlich auch. Es sei denn, es ist anders gewünscht 😉

    • Ich finde, dass es guter Stil ist einen Modifier anzugeben. Dadurch macht man sich als Entwickler Gedanken über den Code und der lesende Entwickler sieht, dass eine Methode (oder Attribute) nicht „durch Zufall“ public ist. Natürlich gibt es auch den Standpunkt nur Modifier anzugeben, wenn diese vom Standard abweichen.

  2. Philipp Bräutigam

    09/11/2010 @ 19:34

    Achja, sowas ist immer wieder lustig 😉 Wenn man sich ein paar Jahre alter Code wieder anguckt und nur denkt „Was hab ich da damals überhaupt gemacht?!!?“ 🙂

    Aber wenigstens, weiss man heute, was man früher hätte besser machen können 🙂

    Grüße und schöner Eintrag,
    Philipp

    Antworten
  3. Dem gesagten kann ich nicht zustimmen.

    Kritikpunkte müssten sein:
    1. Kein Kommentar
    2. Keine Semantik im Methodennamen
    3. Kein Parametersanitizing bei der Übergabe an MySQL
    4. $out = $item[’special‘]; <– liefert dank der while Schleife nur das Letzte Recht. Also ein in jeder Hinsicht schlechter Code

    Und das Traurige ist, dass 3. untergeht. PDO hin oder her. Somit diese Analyse also nurnoch mangelhaft genannt werden kann.

    Antworten
    • Über Punkt 3 kann man sich vortrefflich streiten. An welchem Punkt ein Parameter geprüft werden muss, ist nämlich durchaus diskutabel. Du meinst bevor er an die DB gesendet wird. Vielleicht wird $this->user_id zu einem früheren Zeitpunkt gecheckt, sodass dieser Check nicht bei jeder Übergabe an ein SQL-Statement durchgeführt werden muss. Das wäre dann eine saubere Art der Redundanzvermeidung.

Schreibe einen Kommentar

Pflichtfelder sind mit * markiert.