DRY – Don’t repeat yourself.
Das kennt jeder, das _sollte_ jeder wissen. Und so gut wie jeder, der etwas Ahnung hat, versucht es so oft es geht umzusetzen.
Allerdings: Das gilt nicht nur für Funktionsnamen, das DRY-Prinzip geht weiter. Es meint IMHO auch, dass man für viele ähnliche Abfragen sich am besten Wrapper baut, die dann die Anfrage an eine Funktion leiten, die die eigentliche Antwort produziert. Die Wrapper „füttern“ die eigentliche Funktion nur mit den richtigen Fragen.
Ein Beispiel: Siggi Scriptkid hat ein Klasse zur Benutzerverwaltung geschrieben, die braucht er in seinem aktuellen Projekt. Er speichert seine Benutzerdaten in einer MySQL Datenbank und muss nun seiner Klasse folgende Fragen stellen und erwartet folgende Antworten:
- Hole mir den Benutzer mit der ID 5
- Hole mir den Benutzer mit der Mailadresse xyz
- (noch viele weitere, ähnliche Fragen mehr)
Siggi schreibt also ganz fleißig Funktionen in seine Klasse:
(Stark vereinfacht, es fehlen Filtern der Usereingaben, maskieren der SQL String und vieles mehr; der Code dient nur als Beispiel!)
class User
{
public function getUserByID($userid)
{
$ressource = mysql_query('SELECT vorname, name
FROM user
WHERE userid = '.$userid);
$user = mysql_fetch_assoc($ressource);
return $user;
}
public function getUserByMailadress($email)
{
$ressource = mysql_query('SELECT ID, mail, userlevel
FROM user
WHERE mail = "'.$email.'"');
$user = mysql_fetch_assoc($ressource);
return $user;
}
}
Nun trifft man auf folgendes Problem: Die Felder der jeweiligen Rückgabe sind unterschiedlich. Auch wenn Siggi nun hingeht, und in beiden Funktionen die Felder gleich setzt, ergeben sich Wartungsschwierigkeiten:
- Was ist, wenn noch mehr, ähnliche Funktionen hinzukommen? Dann bekommt Siggi wirklich Arbeit!
- Was ist, wenn Siggi der „user“ Tabelle noch mehr Felder hinzufügen muss und es 6 Funktionen gibt?
- Was ist, wenn Siggi auch nur eine der Funktionen vergisst?
Um es kurz zu machen: Hier greift das DRY-Prinzip NICHT! Der o.a. Code sollte einem Entwickler, der nicht Siggi Scriptkid heißen möchte, nicht passieren.
Aber wie wende ich das DRY-Prinzip nun an?
Okay, ich schreibe mal meinen Vorschlag dazu:
class User
{
public function getUserByID($userid)
{
return $this->getUser('ID='.$userid);
}
public function getUserByMailadress($email)
{
return $this->getUser('mail='.$email);
}
private function getUser($where)
{
$ressource = mysql_query('SELECT ID, vorname, name, mail, userlevel
FROM user
WHERE '.$where.'
LIMIT 1);
$user = mysql_fetch_assoc($ressource);
return $user;
}
}
Ich schaffe eine private Methode, die die Daten aus der Datenbank holt. Diese ist die einzige Methode, die das macht, somit habe ich nur eine Stelle, in der ich zukünftig Felder ändern muss, wenn sich welche ändern.
Die „alten“ Funktionen verbleiben als Wrapper-Funktionen und übergeben der privaten Funktion nur die richtigen Parameter, um ihre eigene Aufgabe erfüllen zu können. Dann leiten Sie Antwort einfach als eigene wieder zurück.
Auf diesem Wege verinfache ich die Pflege, bin in der Lage, meine alten Funktionen weiterhin nutzen zu können – mit dem Vorteil, dass nun alle Antworten einheitlich sind – und bin darüberhinaus in der Lage, sehr schnell verschiedene neue Funktionen zu schreiben, die mit neuen Fragestellungen klarkommen, z.B. Benutzer mit E-Mail x und Passwort y.
Was haltet ihr von dieser Vorgehensweise. Ich selbst halte sie für praktiabel und gut, was meint ihr? Gut? Schlecht? Gibt es bessere Vorgehensweisen?
Ist doch glatt mal einen Versuch wert 😉
Ne mal ernsthaft, sieht durchdacht und schön strukturiert aus. Da muss ich echt mal meine Sources durchwühlen 😉
Ist das nicht eine Form des Refactoring? Und mit Code Smells hat dies doch auch etwas zu tun, oder?
Die Idee, den Code nicht unnötig doppelt schreiben zu wollen/müssen ist ja schon mal sehr gut. Du könntest aber noch weitergehen und sagen, dass dein User-Objekt nur die Methoden load() und save() kennt und die eigentliche Arbeit durch eine weitere Klasse erledigt wird. Über Load lassen sich dann die Standards abfragen und die Datenquelle ist im Hintergrund unabhängig.
So wie es Daniel sagt, wäre es auch korrekt. Zudem sollten Coding-Standards wie _method bei private oder protected functions in den Code Einzug halten. So ist immer klar, dass die Methoden nicht von ausserhalb angesprochen werden können.
Die Idee des „Refactorings“ ist gut, aber noch nicht gut genug.
Nachtrag: wieso sollte auch die Class User mit der Datenbank sprechen? Eigentlich wäre User ein Handler und die SQL Geschichten für ein Backend…So schaffst du dir wieder Abhängigkeiten. 🙂
Also das Vorgeschlagene ist in jedem Fall eine Optimierung in Bezug auf Wartungsfreundlichkeit und Flexibilität, wenn man es mit dem Ursprungscode vergleicht.
In meinen Augen könnte man es aber noch etwas verbessern, indem man die Vorschläge von anonym nimmt, die Datenbankabfragen in eine Wrapperklasse auszulagern und zusätzlich den User nicht in einem assoziativen Array vorzuhält, sondern in einem Objekt, welches für jede Spalte in der Datenbank ein Attribut hat.
Zusätzlich sollte dann, die Erzeugung der Objekte in eine Factory gepackt werden, die das Resultset übergeben bekommt. Dieses kann man dann überprüfen, ob es alle benötigten Spalten enthält und andernfalls eine Exception werfen.
Dadurch besteht zwar weiterhin, dass Problem, dass man beim Ändern des Datenbankschemas vergisst, eine Methode anzupassen, jedoch fliegt einem dann beim Testen prompt die Exception um die Ohren.
Danke erstmal an alle Kommentatoren, da scheine ich ja einen kleinen Nerv getroffen zu haben 😉
Eure Ideen sind alle richtig und gut, allerdings versuche ich immer, die Beispiele möglichst nah am Thema und sehr, sehr einfach zu halten, so dass man das Prinzip versteht, ohne sich zusehr in irgendwelche genauen und verschachtelten Sachen zu verirren.
Und zum Thema „Userklasse ruft sql klasse auf, die dann die abfragen macht“ habe ich grade einen thematisch verwandten Artikel in der Entstehung 😉
Anstatt verschiedene Methoden zu machen in denen nur eine hardgecodete variable ändert, könntest du auch eine methode mit $key, $value erstellen. Und damit man auch weiss was für key es so gibt kann man ensprechende konstanten erstellen.
Im Endeffekt greifen alle Methoden auf eine einzige Meth. zurück! … im Endstadium ist nahezu überhaupt nix passiert, bis auf die Auslagerung des SQL Statements / Queries.
Ich verstehe nicht warum man dies überhaupt aufsplitten sollte?! … Meiner Meinung nach – entweder das Datenbankdesign so umstellen, dass die UserID nur noch nummerisch ist und dann entsprechend mit is_numeric() abgefangen wird… oder sonstige Verzweigungen innerhalb einer Methode prüfen.
Es würde theoretisch nix dagegen sprechen ne RegEx auf das „=“ zu machen und den vorderen Teilstring via Switch – Case zu verarbeiten (von Performance mal abgesehen). Obwohl es hierbei auch relativ sinnlos ist,… weil eine Funktion niemals gut sein kann, wenn man bei zwei unterschiedlichen Übergabeparametern … mit zwei unterschdl. Verarbeitungen rechnen muss 😉
Ich mein ne userID wird wohl unique und ein Index sein… und die Mail, im Rahmen der Abfrage, wahrscheinlich zumindest unique sein, sonst würdest du mehr Datensätze erhalten und die Methode wahrscheinlich „getUsers“ nennen ;]
Ansonsten einfach mal das DB-Design umdenken, … da lässt sich mit den View und mit etwas SQL Nerd-Foo…. schon noch was rausholen.
Sorry… und Gruß 😉
@Anonym: Sinn des ganzen ist, dass es nicht 5 Funktionen mit unterschiedlichen Rückgabefeldern gibt, sondern – im Prinzip – nur noch eine „echte“ Funktion, die auch Werte liefert.
Wenn du 5 Funktionen hast, die alle unterschiedliche Felder zurückliefern können, musst du großen Aufwand betreiben, um diese konsistent zu halten. Hast du nur eine Funktion, die Werte liefern kann, musst du auch nur eine Funktion pflegen.
Und das ist nur _eine_ Funktion, um einen User zu bekommen, dazu kamen noch Funktionen um Produkte, Kategorien, Warenkörbe, Seiten, sonstwasdaten zu erhalten. Der Aufwand multipliziert sich schnell und mit dieser Methode kann man das ganze sehr schön vereinfachen.
Das Beispiel erklärt Code Refactoring nach dem DRY Prinzip ganz gut.
Man sollte vielleicht Sicherheitshalber darauf hinweisen das einige dinge wi z.b. SQL Injection nicht berücksichtigt werden und es für die DB anbindung bessere Lösungen gibt.
Der Klassenname User ist irritant den ein Objekt der klasse User ist kein User Objekt sondern ein Objekt mit dem man abfragen auf die User Tabelle steuern kann, das finde ich etwas irritant.