Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    } 
}



?>
share|improve this question
    
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 –  sfk Jan 18 '13 at 12:14
1  
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. –  sfk Jan 18 '13 at 12:17
    
Also, try using css class as soon as possible instead of hard coded styling –  sfk Jan 18 '13 at 12:18
1  
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 * –  sfk Jan 18 '13 at 12:28
    
Queries "query_user" and "query_email" can easily be merged into one sigle query. –  sfk Jan 18 '13 at 12:30
show 2 more comments

1 Answer 1

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;
    } 
}
share|improve this answer
    
<!DOCTYPE html> is valid in every single modern browser. It's HTML5. –  Amelia May 20 '13 at 13:19
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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