Inserting data in the database through POST

Question

My code here is completely working, but I feel like I destroyed or didn't follow the DRY rule, what suggestions can you give to me for this code??

<?php require_once("./includes/Utilities.php") ;?>
<?php require_once("./includes/Db_Resources/db_connection.php");?>
<?php require_once("./includes/Db/DatabaseUtilities.php"); ?>
<?php 
    if(isset($_POST['submit'])){
        require_once("./includes/process_form.inc.php");
        require_once('./includes/Db/CheckPassword.php');
        require_once('./includes/Db/CheckUsername.php');
        $check_pwd = new Db_CheckPassword($password);
        $check_user = new Db_CheckUsername($username,$conn);
        $passwordOK = $check_pwd->check();//Checks if Password is valid 
        $userOK = $check_user->check();//Checks if Username is valid 
        //Checks if Password is the same with the retyped password
        $password_match = Db_CheckPassword::confirmPassword($password,$conf_pass);

        //if everything is okay! we'll finally add the muthafucking users account
        if($userOK && $passwordOK && $password_match){

            if(Db_DatabaseUtilities::registerAccount($conn,$username,$password,$email)){
                header("Location: login.php");
                exit;
            }

        }else{
            $pass_error =  $check_pwd -> getErrors();
            $user_error  = $check_user->getErrors();
        }
    }
?>

Answer 1

Here are some things that I would recommend:

• PHP tags. There's no point in closing and reopening PHP tags if there is no non-PHP code in between. For this file, a single opening PHP tag is all you need for the entire thing. Closing PHP tags (anywhere in the file) can bring potential issues such as unnoticed white space, which will throw errors if you try to use header() or similar later on.
• header() - When using header('Location: xxxxx');, you should use a full URL, not a relative URL, since it is making an HTTP request. Sure, it probably works most of the time, but let's follow the spec and not take chances.
• isset($_POST['submit']) - Some versions of Internet Explorer (not sure when this was fixed) will ignore the name/value of the submit button if it is being used as an image (example: <input type="image" src="login.png" name="submit" value="Login" />). I would check for a different form element (or even the $_POST array itself) to eliminate that potential bug.
• Required files - Since I don't know how the rest of your application is structured, this one is more speculation. But if you find yourself repeating these required lines often in different files, you should consider consolidating the require_once() lines to a single appropriate file. For instance, all of your database connections can be done in once place. All of the form validation files can be consolidated, also. This leaves you with only needing to do a single require_once() in your app's files, and only needing to maintain and update a single file elsewhere if you want to change or add functionality.
• registerAccount() failure? - You perform a redirect if the registerAccount() method is successful, but don't do anything if it fails. Always have an action for when things fail, because they probably will at some point. Logging error messages, even sending yourself an email when it happens is a good way to notice a major error quickly in the event that it happens.

This one is more personal preference, but might be worth it to you:

• I would rename the check() method in your user and password classes to isValid(), since that's what you're specifically looking for (even says so in the comments). I always try to name methods based on context. You could even use the methods directly in your if statement - no real point in assigning those variables, since you only use them once and they don't really save much typing later on.

I hope that helps. :)

Answer 2

One minor thing to add to Cryode's answer:

Db_DatabaseUtilities::registerAccount($conn,$username,$password,$email)

This is either named badly, or likely violates the single responsibility principle.

You should consider moving it into some kind of Account model (or pseudo-model), or at least trying to separate it from general DB utilities items.

Db_DatabaseUtilities is going to create all kinds of dependencies throughout your code, and worse, since it's being called statically, there's no potential to inject the dependency. (Well, technically you could, but it would get very, very ugly.)