Die Klasse ist vor allem eines: zu groß. MMn sollte eine Klasse nach Möglichkeit nicht mehr als 200-300 Zeilen enthalten. Wie Yuuri schon angemerkt hat, bedient die Klasse zu viele verschiedene Funktionen. Ein bekanntes Prinzip der OOP ist das sog. Single Responsibility Principle, das besagt, das jede Klasse nur genau eine Verantwortlichkeit abdecken soll - und "Alles, was irgendwie mit Auth zu tun hat" ist halt keine sauber definierte Verantwortlichkeit
Ich würde also empfehlen, als erstes zu checken, welche Verantwortlichkeiten es gibt, und danach die Klasse aufsplitten.
Auch einige deiner Methoden sind extrem lang, z.B. switchandsetAttribute (~ 270 Zeilen), setSessionLoggedIn (~130 Zeilen) usw. Meine Faustregel hier ist: 10 Zeilen für eine Methode sind super, 20 sind ok, 30 schon viel, und sobald ich sie nicht mehr ohne Scrollen überblicken kann, ist sie definitiv zu lang. Um dem entgegenzuwirken, kannst du lange Methoden in mehrere kürzere aufteilen. Gib ihnen sprechende Namen, und du bekommst wesentlich verständlicheren Code.
In setSessionLoggedIn() könntest du z.B. die Berechnung von $hashexpiredate in eine eigene Methode auslagern, ebenso die Abfrage des Session-Hashes und das Updaten / Einfügen der neuen Session-Daten. Alle diese Methoden zusammen könntest du dann in eine eigene Klasse verschieben, und schon wäre ein erster Schritt zur Modularisierung deines Codes getan.
Ein Teil der Länge liegt auch daran, dass du es mMn bei den SQL-Abfragen mit dem Umbrechen übertreibst. Es muss nicht jeder Spaltenname, jeder Platzhalter in eine eigene Zeile.
Die Methode switchandsetAttribute() ist ein Fall, wo immer wieder der gleiche Code steht, nur mit anderen Attributnamen und mal is_int statt is_string. Das könntest du erheblich verkürzen. Ich würde z.B. ein Array mit allen Attributnamen und -typen anlegen:
PHP:
private $attributes = [
'UNameUserTable' => ['fieldName' => 'usertab', 'type' => 'string'],
'UNameCellUserId' => ['fieldName' => 'celluserid', 'type' => 'string'],
// usw.
];
Statt dieses riesigen switch-case-Blocks könntest du dann schreiben:
PHP:
if (!isset($this->attributes[$att])) {
return '- "'.$att.'" ist keine gueltige Eigenschaft';
}
$attribute = $this->attributes[$att];
$typeCheck = 'is_' . $attribute['type'];
if (!$typeCheck($value)) { // ja, das geht. bisschen hacky, aber hier mMn vertretbar :)
return '- Wert "'.$att.'" muss ein '.ucfirst($attribute['type']).' sein';
}
$fieldName = $attribute['fieldName'];
$this->$fieldName = $value;
Das ist ein Verfahren, das nicht nur in diesem konkreten Fall funktioniert, sondern allgemeingültig ist: Vermeide es, viele Beinahe-Kopien desselben Codes zu haben; stattdessen zieh den gemeinsamen Code heraus und mach die Teile, die verschieden sind, zu Parametern.
Hier würde ich allerdings in Frage stellen, ob es überhaupt sinnvoll ist, eine Methode wie setAttribute() zu haben. Ich rate davon ab, da sie mE mehrere Nachteile hat. Zum einen würde ich generell nicht alle Membervariablen einer Klasse von außen beschreibbar machen, sondern nur die, bei denen es tatsächlich sinnvoll ist (wann ändern sich im laufenden Betrieb schon die Namen von Datenbankspalten?). Zum zweiten hat diese Implementation den Nachteil, dass du beim Aufruf wissen musst, wie die Attribute genau heißen. Bei so vielen Attributen und bei Namen wie 'PwResetTokenExpireHours' und 'ActivationTokenExpireDays' vertippt oder vertut man sich leicht: War es jetzt "Pw" oder "Password"? "Days" oder "Hours"? Wenn du tatsächlich Setter haben willst, solltest du sie mMn für jede Membervariable einzeln schreiben. Dann kann dir deine IDE nämlich helfen, den richtigen zu finden, und dir Tippfehler anstreichen.
Der Empfehlung, eine vollwertige IDE zu benutzen, kann ich mich nur anschließen, auch der Ansicht, dass nichts über PhpStorm geht. Das kostet zwar Geld, aber vielleicht kommst du ja für eine kostenlose Studentenlizenz in Frage. Ich kenne jedenfalls keinen professionellen PHP-Entwickler, der etwas anderes benutzt (außer den paar Freaks, die mit selbst konfigurierten Vim/Emacs unterwegs sind
). Es ist einfach super mächtig, zeigt dir viele potentielle Fehler auf, hilft dir beim Refactoring und kann sogar deinen Code nach PSR-2 formatieren
Die Konvention, PHP-Dateien, die Klassen enthalten, mit .class.php zu benennen, kommt noch aus der Zeit von PHP 4, als Klassen noch nicht so weit verbreitet waren. Heutzutage ist es normal, dass alle PHP-Dateien Klassen enthalten, außer vllt. der index.php, die quasi den "Bootloader" für die eigentliche Applikation darstellt. Deswegen benutzt man heute .class.php normalerweise nicht mehr.
password_hash($passwort, PASSWORD_DEFAULT); reicht vollkommen aus. Damit sind deine Passwörter schon wesentlich sicherer als vermutlich in so manchem Großunternehmen
Bei deinem Beispiel zur Verschachtelungstiefe gibt es einen einfachen, aber wirkungsvollen Trick, mit dem du dir eine Ebene sparen kannst. Da der else-Block ja nur die Exception wirft, kannst du die if-Bedingung folgendermaßen umdrehen:
PHP:
if(!isset($this -> username)) { // wir behandeln zuerst den Ausnahmefall
throw new UserAuthExceptions ('Zum Pruefen des Benutzernamens muss ein Benutzername gesetzt werden. Methode setUsername($username) benutzen');
}
// wenn das if zugeschlagen hat, kommen wir hier gar nicht mehr an, brauchen also kein else und damit auch keine neue Verschachtelungsebene
try{
// ...
}
catch (PDOException $e){
// ...
}
Das gleiche könntest du noch an einigen anderen Stellen machen.
Du benutzt häufig 0/1 als Wahrheitswerte statt true/false. Technisch geht das natürlich, aber empfehlen würde ich trotzdem letzteres - es macht den Code noch verständlicher. (Nebenbei bemerkt: In PHP schreibt man true, false und null standardmäßig klein.)
Inkludieren musst du gar nichts, wenn du wie von Yuuri empfohlen mit Autoloading arbeitest. Das musst du nur einmal einrichten, und dann hast du überall alle Klassen zur Verfügung. Sobald du mal Code von Anderen einbinden willst, wirst du
Composer benutzen wollen; dieses (extrem nützliche und wichtige) Tool kann dir auch einen Autoloader für deine eigenen Klassen bauen. Vorher solltest du dich mit Namespaces vertraut machen (solltest du aber sowieso tun).
Mit welcher PHP-Version arbeitest du? Die aktuelle ist 7.1. Falls du 5.6 oder gar eine noch ältere hast, würde ich dir empfehlen, auf 7 zu wechseln, da hier eindeutig die Zukunft von PHP liegt.
Falls du dich noch weiter in die Webentwicklung mit PHP vertiefen willst, würde ich dir mittelfristig
Symfony ans Herz legen. Das ist ein Framework, das dir viele Dinge schon mitbringt, die du brauchst, und dir auch dabei hilft, deinen Code zu strukturieren. Klar kann es spannend und manchmal sogar lehrreich sein, alles von Grund auf selbst zu schreiben, aber den praktischen Nutzen halte ich doch für begrenzt, da heute die meisten realen PHP-Anwendungen auf einem solchen Framework aufbauen. Symfony mag auf den ersten Blick schrecklich komplex aussehen, und es hat so einige Features, an die man sich als Anfänger erst einmal gewöhnen muss - Dependency Injection zum Beispiel. Aber es lohnt sich mMn, sich damit anzufreunden, denn diese Features sind keine Eigenarten dieses Frameworks, sondern weit verbreitete Standardpraktiken. Falls du mal beruflich PHP entwickeln willst, dann sind Symfony-Kenntnisse auf jeden Fall ein Vorteil.
Einige der Empfehlungen des werten Kollegen Yuuri möchte ich auch noch kommentieren:
- Upper camel case für Membervariablen kenne ich persönlich nur aus dem Microsoft-/.NET-Umfeld. Ist extrem Geschmackssache, ich z.B. finde es gar nicht schön. In PHP ist lowerCamelCase der Standard.
- Alles private zu machen ist super! Vererbung ist wohl der - gerade bei Anfängern - am meisten überbewertete Aspekt der OOP. Versuch so weit wie sinnvoll ohne Vererbung auszukommen. Faustregel: Was private ist, kannst du gefahrlos ändern, ohne dass außerhalb der Klasse etwas kaputtgehen kann. Bei public und protected ist das nicht der Fall.
- Von Active Record möchte ich dringend abraten. Das Pattern ist mMn eine "Noobfalle", das guten Programmierprinzipien (Trennung von Geschäfts- und Persistenzlogik) total im Weg steht. Bei einfachen Programmen merkt man das noch nicht so, aber wenn es komplexer wird und man mit Unit Testing anfangen will, bringt Active Record einen in Teufels Küche. Wenn du dein SQL nicht mehr selber schreiben willst, schau dir Doctrine an - das ist auch bei Symfony standardmäßig dabei.
- Heredocs sind nice, ich möchte aber noch eine weitere Möglichkeit in den Raum werfen, Strings mit Parametern zu versehen: sprintf(). Das führt mMn in einigen Fällen zu noch besser lesbarem Code.
In diesem Fall würde ich persönlich sogar noch einen Schritt weiter gehen und die Namen der Datenbanktabellen und -spalten hart in die Queries hineinschreiben, ohne den Umweg über Membervariablen. Dadurch wird das Schreiben und Lesen der Queries vereinfacht, und der Code wird durch das Wegfallen der Membervariablen auch ein gutes Stück kürzer. Außerdem kann eine gute IDE (PhpStorm auf jeden Fall) auch deine Datenbank einbinden und dann die Tabellen- und Spaltennamen checken und highlighten.
Das soll es für heute abend gewesen sein
