Am I organising my PHP code effectively?

Posted on

Problem

I’m very new to learning PHP & MySQL (I’ve got past experience with Java) and I’m doubting whether my code is organised well. I’ve got an index page which has two forms; the first is a form to login and the second is a form to sign up a user. So far I’ve only completed the signing up of a user. Please comment on my code in general and what I can improve.

The form (table tags removed):

<form action='php/signup.php' method='post' name='signup'>
    <input name='username' type='text' />
    <input name='password' type='password' />
    <input name='signin' type='submit' value='Sign up' />
</form>

signup.php:

<?php
try {
    $db = new PDO(...);
    $db -> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    foreach ($db->query("SELECT username FROM usertable") as $row) {
        if ($row == $username) {
            header("Location: http://example.com/#");
            return;
        }
    }
    $user = $db->prepare("INSERT INTO users (username, password) VALUES (:username, :password)");
    $user->execute(array(":username" => $_POST["username"], ":password" => $_POST["password"]));
    $db = null;
    header("Location: http://example.com/anotherpage.html");
} catch ( PDOException $e ) {
    echo $e -> getMessage();
}
?>

Solution

  1. Iterating through the whole user table could be ineffective:

    foreach ($db->query("SELECT username FROM usertable") as $row) {
    

    You could use a WHERE username LIKE :username condition here. (And you might need a database index for the attribute as well)

  2. The code does not check the password. I guess you should do that.

  3. You should hash your passwords.

  4. Calling a table usertable is a little bit redundant. I’d use simply users.

  5. The following is hard to read because it needs horizontal scanning:

    $user->execute(array(":username" => $_POST["username"], ":password" => $_POST["password"]));
    

    I would introduce an explanatory variable and some line breaks:

    $newuser_parameters  = array(
        ":username" => $_POST["username"], 
        ":password" => $_POST["password"]
    );
    $user->execute($newuser_parameters);
    

    From Code Complete, 2nd Edition, p761:

    Use only one data declaration per line

    […]

    It’s easier to find specific variables because you can scan a single
    column rather than reading each line. It’s easier to find and fix
    syntax errors because the line number the compiler gives you has
    only one declaration on it.

    See also: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler; Clean Code by Robert C. Martin, G19: Use Explanatory Variables.

  6. I would validate at least the maximum length of the username.

  7. This probably isn’t too useful for your clients:

    echo $e -> getMessage();
    

    See #3 in my former answer.

Leave a Reply

Your email address will not be published. Required fields are marked *