Am I organising my PHP code effectively?

Question

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();
}
?>

Answer 1

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.

• Someone: Regarding #2, #3, #6 and #7, I'm too concerned about them because this is for a basic assignment, though I'll implement all of your suggestions anyway. Regarding #1, I was thinking that my implementation wasn't efficient. Thanks a lot. Oh and regarding #5, I'm too used to pressing CRTL-F in eclipse...
• palacsint: @Someone: I'm glad that it helped. #5: Autoformat is great, I guess you can configure it.
• Cerad: Just one minor hint: You can drop the colons from your user parameter array and it still works fine. array('username' => ... Still use the colon in your prepared statement sql.