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
-
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) -
The code does not check the password. I guess you should do that.
-
You should hash your passwords.
-
Calling a table
usertable
is a little bit redundant. I’d use simplyusers
. -
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.
-
I would validate at least the maximum length of the username.
-
This probably isn’t too useful for your clients:
echo $e -> getMessage();
See #3 in my former answer.