Return to Answer

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)

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

2. You should hash your passwords.

1. Calling a table usertable is a little bit redundant. I'd use simply users.

2. 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.

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

2. This probably isn't too useful for your clients:

echo $e -> getMessage();

See #3 in my former answer.

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)

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

2. You should hash your passwords.

1. Calling a table usertable is a little bit redundant. I'd use simply users.

2. 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.

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

2. This probably isn't too useful for your clients:

echo $e -> getMessage();

See #3 in my former answer.

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)

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

2. You should hash your passwords.

1. Calling a table usertable is a little bit redundant. I'd use simply users.

2. 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.

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

2. This probably isn't too useful for your clients:

echo $e -> getMessage();

See #3 in my former answer.

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)

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

2. You should hash your passwords.

1. Calling a table usertable is a little bit redundant. I'd use simply users.

2. 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.

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

2. This probably isn't too useful for your clients:

echo $e -> getMessage();

See #3 in my former answer.