Opbouwende kritiek nodig php programmeur

[Verwijderd]

Hallo,

bij het solliciteren moest ik een snippet van mijn laatst gemaakte code sturen.
Op basis van deze code ben ik afgewezen en na het navragen kreeg ik als antwoord:

Wat ik heb begrepen is dat de opbouw (gelaagdheid?) van de code van een te laag niveau is. Veel meer kan ik hier ook niet over zeggen vanuit een gebrek aan technische achtergrond.

Nu wil ik jullie vragen om mij tips te geven om beter te worden!

PHP:

<?php
// Dit is een script wat kijkt of de ipadres van de gebruiker in de database staat en dus verbannen is.
// Wanneer het ipadres gevonden wordt krijgt de gebruiker een error pagina te zien.

class handler 
{
    public function isBanned() // Kijk of dit ipadres verbannen is in de database
        {   
            $ip = $_SERVER['REMOTE_ADDR']; 
            $query = mysql_query("SELECT * FROM verzoek_bans WHERE ban_ip = '".$ip."'") or die(mysql_error());
            $count = mysql_num_rows($query);
            $row = mysql_fetch_assoc($query);
            if($count > 0) //Is er een ip gevonden in de database
            {
                //Invoeren in het logbestand
                include('../events/eventclass.php');  //include de eventlog module
                $event          = new eventlog; //decare eventlog
                $uname          = $this->getIdWithName($row['ban_name']);
                $logid          = 2; // 1 access 2 denied 3 update 4 handle
                $eventsource    = "Verzoeksysteem"; //Event titel
                $eventdesc      = "Toegang geweigerd: ".$row['ban_name']." met IP: ".$ip; //Event omschrijving  
                $event->insertlog($uname, $logid, $eventsource, $eventdesc); //Alles met de eventfunctie meesturen              
                return TRUE; //De waarde true naar het script sturen
            }
        }
}


$handler    = new handler; //Declare $hander
$isBanned   = $handler->isBanned(); //De variable makkelijer aanroepbaar maken
if($isBanned == TRUE)
{
    exit('U bent verbannen van onze website.');
}
            
?>          

In eventclass.php:
<?php
class eventlog
{
    //invoeren van het event in de database
    public function insertlog($uid,$logid,$desc,$txt)
    { 
        $txt = addslashes($txt); //Addslashes om het script niet te laten crashen
        $desc = addslashes($desc);
        $date = date('Y-m-d H:i:s'); //Current timestamp
        $sql = "INSERT INTO `eventlog` (`user_id`, `log_id`, `event_description`, `event_text`, `event_datetime`) VALUES ('$uid', '$logid', '$desc', '$txt', '$date')";
        mysql_query($sql) or die(mysql_error());
    }
}       
?>






Een whois-online script voor een membersysteem:
De class:
<?php
class online
{
    
    public function update() 
    //Update of creeër records voor de database
     {
        $ip         = $_SERVER['SERVER_ADDR']; 
        $uid        = $_SESSION['userid']; // User id is vastgesteld bij het inloggen
        $time       = date('H:i');
        $query      = "SELECT * FROM whois WHERE whois_ip = '".$ip."' AND user_id = '".$uid."'";
        $result     = mysql_query($query) or die(mysql_error());
        $count      = mysql_num_rows($result);
        
        if($count == 0) 
        // Wanneer er geen gegevens worden gevonden worden deze hier aangemaakt
        {
            mysql_query("
            INSERT INTO whois 
            (`user_id`, 
            `whois_ip`, 
            `whois_time`, 
            `whois_rank`,
            `whois_guestname`) 
            VALUES 
            ('".$id."', 
            '".$ip."', 
            '".$time."', 
            '".$_SESSION['rank']."',
            '".$_SESSION['name']."');
            ");
        }
        else
        // Er is al een record gevonden dus we updaten de tijd
        {
            mysql_query("UPDATE  whois SET whois_time =  '".$time."' WHERE  whois_ip = '".$ip."' AND user_id = '".$uid."'");
        }
    }
    
    public function check()
    // Deze functie controleert of er een gebruiker is die langer dan 1 minuut 
    // zijn record niet geupdate heeft. Deze wordt dan verwijdert zodat de online-
    // lijst accuraat blijft.
    {
        $time       = date('H:i');
        $query      = "SELECT * FROM whois"; // we vragen alle records aan.
        $result     = mysql_query($query) or die(mysql_error());
        // Deze loop controleerd voor elke input of deze over tijd is
        while ($row = mysql_fetch_assoc($result)) 
        {
            // Eerst maken we van de tijd een unixtimestamp en kijken of deze kleiner is dan 
            // de huidige tijd minus een minuut
            if(strtotime($row['whois_time']) < strtotime("-1 minutes"))
            {
                //Deze record is verlopen
                mysql_query("DELETE FROM whois WHERE whois_id =".$row['whois_id']);
            }
        }
    }
}

?>

Het volgende script maakt een reloading DIV:

<script language="JavaScript"> 
    var auto_refresh = setInterval(
    function()
    {
        $('#WhoisOnline').load('reload.php');
    }, 5000);
</script>
<div id="WhoisOnline"></div>

Reload.php:

<?php
    include("online/onlineclass.php");
    $online = new online;
    $online->update(); //insert/update online gegevens
    $online->check(); //check of er iemand niet meer is
?>

Graag opbouwende kritiek!

B.v.d,

[!null]

Wellicht dat je nog ouderwets SQL samenstelt terwijl je ook gebruik kan maken van prepared statements. Wat dat betreft kun je dus ook meer gebruik maken van bestaande libraries/componenten. Voor database werk. Maar ook voor HTML output kun je Smarty of andere templating e.d. gebruiken.

Echter bij je eerste voorbeeld (isBanned) is het toevoeggen van zo'n class weer vrij nutteloos, want het gaat maar om 1 functie, dus dat kan prima zonder class. Al weet ik de context natuurlijk niet. En dat zou dan juist minder gelaagdheid zijn.

[Voutloos]

• mysql_* is al jaaaaaaaaren deprecated
• addslashes: strings escape je echt met mysqli_real_escape_string (of je gebruikt parametrized queries), en als je 'om niet te laten crashen' erbij laat je wel heel duidelijk weten dat je niet weet waarom je moet escapen. Leesvoer dus.
• het loggen van je event is boilerplate code, had ook wel in de event class gekund.
• or die(mysql_error()) moet je _nooit_ doen
• Praktisch al je comments zijn nutteloos / obvious.

[CyberJack]

Als ik deze code zou moeten beoordelen zou mij het volgende opvallen:

- De classes hebben geen constructors
- Geen gebruik van autoloaders. Er worden dus veel files manueel inladen via include regels (files als "../events/eventclass.php" en "online/onlineclass.php")
- gebruik van addslashes voor het opslaan van data (ipv mysql_real_escape_string). Nog beter zou zijn om gebruik te maken van MySQLi of PDO. ( i.v.m. sql injections e.d. )
- Gebruik van date functies ipv DateTime objecten

Persoonlijk zou ik dan nog kijken naar (maar daar zou ik niet op letten bij aangeleverde code voor een sollicitatie)
- Namespaces als je gebruik maakt van php 5.3 of nieuwer
- Geen commentaar boven de functies / classen (is handig als je met een goede IDE werkt en kan documentatie van gegenereerd worden)
- Sommige functie namen hebben camelcase namen en sommige alleen lowercase ("isBanned" en "insertlog" bijvoorbeeld). Is niet consequent.

Dan nog een javascript puntje:
"<script Language="Javascript">" wordt al niet meer gebruikt sinds html 4.01
Tegenwoordig is dat "<script>" of "<script type="text/javascript">"


@voutloos: mysql_ functies zijn depreciated vanaf php 5.5.0 en die is op 20 juni 2013 uitgekomen. Is dus geen jaren (al gebruik ik het al jaren niet meer)

[ Voor 6% gewijzigd door CyberJack op 30-08-2013 16:50 ]

[Gamebuster]

Hard gezegd, maar op basis van deze code zou ik je ook niet aannemen, tenminste niet als ik niet eerst iemand drastisch wil bijscholen.

Meest opvallend: Het is 1 grote MySQL injection lek, volgens mij heb ik geen "veilige" query gezien.
Los daarvan zijn er ook andere issues die aantonen dat je niet echt lijkt te weten wat je nou eigenlijk precies doet, en dat is niet een eigenschap die je wil hebben in een developer.

Kortom, als "opbouwende kritiek" zou ik zeggen:
- Laat anderen vaker naar je code kijken om eventuele issues aan te wijzen.
- Bekijk de documentatie van iedere methode die je gebruikt
- Bij copy/paste van snippits van internet: Neem niet alleen over en check of het werkt, maar zorg er ook voor dat je precies weet wat het doet en waarom je het doet.
- Je comments staan er echt bij met het idee "meer = beter"; bijv bij deze include('../events/eventclass.php'); //include de eventlog module en vrijwel alle andere comments heb ik een erg hoog "no shit, sherlock" gehalte; comments zijn voor dingen die in 1e instantie niet duidelijk zouden kunnen zijn. Comments zijn bijv. niet echt om te omschrijven wat je code doet, maar vooral waarom het doet wat het doet.

Je hebt gewoon nog veel foute dingen te ontdekken. Lees een boek (of meerdere), lees de documentatie op php.net zelf ook

[ Voor 4% gewijzigd door Gamebuster op 30-08-2013 16:52 ]

[Bolletje]

Ik deel de mening over de comments: ze zeggen niet zoveel. Houd wel in gedachten dat je het naar bedrijf stuurt waar ze al kennis van PHP hebben. Ik snap er al wel veel van, want het is niet echt gecompliceerde code. Dat terwijl ik helemaal geen expert ben op het gebied van PHP. Moet je nagaan wat die programmeurs op dat werk denken.

cyberjack77 schreef op vrijdag 30 augustus 2013 @ 16:47:
Dan nog een javascript puntje:
"<script Language="Javascript">" wordt al niet meer gebruikt sinds html 4.01
Tegenwoordig is dat "<script>" of "<script type="text/javascript">"

Ja, dat viel mij ook op.

cyberjack77 schreef op vrijdag 30 augustus 2013 @ 16:47:
Persoonlijk zou ik dan nog kijken naar (maar daar zou ik niet op letten bij aangeleverde code voor een sollicitatie)
- Namespaces als je gebruik maakt van php 5.3 of nieuwer
- Geen commentaar boven de functies / classen (is handig als je met een goede IDE werkt en kan documentatie van gegenereerd worden)
- Sommige functie namen hebben camelcase namen en sommige alleen lowercase ("isBanned" en "insertlog" bijvoorbeeld). Is niet consequent.

Waarom zijn dat geen onderdelen waar je niet zo zeer naar zou kijken bij een sollicitatie? Het geeft toch enigszins aan hoe iemand te werk gaat en als dat in lijn is met de andere programmeurs dan is dat toch fijn?

Ik ben geen programmeur, maar ik zou een consequente manier van werken van collega's wel waarderen.

[ Voor 40% gewijzigd door Bolletje op 30-08-2013 17:05 ]

[kwaakvaak_v2]

Bolletje schreef op vrijdag 30 augustus 2013 @ 16:57:

Waarom zijn dat geen onderdelen waar je niet zo zeer naar zou kijken bij een sollicitatie? Het geeft toch enigszins aan hoe iemand te werk gaat en als dat in lijn is met de andere programmeurs dan is dat toch fijn?

Ik ben geen programmeur, maar ik zou een consequente manier van werken wel waarderen van collega's.

Omdat dat afspraken zijn die van bedrijf tot bedrijf verschillen en aan te leren zijn a.d.h.v code guidelines etc. Dat zou voor mij ook geen reden zijn om iemand af te wijzen.

Het niet escapen van mysql, samen met de rest van de voorbeeld code is een hele goede reden om aan te nemen dat de programmeur in kwestie ergens in 1998 is blijven hangen en op basis van die aanname zou ik de TS ook niet aangenomen hebben.

Ik vind dat developers ook bij moeten blijven in hun vakgebied. Desnoods in eigen tijd als een werkgever er geen tijd voor wil vrij maken.

[DexterDee]

Om even in te haken op het belangrijkste punt van kritiek, de gelaagdheid van je code:

Je code mist een logische opbouw van entiteiten, relaties en verantwoordelijkheden. De manier waarop je op dit moment classes gebruikt is nog steeds heel procedureel. Een class is veel meer dan een "namespace" c.q. verzamelbak waarin je handige functies groepeert.

Het doel van objectgeoriënteerd programmeren is om middels een (hierarchisch) model van objecten bepaalde complexiteit te abstraheren en hergebruik en overerving van functionaliteit mogelijk en makkelijk te maken. Doordat objecten via een vaste interface met elkaar communiceren kun je een modulaire applicatie opzetten, waarbij onderdelen van de code gemakkelijk losgetrokken en vervangen kunnen worden.

Een veelgebruikt ontwerpmodel in PHP applicaties is het zogenaamde Model View Controller model (MVC). Hierbij scheid je de logica in classes die de data representeren (model), de gebruiksinterface (view) en de besturing c.q. reageren op events van de bezoeker (controller).

Als je al ervaring hebt in procedureel programmeren en je moet de stap maken naar objectgeoriënteerd programmeren, dan is de uitdaging niet het leren van de juiste PHP OOP syntax. Je moet een mindshift gaan maken in de manier waarop je je code structureert en hoe je bepaalde problemen oplost. Dat is zeker niet gemakkelijk. Door veel te oefenen en vooral te doen zul je zien dat dat steeds beter gaat. Een boek over objectgeoriënteerd programmeren kan je wellicht wat handvaten geven om die mindshift te maken.

[pedorus]

Even nog iets wat niet genoemd is: Een functie als check() lijkt me een mooi voorbeeld van hoe je niet een database moet gebruiken. De gehele functie kan in een enkel sql statement worden omgezet, wat daarnaast de boel efficiënter maakt, want dan hoef je niet een "select *" over een gehele tabel hoeft te doen en losse deletes te doen. "select *" is veelal sowieso geen goed idee, vraag gewoon specifieke kolommen op. De naam check() is overigens nietszeggend, goede naamgeving is erg belangrijk. Ook de functie update kan trouwens in een enkel mysql statement. Scheelt weer 2 queries.

Verder is het gebruik van witruimte niet erg consistent, en snap ik niet waarom je hier classes gebruikt aangezien de code procedureel is. Was classes gebruiken een opdracht?

[Creepy]

Nu is het eigenlijk niet de bedoeling om een topic als dit te open zonder dat je zelf aangeeft waar je denkt dat het aan ligt. Code check topics zien we liever niet maar als je zelf aangeeft wat jij denkt dat het probleem is dan moet dat wel kunnen. Dat doe je echter helemaal niet, dus ik ga je topic sluiten. Je hebt denk ik genoeg concrete punten om aan te werken. Los daarvan zou ik even blijven doorduwen bij het bedrijf zodat je precies weet waarom ze je hebben afgewezen.