Feedback gevraagd: basis php

Zoals Rickert zei, je database functie kan beter. Als je toch al over gaat op OOP, mooie tijd om te zien hoe statics werken :)

<?php
function dbConnect(){
    static $dbh;
    // connect to db. If connection fails, return false, otherwise return PDO object.
    if(!empty($dbh)){
      return $dbh;
    }

    // db settings
    $host = '';
    $dbname = '';
    $user = '';
    $pw = '';

    try{
        // connect to db and return pdo opject if connection succeeds
        $dbh = new PDO("mysql:host=$host;dbname=$dbname", $user, $pw);
        $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    }catch(PDOException $e){
        $dbh = false;
        error_log( $e->getMessage(), 3,"../logs.txt");
    }

    return $dbh;

}?>

@Johan: De db Settings horen natuurlijk netjes thuis in een configuratie bestand.

Als het mag dan wil ik graag nog een praktische tip geven:

In de function updatePw:
<?php
function updatePw()
{
if(a) {
if(b) {
if(c) {
if(d) {
// doe iets
return TRUE;
} else {
return FALSE;
}
} else {
return FALSE;
}
} else {
return FALSE;
}
} else {
return FALSE;
}
}
?>

Kun je vereenvoudigen tot:

<?php
function updatePw()
{
if(!a) {
return FALSE;
}

if(!b) {
return FALSE;
}

if(!c) {
return FALSE;
}

if(!d) {
return FALSE;
}

// doe iets
return TRUE;
}
?>

Of:

<?php
function updatePw()
{
if(a) {
if(b) {
if(c) {
if(d) {
// doe iets
return TRUE;
}
}
}
}

return FALSE;
}
?>

Of

<?php
function updatePw() {
    if (a && b && c && d) {
        // doe iets
        return true;
    }
    return false;
}
?>

Verder zou ik niet strip_tags() over je input strooien, dat is niet echt ergens voor nodig en is toch een beetje schijnveiligheid.

Dan weet ik niet hoe header.php en footer.php er uit zien maar is het toch wel belangrijk dat je overal consistent dezelfde character encoding gebruikt. Ook bij het maken van een verbinding met je database zit in je DSN (je connectie-string) geen charset-aanduiding. Ook je escaping-functionaliteit ( functies zoals htmlspecialchars()) werkt mogelijk niet goed als je niet de juiste character encoding instelt.

En dan nog twee samenhangende issues:
Enerzijds zijn je acties niet heel erg gescheiden, zo combineer je bijvoorbeeld enerzijds het tonen van een signup-formulier, en anderzijds het afhandelen van het verwerken hiervan. Deze acties ("toon inschrijfformulier", "verwerk inschrijfformulier") zou je verder uit elkaar moeten trekken zodat je de code voor elk van deze acties in afzondering kunt behandelen.

Anderzijds introduceer je voor allerhande zaken allerlei functies die je vervolgens maar 1x gebruikt. Dat is niet echt het idee van een functie. In een functie bundel je code die je meerdere keren kunt (her)gebruiken. Zo is de functie "sendConfirm()" veel te specifiek, maar als je daar een soort van wrappertje van maakt om de mail() functie om een standaard zwik headers mee te sturen, en deze voorziet van wat meer parameters (titel, bodytext etc.), dan wordt deze functie al meteen een stuk breder inzetbaar.

Ik denk dat je een hoop functies gewoon "inline" kunt zetten in de eerdergenoemde "acties".

Rickert Bombaklats op 28/08/2015 13:25:11

Je code bevat een aantal fucnties die je aanroept, dit is helaas geen OOP :(

Hi Rickert :)

Nee, dat klopt ook! Ik zeg ook in mijn eerste post dat ik het nu nog procedural is, en dat ik VOORDAT ik overga op OO, eerst wat feedback wil ;)

Bedankt voor je tips. Ik vraag me alleen af hoe je bepaalde 'flow' zonder geneste if/else kunt doen, wat mijn code betreft?

En hoe zou ik het netter kunnen maken? Of heeft dat vooral met de procedural opzet te maken?
Template frameworks e.d. heb ik mij nog niet in verdiept. Ik wilde nu mijn basiskennis die ik tot nu toe heb vergaard, oefenen.

Groetjes, Iris

Je zou ook even kunnen kijken naar "ternary" operators. Hiermee kan je veel code in jouw script 1-linen.

  echo 1 > 0 ? 'true' : 'false'; // dit echo'd true want 1 is groter dan 0.

  // of in jouw script (functies lijn 128->135)
  return $stm->rowCount() > 0 ? $stm->fetchObject() : false;
  // en verander lijn 143 naar
  return false;

Als er dan een exception op treet in $stm->fetchObject() dan word die catch block uitgevoerd inplaats van try block, dus er word niets gereturned. Aangezien de functie standaard van false returned behaal je hetzelfde, met minder code. Dit kan overal in jouw gehele code ook gedaan worden.

- Aar - op 28/08/2015 14:04:55

Als je wilt controleren of een formulier verstuurd is, gebruik liever dit:

<?php 
if($_SERVER['REQUEST_METHOD']=="POST") {
// afhandeling
} else {
// niet ge-POST.
?>

Hi Aar,

Bedankt voor de tip.
Kun je toelichten waarom dit beter is?

Groetjes, Iris

[size=xsmall]Toevoeging op 29/08/2015 07:52:06:[/size]

Beste Johan, Frank en Thomas,

Echt top, jullie feedback! Hier heb ik heel veel aan!!

Ik ga ermee aan de slag en als ik nog vragen heb, dan post ik ze.

Fijn weekend.

>> Kun je toelichten waarom dit beter is?

Er zijn vier verschillende manieren waarop je een verzoek aan een webserver kunt sturen:

HTTP GET: Voor het opvragen van informatie
HTTP POST: Voor het verwerken van nieuwe gegevens
HTTP PUT: Voor het updaten van gegevens
HTTP DELETE: Voor het verwijderen van gegevens

Met het uitlezen van de superglobal $_SERVER['REQUEST_METHOD'] kom je er achter welke 'METHOD' er door de client gebruikt is en als dat de POST methode is dan mag je dus ook min of meer verwachten dat er data ($_POST variabelen) meegestuurd wordt door de client.

Er zijn meer request methods dan alleen POST, GET, PUT en DELETE.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

Het zijn wel de meest gebruikelijke. Vooral met de REST standaard krijg je hier veel mee te maken.

@johan, ik zou het gebruik van tenary operators zoveel mogelijk afraden, soms ontkom je er niet aan maar het maakt je code onduidelijk voor de andere developers.

Wat Frank zegt is super belangrijk, snel returnen.
Hierin maak je een method of functionaliteit waarbij je elke keer als er een conditie zich voordoet je dit meteen kan tergug geven.

<?php
function defineNameIsIris($name) {
if ('Iris' == $name) {
 return true;
}

return false;
}
?>