Hallo,

Gister weer begonnen met OOP en ik had gister een login/register/passforget systeem gemaakt met OOP en ik vroeg mij af of ik het zo ongeveer goed deed en wat ik kan verbeteren.

De database users ziet er zo uit:


databaseClass.php

<?php
    class Database
    {
        private $servername;
        private $username;
        private $password;
        private $database;
        private $conn;

        protected function connect()
        {
            if ($this->conn == NULL)
            {
                $this->servername = 'localhost';
                $this->username = 'root';
                $this->password = '';
                $this->database = 'oop';

                $this->conn = new mysqli($this->servername, $this->username, $this->password, $this->database);
            }
            
            return $this->conn;
        }
    }
?>


userClass.php

<?php
    class User extends Database
    {
        protected function generateRandomCode($length)
        {
            $unique = false;

            while (!$unique)
            {
                $characters = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
                $charactersLength = strlen($characters);
                $randomCode = '';

                for ($i = 0; $i < $length; $i++)
                {
                    $randomCode .= $characters[rand(0, $charactersLength - 1)];
                }

                $sql = "SELECT `pcode` FROM `users` WHERE `change_ww_code` = '" . $this->connect()->real_escape_string($randomCode) . "'";
                $query = $this->connect()->query($sql);

                if ($query || $query->num_rows == 0)
                {
                    $unique = true;
                }
            }

            if ($unique == true)
            {
                return $randomCode;
            }
        }

        public function login($username, $password)
        {
            $sql = "SELECT `username`, `password`, `active` FROM `users` WHERE `username` = '" . $this->connect()->real_escape_string($username) . "'";
            $query = $this->connect()->query($sql);

            if ($query->num_rows == 0)
            {
                throw new Exception("De inlog gegevens zijn onjuist.");
            }
            else
            {
                $data = $query->fetch_assoc();

                if ($data['active'] == 0)
                {
                    throw new Exception("Je account is gedeactiveerd, neem contact op met een administrator.");
                }
                elseif (password_verify($password, $data['password']) == true)
                {
                    $sql = "UPDATE `users` SET `password` = '" . password_hash($password, PASSWORD_BCRYPT) . "', `last_online` = '" . date('Y-m-d H:i:s') . "', `change_ww_code` = '', `change_ww_perm` = '0' WHERE `username` = '" . $this->connect()->real_escape_string($username) . "'";
                    $query = $this->connect()->query($sql);
                }
                else
                {
                    throw new Exception("De inlog gegevens zijn onjuist.");
                }
            }
        }

        public function register($username, $password, $passwordrepeat)
        {
            $sql = "SELECT `username`, `password` FROM `users` WHERE `username` = '" . $this->connect()->real_escape_string($username) . "'";
            $query = $this->connect()->query($sql);

            if ($query->num_rows == 1)
            {
                throw new Exception("De gebruikersnaam is ongeldig.");
            }
            elseif ($password != $passwordrepeat || strlen($password) <= 5)
            {
                throw new Exception("Controleer de wachtwoorden of ze overeenkomen en 6 tekens of langer zijn.");
            }
            else
            {
                $sql = "INSERT INTO `users` (`username`, `password`, `register_date`) VALUE ('" . $this->connect()->real_escape_string($username) . "', '" . password_hash($password, PASSWORD_BCRYPT) . "', '" . date('Y-m-d H:i:s') . "')";
                $query = $this->connect()->query($sql);
                if (!$query)
                {
                    throw new Exception("Er is iets fout gegaan, neem contact op met een administrator.");
                }
            }
        }

        public function passForget($username, $password, $passwordrepeat, $code)
        {
            $sql = "SELECT `username`, `change_ww_perm`, `change_ww_code` FROM `users` WHERE `username` = '" . $this->connect()->real_escape_string($username) . "'";
            $query = $this->connect()->query($sql);

            if ($query->num_rows == 0)
            {
                throw new Exception("De gebruikersnaam is ongeldig.");
            }
            else
            {
                $data = $query->fetch_assoc();
                if ($password != $passwordrepeat || strlen($password) <= 5)
                {
                    throw new Exception("Controleer de wachtwoorden of ze overeenkomen en 6 tekens of langer zijn.");
                }
                elseif ($data['change_ww_perm'] == 0)
                {
                    throw new Exception("Je hebt geen toestemming om je wachtwoord te veranderen.");
                }
                elseif (!isset($code) || $code != $data['change_ww_code'])
                {
                    throw new Exception("De opgegeven code is onjuist.");
                }
                else
                {
                    $sql = "UPDATE `users` SET `password` = '" . password_hash($password, PASSWORD_BCRYPT) . "', `change_ww_perm` = '', `change_ww_code` = '' WHERE `username` = '" . $this->connect()->real_escape_string($username) . "'";
                    $query = $this->connect()->query($sql);
                    if (!$query)
                    {
                        throw new Exception("Er is iets fout gegaan, neem contact op met een administrator.");
                    }
                }
            }
        }
    }
?>


Ik weet niet of ik teveel if/elseif/else statements heb zo ja, zouden jullie mij dan kunnen vertellen hoe ik dit kan verbeteren :)

Graag hoor ik verbeter punten.

Mvg,
Rob
Volgens youtube https://www.youtube.com/watch?v=PHiu0JA9eqE kan dit wel gewoon. Dus dat filmpje heeft het fout? En hoe moet ik dan zorgen dat de User class de database connectie kan gebruiken? Moet ik daarvoor global $conn gebruiken, maar global is volgens vele mensen niet veilig..
Dat het in PHP kan, wil niet zeggen dat het juist is. Het filmpje heeft het dan ongetwijfeld behoorlijk fout.
De beste oplossing is om de instance van de database-class te voeren aan als argument aan je User-class.
Zou u misschien ergens een voorbeeld willen neerzetten? Ik heb het opgezocht maar snap niet precies wat ze aan het doen zijn.
Misschien moet je ook exceptions iets anders gaan zien.

Een exception treedt op (of zou op moeten treden) als er iets onverwachts gebeurt waarbij het mogelijk niet direct duidelijk is hoe verder te gaan, en soms staat het ter discussie of het uberhaupt mogelijk is om verder te kunnen gaan.

Dit is natuurlijk een groot grijs gebied, maar als je bijvoorbeeld naar je login-methode kijkt dan ligt het in zekere zin in de lijn der verwachting dat het inloggen wel eens mis kan gaan om een aantal uiteenlopende redenen. Ik weet echter niet of je deze zou moeten classificeren als exceptions, want je weet op dat moment precies wat er gebeurt, en wat er mis is gegaan.

En dan is het de vraag of je in dit geval de gebruiker (lees: potentiële aanvaller) uit moet leggen wat er precies misgaat. Je zou ook kunnen volstaan met een bericht -naar de gebruiker toe- dat de login simpelweg is mislukt, en dat deze het nogmaals moet proberen. Ondertussen zou je de (mislukte en geslaagde) loginpoging(en) kunnen loggen waarbij je uitgebreidere informatie bijhoudt.

Dan over methoden: deze voeren meestal een enkele taak uit, maar dat hoeft niet in te houden dat dit altijd weinig code is. Het moet natuurlijk wel een duidelijk afgebakend gebied zijn. De passForget-methode is een beetje vreemd: als iemand een recht niet heeft, geef deze persoon dan simpelweg de keuze niet om iets te kunnen wijzigen. Ook zou je hier eigenlijk van database-transacties gebruik moeten maken: de informatie die je mogelijk aanpast in de methode moet gedurende die methode vergrendeld zijn.

Over regels 72 en 99: daar wordt twee keer eenzelfde conditie vastgelegd waar een wachtwoord aan moet voldoen: ten minste 6 karakters. Wat als je dadelijk meer regels hebt? Deze moet je eigenlijk op één plaats vastleggen zodat je deze ook op één plaats kunt wijzigen. Als je code groeit heb je straks op tig plaatsen allerlei (en in het ergste geval: verschillende) condities waar wachtwoorden aan zouden moeten voldoen.

Over de database class: vraag jezelf af wat de meerwaarde van het opslaan van connectie-parameters is. Wat mij betreft id die er niet echt. Het is nogal raar dat $conn private is, maar je daar wel -buiten het object om, en daarmee in feite dus rechtstreeks via de mysqli-class- mee communiceert.

Op dit moment heeft de database-class niet echt veel toegevoegde waarde, het neemt niet echt veel werk uit handen. Dit verandert natuurlijk wanneer je deze class wat verder uitbreidt. Kijk bijvoorbeeld voor inspiratie in deze thread (moet link even opsnorren omdat ik mijn eigen topics niet kan vinden met zoekfunctionaliteit -_-). EDIT: in grote lijnen gebruik ik deze classes (hier en daar enkele toevoegingen) nog steeds voor wat eigen projectjes. Eigenlijk het belangrijkste is dat je een CHARACTER ENCODING instelt bij het maken van een connectie.
- Ariën - op 26/12/2017 14:14:37

Het extenden klopt sowieso al niet. Want jij zegt dat een User een soort Database is.


Zou je User niet kunnen zien als een representatie van een deel van de database? De kinderen delen daarbij de gemeenschappelijke connectie zoals gedefinieerd in de parent?

EDIT: toevoeging... ik wil de topicstarter (Rob) niet meteen overladen, maar ik zie dat de User-class zowel de database benadert als ook de opgevraagde data verwerkt. Een veelgebruikt design-pattern is MVC: Model - View - Control.

Thomas van den Heuvel op 26/12/2017 15:40:20

Misschien moet je ook exceptions iets anders gaan zien.

Een exception treedt op (of zou op moeten treden) als er iets onverwachts gebeurt waarbij het mogelijk niet direct duidelijk is hoe verder te gaan, of dat men uberhaupt verder kan gaan.

Dit is natuurlijk een groot grijs gebied, maar als je bijvoorbeeld naar je login-methode kijkt dan ligt het in zekere zin in de lijn der verwachting dat het inloggen wel eens mis kan gaan om een aantal uiteenlopende redenen. Ik weet echter niet of je deze zou moeten classificeren als exceptions, want je weet op dat moment precies wat er gebeurt, en wat er mis is gegaan.

En dan is het de vraag of je in dit geval de gebruiker (lees: potentiële aanvaller) uit moet leggen wat er precies misgaat. Je zou ook kunnen volstaan met een bericht -naar de gebruiker toe- dat de login simpelweg is mislukt, en dat deze het nogmaals moet proberen. Ondertussen zou je de (mislukte en) loginpoging(en) kunnen loggen waarbij je uitgebreidere informatie bijhoudt.

Dan over methoden: deze voeren meestal een enkele taak uit, maar dat hoeft niet in te houden dat dit altijd weinig code is. Het moet natuurlijk wel een duidelijk afgebakend gebied zijn. De passForget-methode is een beetje vreemd: als iemand een recht niet heeft, geef deze persoon dan simpelweg de keuze niet om iets te kunnen wijzigen. Ook zou je hier eigenlijk van database-transacties gebruik moeten maken: de informatie die je mogelijk aanpast in de methode moet gedurende die methode vergrendeld zijn.

Over regels 72 en 99: daar wordt twee keer eenzelfde conditie vastgelegd waar een wachtwoord aan moet voldoen: ten minste 6 karakters. Wat als je dadelijk meer regels hebt? Deze moet je eigenlijk op één plaats vastleggen zodat je deze ook op één plaats kunt wijzigen. Als je code groeit heb je straks op tig plaatsen allerlei (en in het ergste geval: verschillende) condities waar wachtwoorden aan zouden moeten voldoen.

Over de database class: vraag jezelf af wat de meerwaarde van het opslaan van connectie-parameters is. Wat mij betreft id die er niet echt. Het is nogal raar dat $conn private is, maar je daar wel -buiten het object om, en daarmee in feite dus rechtstreeks via de mysqli-class- mee communiceert.

Op dit moment heeft de database-class niet echt veel toegevoegde waarde, het neemt niet echt veel werk uit handen. Dit verandert natuurlijk wanneer je deze class wat verder uitbreidt. Kijk bijvoorbeeld voor inspiratie in deze thread (moet link even opsnorren omdat ik mijn eigen topics niet kan vinden met zoekfunctionaliteit -_-). EDIT: in grote lijnen gebruik ik deze classes (hier en daar enkele toevoegingen) nog steeds voor wat eigen projectjes. Eigenlijk het belangrijkste is dat je een CHARACTER ENCODING instelt bij het maken van een connectie.


De expcetions doe ik omdat dit mij is is verteld in een van mijn eerdere topics.

Als dit is zo bij het inloggen, maar bij het registreren moeten ze wel weten waarom hun wachtwoord niet goed is, anders blijven ze het maar proberen, of niet?

Ik heb $conn private gemaakt zodat alleen DIE class er iets mee kan doen, via connect() kan ik dan vervolgens het gaan gebruiken in andere classen


[size=xsmall]Toevoeging op 26/12/2017 16:05:53:[/size]

- Ariën - op 26/12/2017 15:03:23

Dat het in PHP kan, wil niet zeggen dat het juist is. Het filmpje heeft het dan ongetwijfeld behoorlijk fout.
De beste oplossing is om de instance van de database-class te voeren aan als argument aan je User-class.


Zou ik bv. ook aan Mapper kunnen gebruiken, zie deze thread: https://www.phphulp.nl/php/forum/topic/oop/100868/ (ergens onderaan)

De expcetions doe ik omdat dit mij is is verteld in een van mijn eerdere topics.

If it's on the internet, it must be true.

Als dit is zo bij het inloggen, maar bij het registreren moeten ze wel weten waarom hun wachtwoord niet goed is, anders blijven ze het maar proberen, of niet?

Ja, maar dat maakt het nog geen exception. Ongeldige invoer in een formulier is niet (of in ieder geval niet per definitie) een exception. Dit resulteert enkel in het niet slagen van een validatie, maar de code weet precies wat er vervolgens moet gebeuren: foutmeldingen terugsturen naar de gebruiker. Hier is dus geen sprake van code die spaak loopt doordat er iets onverwachts is gebeurd, maar omdat de gebruiker ongeldige invoer heeft ingevuld. Dit ligt dus in de lijn der verwachting en als het goed is heb je hier ook een (gebruiksvriendelijke) afhandeling voor. Exception zijn juist voor situaties waarbij je niet (direct) weet hoe verder te gaan.

Ik heb $conn private gemaakt zodat alleen DIE class er iets mee kan doen, via connect() kan ik dan vervolgens het gaan gebruiken in andere classen

Ja, je retourneert (een private) $this->conn, en dat is niets meer dan een mysqli-object. Dus waarom werk je daar dan niet rechtstreeks mee :p. Je kunt ook ergens gewoon een variabele $db die je doorgeeft aan andere objecten. Maar $db is dan wel van de klasse Database (je wrapper), en niet van mysqli, die enkel in Database intern wordt gebruikt...

EDIT: het is op zijn minst een verkeerde gebruik van private, want dat ding is -in het gebruik- niet private, je mikt het gewoon de buitenwereld in middels return $this->conn. Hiermee schep je een verkeerd beeld van $conn.
"If it's on the internet, it must be true."
Dat niet persee maar Ariën had mij dit gezegd.

Hoe zou ik anders die fouten moeten afhandelen? Echo is niet netjes.
Maak een Form class waarmee je een formulier+velden kunt opbouwen, valideren etc? :]

Zou je ook eerst zonder classes/objecten kunnen doen. In zijn simpelste vorm:

toon Form (al dan niet met foutmeldingen bij de velden met verkeerde invoer van eerdere pogingen)
<verstuur>
validatie van form
ok -> verwerk -> stuur door naar bedankpagina/overzicht/whatever
niet ok -> sla fouten op (bijvoorbeeld in sessie), terug naar toon Form

En fouten op het scherm dumpen is niet echt afhandelen he, da's meer debug :).

Reageren