patternphpMinor
Am I organising my PHP code effectively?
Viewed 0 times
codephporganisingeffectively
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):
signup.php:
The form (table tags removed):
signup.php:
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:
You could use a
-
The code does not check the password. I guess you should do that.
-
You should hash your passwords.
-
Calling a table
-
The following is hard to read because it needs horizontal scanning:
I would introduce an explanatory variable and some line breaks:
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:
See #3 in my former answer.
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 simply users.-
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.
Code Snippets
foreach ($db->query("SELECT username FROM usertable") as $row) {$user->execute(array(":username" => $_POST["username"], ":password" => $_POST["password"]));$newuser_parameters = array(
":username" => $_POST["username"],
":password" => $_POST["password"]
);
$user->execute($newuser_parameters);echo $e -> getMessage();Context
StackExchange Code Review Q#45234, answer score: 3
Revisions (0)
No revisions yet.