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.

^?View Code PHP

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

PHPGangsta
03/11/2010 @ 10:36

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
- Norbert
  03/11/2010 @ 11:17
  
  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
mario
03/11/2010 @ 21:11

“Access modifier muss her, egal welcher”? Hmm, den Eindruck bekomm ich letztens öfter.
- Norbert
  03/11/2010 @ 21:21
  
  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.
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
Iggi
18/11/2010 @ 11:44

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.
- Norbert
  20/11/2010 @ 16:23
  
  Ü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.