Benötige eine Bewertung meiner Klasse

**mermshaus** · 21-03-2017, 23:57

Ich formuliere die Punkte relativ direkt, aber es sind natürlich nur Empfehlungen.

Formales:

Es gibt keinen wirklichen Grund, den Code so zu schreiben, dass er nur ASCII-Zeichen enthaelt. In Code-Kommentaren mag es ja „egal“ sein, etwa „ae“ statt „ä“ zu nutzen, aber bei Strings, die sichtbare Ausgaben enthalten ("$resmailsubject = 'Zuruecksetzen des Passworts'"), ist es nicht sinnvoll. „Verbreitete“ Unicode-Zeichen (äöü, ß, €-Symbol, …) solltest du direkt als UTF-8-Zeichen in den Code schreiben ("ä"). Bei esoterischen Sachen wie chinesischen Schriftzeichen oder so, die vielleicht mal im Code auftreten, dann lieber eine Escape-Sequenz nutzen und dem Leser per Kommentar mitteilen, was das für ein Zeichen ist. (`$char = "\xE2\x98\x83"; // U+2603 SNOWMAN`) Die Idee ist, dass Editor-Fonts Probleme haben könnten, die Zeichen darzustellen.
Schlüsselwörter konsequent kleinschreiben. „Class“ (Zeile 11), „Switch“ (403, 1410). Gilt tendenziell eigentlich auch für null, true und false.
Abschließendes ?> am Dateiende weglassen. Die Datei aber mit einer Leerzeile (bzw. einem Zeilenumbruch) enden lassen.
Zeilenumbrüche im Code sollten meiner Meinung nach immer LF sein, nicht CRLF.
„String“ als Datentyp (etwa in „@var String“) würde ich ebenfalls kleinschreiben.
„@access private“ braucht es eigentlich nicht, wenn das private-Schlüsselwort genutzt wird, aber mag sein, dass es Tools hilft.
Tabs richtig nutzen oder auf Spaces umbauen. Schlecht: http://i.imgur.com/S39nhGo.png Besser: http://i.imgur.com/H1IoF7q.png
Editor Whitespace am Zeilenende beim Speichern entfernen lassen.
Nur eine Klasse pro Datei deklarieren. (Hier nicht so relevant.)
Vielleicht verbreiteten Coding-Standard nutzen. Im PHP-Bereich etwa: PSR-1: Basic Coding Standard - PHP-FIG, PSR-2: Coding Style Guide - PHP-FIG -- Wenn jemand eine Tabweiten-Einstellung von 8 hat, ist er auch bei so was http://i.imgur.com/o88krpz.png dann bei dem if schon 24 Zeichen weiter rechts vom switch. Das ist vielleicht etwas extrem.
Wegen Lesbarkeit besser nicht $celltokenexpire, sondern $cellTokenExpire oder $cell_token_expire.

Einige Tippfehler:

10: UserAuthExceptions → …Exception
153: $tokenlenght → $tokenlength
308: $emailadress → …address (aber nur im Englischen mit doppeltem d)
361: „Atriibutname“
…

Allgemeine Tipps/Beobachtungen:

Nach Möglichkeit keine schwer aufzufindenden externen Abhängigkeiten schaffen.
- private $pepper = PEPPER; (126) -- PEPPER ist anderswo deklariert. Heißt das nicht eigentlich SALT?
270: `@var ObjectReferenz`. Das ist `@var PDO`.
340: `public function __construct($dbconnection){` Type-Hint auf PDO nutzen: `public function __construct(PDO $dbconnection){`
342ff.: Weiß spontan nicht, ob eine `if(!isset($_SESSION))`-Prüfung sinnvoll ist. Ich weiß auch nicht, ob es die Zuständigkeit der Klasse sein muss, gegebenenfalls die Session zu starten.
359: `@param String_oder_Array` schreibt man als: `@param string|array`
Bei Datenbanken ist für Feld/Spalte eher nicht Zelle (cell) gebräuchlich, sondern eben Feld oder Spalte (field oder column).
Die Setter (setUsername, setPassword, …) stellen nicht die Korrektheit der Eingabe sicher. Prüfen, ob es ein String ist und dergleichen.
doRegister: Nach dem ersten if keinen else-Zweig aufmachen. Durch die Exception springt der Code sowieso raus. (Wenn keine Exception, dann ruhig auch mal ein frühes return nutzen. Stichwort: Guard Clause.) Das verschachtelt aktuell den nachfolgenden Code nur unnötig.
786: `if ($errorflag == 0){` -- Auch hier ruhig gleich rausspringen und für den Folgecode Verschachtelung sparen.
800ff.: Vielleicht prinzipiell ein wenig extrem, die Feldnamen konfigurierbar zu machen. Tabellenname ist natürlich okay. Die Spalten, die du da per `$addtosqlqueryindex` dranpappst, vielleicht in eine zweite Tabelle schreiben? Und vielleicht auch nicht als Spalten, sondern eher als Key-Value-Paare. (INSERT INTO user_data (user_id, key, value) ...) Aber nur als Überlegung. So wirklich kann ich es spontan nicht beurteilen.
checkUsername: Durchführen von Queries (inklusive Fehlerbehandlung) vielleicht ruhig als private Methode rausabstrahieren/zentralisieren und aus einer Methode wie checkUsername nur mit Parametern aufrufen. Weil die Methode kurz ist, stelle ich sie exemplarisch mal schnell etwas um: http://i.imgur.com/q5RPo3t.png -- $this->query würde dann im Fehlerfall eben die Exception werfen.
Noch einige Anmerkungen zu checkUsername: Das SELECT * ist sinnfrei. Eigentlich willst du inhaltlich auch was mit SELECT COUNT(*). Die Methode ist nicht maximal verständlich benannt. Spitzenlösung habe ich gerade keine, aber mehr was in der Richtung von `doesUsernameExist`, damit man gleich versteht, was true und false als Rückgaben meinen. Ich weiß nicht, ob es elegant ist, mit einer Instanzvariablen zu arbeiten, statt den Username als Parameter zu übergeben. Das ist aber eine allgemeine Geschichte, was die Unterteilung von Zuständigkeiten im Code angeht. Dazu komme ich hoffentlich später noch.

Der Check, ob der Username gesetzt ist, ist eigentlich auch nicht toll. Der sollte nicht notwendig sein. Besser wäre es an der Stelle von der Idee her, wenn die Instanz durchsetzen würde, dass immer ein Username gesetzt ist, dass sie sich also gewissermaßen nie in einem „halbkonfigurierten“ Zustand befindet.
checkEmailAdress (*Address) kann etwa auch so umgebaut werden. Das spart dann schon mal so einige Zeilen.
createPasswordHash: Teilbedingungen negieren und OR-verknüpfen und dort Exception werfen und auch wieder auf else-Zweig verzichten.
Bei Datumsfunktionalität (DateTime) prinzipiell darauf achten, in der erwarteten Zeitzone zu sein. Globale Konfiguration ist tendenziell auch okay, aber trotzdem immer ein Konzept davon haben, in welcher Zeitzone was ist.
Dazu: Auch beachten, dass während der Scriptausführung Zeit vergeht und zwei `new DateTime()`-Anweisungen auch für eine niedrige Auflösung („minutengenau“) nicht zwingend die gleiche Zeit liefern. Wenn es ganz blöd läuft, kann sich das auch in Minute, Stunde, Tag, … unterscheiden. Wenn die Ausführung zum Beispiel um 23:59:59 beginnt.
verifyActRes: Ich nehme mal an, die Ausgaben dort (echo) sind Debugkram? Ansonsten müssen die weg. Wenn du das brauchst, gibt es als String zurück und lass andere Stellen entscheiden, ob, wann und wie es ausgegeben wird.
1168ff.: Für die geschachtelten `return false`-Zeilen sollte inzwischen klar sein, wie das umgebaut werden sollte. Faustregel zu Schachtelungstiefe: Je tiefer die Schachtelung, desto eher will der Code anders aufgebaut sein (vielleicht mit Aufteilung in mehr Methoden). Es gibt natürlich Ausnahmen, aber ab einer Tiefe von spätestens vier geschachtelten Konstrukten sollten die Alarmglocken schrillen. Du kommst da bis auf eine Tiefe von sechs oder sieben, wenn ich das richtig sehe. Das ist too much.
doLogin: Wo ich es gerade noch mal sehe: Versuch mal, die Methoden so zu schreiben, dass immer eine return-Zeile die letzte Zeile des Methodenkörpers ist. Nicht eine geschweifte Klammer.
verifyPassword macht zumindest nicht nur das, was der Name besagt. Auch hier wieder Exception als Guard Clause nach vorne ziehen und Schachtelungstiefe sparen.
AutoLogin: Zugriff auf $_SESSION und $_COOKIE würde ich tendenziell auch irgendwie abstrahieren, weil es da schnell Probleme gibt, wenn verschiedene Anwendungen auf der gleichen Domain laufen und sich die Keys für Sessions und Cookies teilen müssen. Vielleicht zumindest die Keys mit einem Namespace versehen oder sie in einem Namespace-Unter-Array von etwa Session halten. Die direkte Arbeit mit Superglobals ist auch immer ein kleines Warnsignal, auch wenn ich das jetzt nicht übertreiben will. $_SERVER['HTTP_USER_AGENT'] sollte vielleicht tendenziell auch von außen reingereicht werden.
setSessionLoggedIn: Je mehr Infos du direkt in die Sessions schreibst, desto mehr Werte gehen potenziell out of sync, wenn sie in der DB geändert werden. So was wie username und emailadress lieber bei Bedarf aus der DB holen und nur die User-ID in die Session schreiben. Bei dem switch fehlt vielleicht ein default-case. Die einzelnen Schritte/DB-Abfragen dieser Methode wären sicherlich Kandidaten, in eigene (private) Methoden ausgelagert zu werden.
checkLogin: `return (isset($_SESSION['loggedin']) && $_SESSION['loggedin'] === 1);` -- Name eher so was wie `isLoggedIn`.
doLogout: Damit zerstörst du alles, was auf der Domain für den Client gerade Session-Daten gesetzt hat. Das ist nicht zwangsläufig tragisch (mache ich gerade bei kleineren Sachen auch oft so), aber da wäre letztlich die Frage, wie du deine Klasse positioniert siehst. Wenn sie „fair“ mit potenziell weiterer Software spielen soll, wäre es in jedem Fall gut, die eigenen Session-Daten in einem Namespace zu halten (siehe oben). Wiederverwendbare Komponenten oder Anwendungen, die von Leuten als Third-Pary-Software genutzt werden sollen, sollten darauf achten, nichts Fremdes zu zerschießen.
generateToken: Besser mt_rand verwenden und die 54 per Code ermitteln. Vielleicht noch in eine for-Schleife ändern, aber nicht so wichtig. Theoretisch musst du die Buchstaben auch nicht ausbuchstabieren, sonderen kannst `range('a', 'z')` usw. aufrufen. (Achtung: Nicht `range('A', 'z')` -- man beachte Groß-/Kleinschreibung -- schreiben. Alles schon gesehen. )
generateHash: Ich würde aus dem Stegreif sagen, dass die openssl-Erweiterung nicht auf jedem Webspace verfügbar ist. Vielleicht nicht extra wegen dieser einen (?) Sache eine Abhängigkeit dazu verursachen.

**HighTec** · 22-03-2017, 15:54

Hey, erstmal vielen Dank für dein Feedback. (Hatte ehrlich gesagt nicht mehr mit einer Reaktion gerechnet)

- Da password_hash von Haus aus schon ein Salt generiert und einfügt wollte ic damit nur noch etwas "Pfeffer" dazu geben

Ich bin mir aber bewusst, dass dies eigentlich nicht nötig ist, da pasword_hash($passwort, PASSWORD_DEFAULT) bereits sicher genug ist und ich mir somit keine
Probleme bei einer eventuellen Änderung des Peppers einfange. Ich lass es zukünftig ganz weg.

- Zu 786: Das $errorflag benutze ich in diesem Fall um Mehrere Fehlermedungen zu sammeln und gesammelt zurück zu geben. Damit man direkt alle Fehler korrigieren kann. Gibt es da eine andere Nöglichkeit um dies zu bewerkstelligen?

- verifyActRes: Ja die Ausgaben sind Debugkram

hatt die nur vergessen heraus zu nehmen.

Deine anderen Punkte werde ich natürlich beherzigen. Ich bin mittlerweile auf den Stand die vorhandene Klasse in den Rundordner einzusorteiren und mich erstmal die nächsten Tage mit anderen Dingen wie Exceptions, Model-Interfaces, PSR1/2, usw. zu beschäftigen, bevor ich mich wieder an die Programmierung und Strukturierung heranwage.
Dennoch bin ich noch hin und hergerissen bei der Verwendung fertiger Klassen wie Active Record Pattern oder ähnliches.
natürlich erleichtert dies einem die Arbeit, aber mir riecht das immer mehr nach CopyAndPaste als nach eigener Leistung.

Zu Lernzwecken vielleicht selbst schreiben und die Fertigen Libs verwenden wenn ein konkretes Projekt fertig gestellt werden soll.
Oder sehe ich das falsch?

Gruß

Christian

**mermshaus** · 22-03-2017, 19:31

Zu 786: Das $errorflag benutze ich in diesem Fall um Mehrere Fehlermedungen zu sammeln und gesammelt zurück zu geben. Damit man direkt alle Fehler korrigieren kann. Gibt es da eine andere Nöglichkeit um dies zu bewerkstelligen?

Ich meinte einfach so:

PHP Code:


if ($errorflag !== 0) {
    return $errorRegister;
}
// kein else

Eine Alternative für den gesamten Abschnitt könnte zum Beispiel in diese Richtung gehen:

PHP Code:


// Check values for errors

$errors = array();

if (!$this->checkUsername()) {
    $errors[] = 'usernametaken';
}

$checkemail = $this->checkEmailAdress();

if ($checkemail['EMailcorrectly'] === 0) {
    $errors[] = 'emailinvalid';
} elseif ($checkemail['EMailavilable'] === 0) {
    $errors[] = 'emailtaken';
}

if (count($errors) > 0) {
    return $errors;
}

Ich habe die Methode insgesamt auch noch mal exemplarisch etwas umgestaltet:

PHP Code:


/**
 * All-in-One Methode zur Registration von neuen Benutzern
 *
 * @param array $dbvalues Array mit weiteren Eintragungen fuer die Benutzerdatenbank (Der Index ist der Zellenname, Der Wert ist der Wert der Zelle)
 * @return Bool TRUE bei erfolgreicher Registrierung.
 * @return array $errorRegister bei einem Fehler
 */
public function doRegister(array $dbvalues = array())
{
    if (
        $this->username === null
        || $this->password === null
        || $this->emailadress === null
    ) {
        throw new UserAuthExceptions(sprintf(
            'doRegister->Die Eigenschaften Username, Password und Emailadress muessen gesetzt sein. Zustand zum Fehlerzeitpunkt: Username = "%s"; Emailadresse = "%s"; Passwort = "%s"',
            $this->username,
            $this->password,
            (empty($this->password)) ? 'nicht gesetzt' : 'gesetzt'
        ));
    }

    // Check values for errors

    $errors = array();

    if (!$this->checkUsername()) {
        $errors[] = 'usernametaken';
    }

    $checkemail = $this->checkEmailAdress();

    if ($checkemail['EMailcorrectly'] === 0) {
        $errors[] = 'emailinvalid';
    } elseif ($checkemail['EMailavilable'] === 0) {
        $errors[] = 'emailtaken';
    }

    if (count($errors) > 0) {
        return $errors;
    }

    // Generate hashes and tokens

    $phash = $this->createPasswordHash();

    $this->generateActResToken();

    // DB query

    $addtosqlqueryindex  = '';
    $addtosqlqueryvalues = '';

    if (count($dbvalues) > 0) {
        $addtosqlqueryindex  = ', ' . implode(', ', array_keys($dbvalues));
        $addtosqlqueryvalues = str_repeat(', ?', count($dbvalues));
    }

    $query = '
        INSERT INTO ' . $this->usertab . '
        (
            ' . $this->cellusername    . ',
            ' . $this->cellpassword    . ',
            ' . $this->cellemail       . ',
            ' . $this->cellactstat     . ',
            ' . $this->celltoken       . ',
            ' . $this->celltokenexpire . '
            ' . $addtosqlqueryindex    . '
        )
        VALUES
        (
            ?,
            ?,
            ?,
            "0",
            ?,
            ?' . $addtosqlqueryvalues . '
        )
    ';

    $params = array(
        $this->username,
        $phash,
        $this->emailadress,
        $this->dbtoken,
        $this->tokenexpiredatetime
    );

    foreach ($dbvalues as $value) {
        $params[] = $value;
    }

    $this->query($query, $params);

    // Update fields

    $this->setUserID();
    $this->sendActResLink();
    $this->password = null;

    return true;
}

Das sind teilweise aber auch bloß Anregungen oder persönliche Vorlieben von mir. Ich habe auch wieder die nicht vorhandene query-Methode genutzt, und der Code wird sicher ein, zwei Fehler haben, weil er nicht getestet ist.

Das soll vor allem zeigen, dass man ohne viel Schachtelung auskommen kann.

Ich bin mittlerweile auf den Stand die vorhandene Klasse in den Rundordner einzusorteiren und mich erstmal die nächsten Tage mit anderen Dingen wie Exceptions, Model-Interfaces, PSR1/2, usw. zu beschäftigen, bevor ich mich wieder an die Programmierung und Strukturierung heranwage.

Wenn ich das sagen darf: Der Code an sich ist zumindest solide. Hier und da fehlt vielleicht ein wenig was oder ist nicht ganz hundertprozentig oder hat einen Tippfehler drin, aber die Grundlage ist in jedem Fall nicht schlecht. Auch die Instanzvariablen und Methoden mit Nutzung von public/private sind vom Ansatz her durchaus als in Ordnung erkennbar. Das geht in die richtige Richtung.

Das wahrscheinlich größte Problem mit deiner Klasse ist, dass sie zu umfangreich ist und zu viel macht (und dass sie dazu gerade bei den DB-Abfragen mit Fehlerbehandlung auch noch einige Redundanz im Code enthält). Das gilt auch für die Methoden selbst. Die sind oftmals zu lang und machen zu viele unterschiedliche Dinge. Ich habe vor allem versucht, ein paar Tipps zu geben, wie man den Code „handwerklich“ etwas entzerrter/lesbarer gestaltet.

Was die Strukturierung (etwa in verschiedene Klassen) angeht, ist das leider nicht so leicht in einem Satz zu erklären beziehungsweise ist es oft schwierig, Aspekte losgelöst vom Gesamtsystem zu betrachten. Außerdem gibt es dann letztlich auch nicht die eine korrekte Vorgehensweise, so was umzusetzen. Mein Tipp ist immer, mal ein Tutorial von einem verbreiteten Framework (Symfony, Laravel, vielleicht Zend) zu machen.

Dennoch bin ich noch hin und hergerissen bei der Verwendung fertiger Klassen wie Active Record Pattern oder ähnliches.
natürlich erleichtert dies einem die Arbeit, aber mir riecht das immer mehr nach CopyAndPaste als nach eigener Leistung.

Ich verstehe schon, was du meinst, aber wir sind nun mal auch keine Inseln.

Man kann sich nicht alles wirklich selbst erarbeiten. Das ist keine effiziente Zeitnutzung, und dazu sind die Dinge in den Details auch einfach zu kompliziert. Sicher gibt es einen Lerneffekt, der ein Argument ist und der bestimmt nicht vernachlässigt werden sollte, wenn man etwas von Grund auf selbst schreibt, aber andererseits bringt es am Ende – wenn man es mal final motiviert betrachtet – auch nicht so recht was, wenn man eine Komponente geschrieben hat, die schlechter ist als eine bestehende und verbreitete Komponente und die außer einem selbst niemand nutzt. Wobei auch das natürlich nicht immer so ganz in Stein gemeißelt ist, weil es wiederum auch Situationen gibt, in denen keine ausreichende bestehende Komponente existiert oder in denen sich die Nutzung einer größeren Library (die dann ja auch aktualisiert und gewartet werden will) einfach nicht lohnt. Das ist so ein wenig wie die Frage, ob CMS oder Eigenentwicklung. Wenn ich bloß einen Strauch im Garten einpflanze, brauche ich keinen Kleinbagger. Andererseits ist es auch nicht mein Job, Löcher mit dem Spaten zu graben. Es ist immer ein Abwägen. Ich tue mich da schwer mit Pauschalantworten oder definitiven Ratschlägen. Ich finde es wichtig, die Optionen zu kennen. Beschäftigung mit fertigen Komponenten ist zum Beispiel auch deshalb sinnvoll, weil man aus der Arbeit und den Ideen anderer Entwickler Erkenntnisse für eigenen Code mitnehmen kann. Dabei stößt man durchaus mal auf Dinge, die man vielleicht selbst noch überhaupt nicht bedacht hat.

Zu Lernzwecken vielleicht selbst schreiben und die Fertigen Libs verwenden wenn ein konkretes Projekt fertig gestellt werden soll.
Oder sehe ich das falsch?

Das habe ich im Prinzip schon kommentiert, aber dazu noch mal explizit als Bestätigung: Das wäre eine Vorgehensweise, klar. Ob es die sinnvollste oder beste ist: Ich kann es nicht sagen. Ein Aspekt wäre dazu wahrscheinlich noch, ob du im Team arbeitest oder im Team arbeiten willst. Vorsichtig gesagt: Je mehr Team, desto mehr Framework, weil einfach die verlässlich(er)en Konventionen Sinn ergeben.

Benötige eine Bewertung meiner Klasse