Need some feedback on my code. What's good, what's bad and so on

Question

First of all you'll have to excuse my horrible coding skills as I've been studying PHP for ~1 week now. Following the advice of people who know better than me, I ask for review so that I don't "learn to write bad code from the beginning". Long story short, here's the code. Any advice is welcomed. What can be improved and so on.

<!DOCTYPE html>
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>Register</title>
    </head>
    <body>      
        <form name="input" action="" method="POST" >
            Username:           <br>    <input type="text" name="user"><br>
            Password:          
                                <br>    <input type="text" name="pass"><br>    
            Retype password:    
                                <br>    <input type="text" name="rpass"><br>
            Email:              <br>    <input type="text" name="email"><br>
            <input type="submit" name="submit" value="Register">
        </form>
    </body>

<?php


if(isset($_POST['submit'])){

    //get values from FORM
    $user = $_POST['user'];
    $pass = $_POST['pass'];
    $rpass = $_POST['rpass'];
    $email = $_POST['email'];
    $email_regex = '/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$/';

    //check if either field is empty
    if (empty($user) || empty($pass) || empty($rpass) || empty($email)){
        echo '<font size="3" color="red">You must fill in all fields.</font><br>';
        die;
    }

    //check if files exist
    if(!require 'functions.php'){
        die('File not found!');
    }

    $_regError = new registerErrors;

   //Assign checks to credentials
   $credentials = array(
                        'username_valid' => preg_match("/^[a-z0-9._-]*$/i", ($user)),
                        'pass_match' => ($pass === $rpass),
                        'pass_len' => ($pass < 6),
                        'email_valid' => preg_match($email_regex,  $email)
    );

    $errors = array();

    foreach($credentials as $key=>$isvalid){
        if(!$isvalid){
            switch($key){
                case 'username_valid':
                            $errors [] = $_regError -> userInvalid();
                    break;
                case 'pass_match':
                            $errors [] = $_regError->passwordNoMatch();
                    break;
                case 'pass_len':
                            $errors [] = $_regError -> passwordNoMatch();
                    break;
                case 'email_valid':
                            $errors [] = $_regError -> emailInvalid();
                    break;    
            }

        }        

    //foreach ends here
   }

    if(count($errors) == 0){
        $db = new PDO('mysql:host=localhost;dbname=xxxxt', 'xxx', 'xxxx');
        if(!$db){
            echo 'Unable to connect to database.';
            die;
        } 

        $pass_hash = new hashPassword();
        $hash = $pass_hash->hashPass($pass);

        $query_user = $db->prepare("SELECT * FROM users WHERE `user` =?");
        $query_email = $db->prepare("SELECT * FROM users WHERE `email` =?");
        $query_user->execute(array($user));
        $query_email->execute(array($email));
        if(($query_user->rowCount() > 0) || ($query_email->rowCount() > 0)){
            $_regError->userExists();           
        }else{
            $query = $db->prepare("INSERT INTO users (user, pass, email) VALUES ('$user', '$hash', '$email')");
            $query->execute();

            echo '<font size="3" color="red">Registration succesful!<br>';


        }

     //count errors ends here
    }

//check (!submit) ends here
}

?>

<?php

class registerErrors{
    public function passwordNoMatch(){
        echo '<font size="3" color="red">Passwords must match and be at least 6 characters in lenght.</font><br>';
        die();
    }
    public function emailInvalid(){
        echo '<font size="3" color="red">The Email is invalid.<br>'; 
        die();
    }
    public function userInvalid(){
        echo '<font size="3" color="red">The Username is invalid.<br>';
        die();
    }
    public function userExists(){
        echo '<font size="3" color="red">The Username or email already exists.<br>';
    }

//registerErrors ends here
}

class hashPassword{
    public function hashPass($pass){  
        //crypt password ! CHANGE THIS TO BCRYPT WHEN YOU GET THE TIME !
        $salt = substr($pass, (strlen($pass) / 2));
        $hash = MD5($pass, $salt );
        return $hash;
    } 
}



?>

About comments : use keywords like TODO, HACK, FIXME, OPTIMIZE to mark something as unfinished. Eg. : //crypt password ! CHANGE THIS TO BCRYPT WHEN YOU GET THE TIME ! ----> // crypt password ; FIXME use bcrypt instead of md5
I don't think that functions in class registerErrors should call die(). They should return an error message instead. They should not call echo but return the error string.
Also, try using css class as soon as possible instead of hard coded styling
When using a DB query, if you only need to know the number of rows instead of the actual data, use SELECT COUNT(*), not SELECT *
Queries "query_user" and "query_email" can easily be merged into one sigle query.

mnhg · Answer 1 · 2013-01-18 13:58:15Z

I restructured your code a little and added some comments. Of course you could also separate the layout from the logic and use a more OOP approach, but I guess it will meat your requirements. (Please also consider sfk's comments) Feel free do ask if any of my changes feels somehow strange.

<!DOCTYPE html><!--is this valid?-->
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>Register</title>
    </head>
    <body> 
        <?
        //You don't want to display your error at the end.
            //Maybe we should also find a nicer way to handle the success ...
        $errors=handleSubmit()
        if (count($errors>0) {
            ?><ul><?
            for ($errors as $error)
            {
                ?><li><font size="3" color="red"><?=$error?></font></li><?
            }
            ?></ul><?
        }
        ?>
        <form name="input" action="" method="POST" >
            Username:           <br>    <input type="text" name="user"><br>
            Password:          
                                <br>    <input type="text" name="pass"><br>    
            Retype password:    
                                <br>    <input type="text" name="rpass"><br>
            Email:              <br>    <input type="text" name="email"><br>
            <input type="submit" name="submit" value="Register">
        </form>
    </body>
</html><!--closing HTML missing//-->

<?php


function handleSubmit()
{
    if(!isset($_POST['submit'])) return null;

    // Use comments to give hints regarding the intention. Use methods to group and name code segments

    $user = $_POST['user'];
    $pass = $_POST['pass'];
    $rpass = $_POST['rpass'];
    $email = $_POST['email'];

    $errors=validateInput($user,$pass,$rpass,$email);
    if ($errors!=null) return $errors;

    $db = new PDO('mysql:host=localhost;dbname=xxxxt', 'xxx', 'xxxx');
    if(!$db) return array('Unable to connect to database.');

    $errors=validateExistence($user,$email);
    if ($errors!=null) return $errors;

    require 'HashGenerator.php' // will die anyway if the file not exists

    $generator = new HashGenerator();
    $hash = $generator->hashPass($pass);

    $query = $db->prepare("INSERT INTO users (user, pass, email) VALUES ('$user', '$hash', '$email')");
    $query->execute();

    return array('Registration succesful!');
}

function validateInput($user,$pass,$rpass,$email)
{
    if (empty($user) || empty($pass) || empty($rpass) || empty($email)) return array("You must fill in all fields.");

    $errors=array();
    if (!preg_match("/^[a-z0-9._-]*$/i", ($user)) $errors[]="The Username is invalid.";
    if ($pass != $rpass) $errors[]="Passwords must match";
    if ($pass < 6) $errors[]="Passwords must be at least 6 characters in lenght.";
    $email_regex = '/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$/';
    if (!preg_match($email_regex,  $email)) $errors[]="The Email is invalid.";

    return $errors;
}

function validateExistence($user,$email)
{
    $query_user = $db->prepare("SELECT * FROM users WHERE `user` =?");
    $query_email = $db->prepare("SELECT * FROM users WHERE `email` =?");
    $query_user->execute(array($user));
    $query_email->execute(array($email));
    $errors=array();
    if($query_user->rowCount() > 0)  $errors[]="The Username already exists. ";
    if($query_email->rowCount() > 0) $errors[]="The email already exists. ";
    return $errors; 
}

?>

HashGenerator.php (every class in a separate file with the class name in the file name )

<?php
class HashGenerator{ //best practice is a capital starting letter
    public function hashPass($pass){  
        //TODO crypt password ! CHANGE THIS TO BCRYPT WHEN YOU GET THE TIME !
        $salt = substr($pass, (strlen($pass) / 2));
        $hash = MD5($pass, $salt );
        return $hash;
    } 
}

<!DOCTYPE html> is valid in every single modern browser. It's HTML5.

asked	4 months ago
viewed	121 times
active	6 days ago

Need some feedback on my code. What's good, what's bad and so on

1 Answer

Your Answer

Not the answer you're looking for? Browse other questions tagged php or ask your own question.

Need some feedback on my code. What's good, what's bad and so on

1 Answer

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php or ask your own question.

Related