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
Hallo,

Dit soort classes maken is natuurlijk altijd goed om te leren hoe je oop moet programmeren. Hopelijk heb je wat aan de volgende opmerking(en):

De scope van je functies zijn te groot. Je zou per functie slechts één ding moeten doen. Klein voorbeeld van een aanroep(uit de duim gezogen) waarbij elke functie WEL 1 ding doet:

$kareltje = $database->select("user")->whereEquals('id', 5)->whereEquals("nog een vergelijking", "...")->order("id", "DESC")->firstOrDefault();

Ik wil je code niet afzeiken(het doet naar verluid wat het moet doen, en daar gaat het om), maar ik ga ervan uit dat je jouw programmeerkunsten wilt verbeteren.

Als je als doel stelt dat een functie écht maar 1 ding doet, krijg je code die gemakkelijk onderhouden kan worden. Om functionaliteit toe te voegen kan je namelijk een functie toevoegen ipv een functie aan te passen. Het aanpassen van een functie is bug- gevoelig, vaak zelfs onmogelijk zonder er een bak met tijd overheen te gooien.

Verder kan het handig zijn om "magical strings" in constanten op te slaan. "Where clause is empty for update method" komt bijvoorbeeld 2x voor in je code. Dubbele code moet je altijd proberen te voorkomen. dit kan je oplossen door een constant toe te voegen aan de class met de desbetreffende text. Persoonlijk sla ik constants altijd op in een aparte class. Dit doe je ten slotte ook met taalspecifieke onderdelen.

Hopelijk heb je hier al wat aan.

Nog wat tips:
https://laravel-news.com/clean-code-php-guide
Thnx voor reactie, Dit is alleen niet mijn code :P is van github en ik wou graag weten of dit goed was zodat ik ervan kan leren :)
Niet aan gedacht, ik wou alleen even kijken of dat goed was aangezien het 2 jaar geleden is. Zodat ik kan zien of ik ervan kan leren.
Oké, dan weet je het in ieder geval voor een volgende keer ;-)
- Rob - op 26/12/2017 14:28:39

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..


Nee geen Global maar injections.

DatabaseInterface.php
<?php

interface DatabaseInterface
{
// een lijst met verplichte functies die een Database class VERPLICHT MOET hebben
// google eens op 'php interface'
{
?>

Database.php
<?php
class Database implements DatabaseInterface
{
// 'De' database class. De koppeling tussen database en jouw applicatie
}
?>

User.php
<?php
// Deze class slaat de gegevens van één user op in het geheugen. een soort OOP associatieve array dus
class User {
private username;
// etc

public function setUsername($username) {
$this->username = $username;

return $this; // google op 'php method chaining'
}

public function getUsername() {
return $this->username;
}

// etc
}
?>

Repository.php
<?php

class Repository
{
private $db;

public __contstruct(DatabaseInterface $database) {
$this->db = $database;
}

// deze class is een base class voor UserRepository maar later ook voor OrderRepository, CustomerRepository en weet ik veel (bedenk het maar)

// hier schrijf je dus methods die door de child classes gebruikt kunnen worden
}
?>

UserRepository.php
<?php
class UserRepository extends Repository
{
public __contstruct(DatabaseInterface $database) { // <-- DatabaseINTERFACE en niet Database. zie commentaar aan het einde
parent::__construct($database);
}

// hier ga je je queries met betrekking tot de user tabel schrijven

public function findUserByUsername($username) {
$result = $db->query("SELECT * FROM users WHERE username='$username'");
if($result) {
$user = new User();
$user->fromArray($db->fetch_assoc($result));
return $user;
}

}

// meer 'query' functies
}
?>

Waarom een database interface?

Waarschijnlijk wil je nu een mysql database gaan gebruiken. Maar wat nu als je later je code met een MongoDatabase wilt laten werken? Of met een MySqLite database? Dan kun je een nieuwe Database class maken die ook DatabaseInterface implementeert en dan weet je dat deze class met AL je code gewoon werkt,
Hier moet ik me ook eens in verdiepen. Maar zo'n 'Repository' zoals jij omschrijft, heet dat ook geen Mapper? Of is dat weer wat anders?
Waar Frank schrijft __contstruct (what were you thinking) moet je schrijven __construct.
- Rob - op 27/12/2017 19:27:55
Hey,

Is dit een goede database class?:
<knip>

Nee, niet echt.

Hierover verschillen de meningen, maar final en private verdienen in principe niet de voorkeur omdat dit niet het uitbreidbaarheidsbeginsel van object-georiënteerde code volgt.

Daarnaast introduceert het nogal wat abstractie, de vraag is of je hier iets aan hebt wanneer je (MySQL-)specifieke queries aan het opstellen bent. Persoonlijk vind ik het veel fijner wanneer je zelf de volledige controle heb over hoe een query er uit ziet. Ook is dat vaak handiger wanneer je queries moet debuggen. Deze class regelt ook niets omtrent transacties :/.

Maar het meest funeste is nog dat deze database-class niet expliciet een character encoding instelt bij het maken van je connectie. Escaping-functionaliteit is (mede) afhankelijk van een juiste character encoding. Dit had ik eigenlijk al aangegeven in een vorige reactie. Hierin staat tevens een link naar een andere implementatie. Ik zeg niet dat deze beter is (dit is mijn eigen wiel) maar heb je hier uberhaupt naar gekeken?

En inderdaad, wanneer je met OOP begint te werken is het vooral belangrijk dat je niet direct teveel hooi op je vork neemt.

EDIT: en dat is misschien een aparte discussie waard: in hoeverre is (een ver doorgevoerde mate van) abstractie nog van toegevoegde waarde. Je kunt wel allerlei tussenlagen inbouwen om in het midden te laten wat code uiteindelijk moet doen, maar uiteindelijk zal het toch iets moeten doen. Door abstractie wordt code ook abstracter :). Dit maakt het moeilijker leesbaar, moeilijker onderhoudbaar, dit kost ook tijd en dus geld. En mogelijk benut je nooit echt de geïntroduceerde flexibiliteit en in dat geval is al die code overbodige fluf.

EDIT #2: toegepast op @Frank zijn voorbeeld, en dit is niet bedoeld om zijn reactie af te kraken want deze code is ongetwijfeld opgesteld volgens de regels der kunst, maar hoe vaak zal er in de levensloop van een applicatie (vrij) geschakeld worden tussen verschillende database-typen? Het is denk ik vooral belangrijk om praktisch te blijven. OOP is een middel, geen doel, dus om in OOP te programmeren "enkel voor de vorm" lijkt mij niet de goede insteek. Je moet een reden hebben om dingen op een OOP-manier aan te pakken. PHP is een hybride taal, dus het is mogelijk om hybride (OOP/procedureel) te programmeren.

Daarintegen, er is nog steeds iets voor te zeggen om een wrapper om MySQL(i)-functies of -methoden te schrijven. Dit om de volgende reden: er zou van alles in MySQL-land kunnen veranderen en dat zou kunnen betekenen dat je dan de manier waarop je met je database communiceert om moet gooien. Wanneer deze omgang met de database rechtstreeks plaatsvondt zou dit inhouden dat alle aanroepen hardcoded in code zouden zitten. Dit zou dan ook kunnen inhouden dat je dus alle of een heleboel instanties van deze aanroepen moet aanpassen als hier iets in is veranderd. Ook al lijken wrappers redelijk loos (en je zult zelf moeten bepalen of (en zorgen dat) er een zekere meerwaarde bij gebruik is), deze zorgen nog steeds voor een zekere mate van abstractie - ze stellen je in staat om indirect met je database te communiceren. Mocht er bij gebruik van wrappers iets veranderen in MySQL, dan hoef je -idealiter uiteraard- maar op één plek code aan te passen.
Thomas van den Heuvel op 28/12/2017 14:13:49

Het is denk ik vooral belangrijk om praktisch te blijven. OOP is een middel, geen doel, dus om in OOP te programmeren "enkel voor de vorm" lijkt mij niet de goede insteek. Je moet een reden hebben om dingen op een OOP-manier aan te pakken.

Mooi gezegd, en helemaal waar.

+1

Reageren