Register Script Feedback

[Hedge]

I weighed up my options about what free engine (cannot afford a paid one) to use for my game, but decided that id learn more by trying to make my own game from scratch.

So ive had a go at making a register script, im after feedback on it.

I only know what ive learnt , and if something ive learnt is wrong, i probably don't relies. hence the request for feedback.

at this point in time, it im pretty sure it does what i set out to achieve. It doesn't throw any errors, and the information is inputted into the database.

 

<?php
session_start();
/**
* SynitiCity Copyright 2013
* Made by Hedge
* Page: register.php
* Version: 1.0
* Date: 28 May 2013 Updated: n/a
*/
require_once __DIR__ . '/header_public.php';
include_once ('../configs/conn.php');
include_once ('../includes/classes.php');

//information is retrived from public/index.php and cleaned.
$username = stripslashes(strip_tags(trim($_POST["Username"])));
$password = stripslashes(strip_tags(trim($_POST["Password"])));
$password2 = stripslashes(strip_tags(trim($_POST["Password2"])));
$email = $_POST["Email"];
$ip = $_SERVER['REMOTE_ADDR']; //retrives ip address
$class = $_POST["Class"];

if ($username === "") { //errors if username is empty
$error = "You did not enter a username.";
}

if ($password === "") { //errors if password if empty 
$error = "You did not enter a password.";
}

if ($password2 === "") { //errors if password if empty 
$error = "You did not enter a password.";
}

if ($password != $password2) { //errors if passwords do not match
$error = "Your passwords do not match.";
}

if (strlen($username) <= 4) { //errors if username is too short
$error = "Your username must be atleast 5 characters long.";
}

if (strlen($username) >= 21) { //errors if username is too long
$error = "Your username must not be longer than 20 characters.";
}

if (strlen($password) <= 5) { //errors if password is too short
$error = "Your password must be atleast 6 characters long.";
}

if (strlen($password2) <= 5) { //errors if password is too short
$error = "Your password must be atleast 6 characters long.";
}

if (strlen($password) >= 21) { //errors if password is too long
$error = "Your password must not be longer than 20 characters.";
}

if (strlen($password2) >= 21) { //errors if password is too long
$error = "Your password must not be longer than 20 characters.";
}

if (is_email($email) === FALSE) { //errors if the email is in an invalid format
$error = "Please enter a valid email address.";
}

//query to check if username is already taken by another player. checks both username and loginname.
$username_query = $mysqli->query("SELECT `userID` FROM `users` WHERE `username` = '$username' OR `loginname` = '$username'");
$username_rows = $username_query->num_rows;
if ($username_rows > 0) {
$error = "Sorry that username is already in use, please choose another.";
$username_query->close();
}

//query to check if the email has already been used before.
$email_query = $mysqli->query("SELECT `userID` FROM `users` WHERE `email` = '$email'");
$email_rows = $email_query->num_rows;
if ($email_rows > 0) {
$error = "Sorry that email address has already been used to create an account.";
$email_query->close();
}

//query to check if the users ip address has already been used to create an account.
$ip_query = $mysqli->query("SELECT `userID` FROM `users` WHERE `ip` = '$ip'");
$ip_rows = $ip_query->num_rows;
if ($ip_rows > 0) {
$error = "Sorry but your IP address has already been used to create an account.<br>If you havent created an account before, please email the staff and an account will be created for you.<br>[email protected]";
$ip_query->close();
}

if (isset($error)) { //if an error was thrown from above, it is outputted here and the page is stopped.
echo Error($error);
require_once __DIR__ . '/footer_public.php';
die();
} else {
$password = crypt($password, $username); //password encrypted using the chosen username as a salt.
//query is run to add the user to the database.
$mysqli->query("INSERT INTO `users` (`loginname`, `username`, `password`, `email`, `ip`, `class`) VALUES ('$username', '$username', '$password', '$email', '$ip', '$class')");

echo Success("Your account has been created, please wait while you are logged in.");
//automaticlly log user in upon registration.
$login = $mysqli->query("SELECT `userID` FROM `users` WHERE `loginname` = '$username'");
$id = $login->fetch_array(MYSQLI_ASSOC);
$_SESSION['loggedin'] = $id['userID'];
$login->close();
echo "<meta http-equiv='refresh' content='5;URL=index.php'>";
}
require_once __DIR__ . '/footer_public.php';
?>

[HauntedDawg]

Ok, Scoping through your script. I see a couple issues.

No one wants to need to include all of this each time you create a new php page:

 

<?php
session_start();
/**
* SynitiCity Copyright 2013
* Made by Hedge
* Page: register.php
* Version: 1.0
* Date: 28 May 2013 Updated: n/a
*/
require_once __DIR__ . '/header_public.php';
include_once ('../configs/conn.php');
include_once ('../includes/classes.php');

 

While the include & session start can go in one file, and include that one file which does it all for you. It is a good practice that you made commenting at the top of the script.

The way you are showing an error, is slightly wrong, although it can be done like that, it's not what you would like to achieve.

I'll put it like this.. I leave my name blank this script runs:

 

if ($username === "") { //errors if username is empty
   $error = "You did not enter a username.";
}

 

Presumably, Id now see "You did not enter a username." message, but that's not the case if i made my password longer than 21 characters.

The reason being is that you keep overwriting your $error variable each time a new error message must occur. You could do this:

 

    $error .= 'Error message here';

 

But you would need line breaks per line to break it up for you, or alternatively we want to create an array as such:

 

$error = array();

if ($username === "") { //errors if username is empty
   $error[] = "You did not enter a username.";
}

 

When we want to display that, we will implode it, with a line break as such:

 

echo implode('<br />', $error);

 

There are many other points where you can improve. I would suggest looking into how to actually use Mysqli to its full potential, as you clearly don't use it:

 

$mysqli->query("INSERT INTO `users` (`loginname`, `username`, `password`, `email`, `ip`, `class`) VALUES ('$username', '$username', '$password', '$email', '$ip', '$class')");

 

Becomes

 

$insert = $mysqli->prepare("INSERT INTO `users` (`loginname`, `username`, `password`, `email`, `ip`, `class`) VALUES (?, ?, ?, ?, ?, ?)");
$insert->bind_param('ssssss', $username, $username, $password, $email, $ip, $class);
$insert->execute();
$insert->close();

 

Which essentially, becomes a lot more secure.

Edited May 30, 2013 by HauntedDawg

[Hedge]

Thank you HD, that's exactly the type of feed back i need.

[Aventro]

If you're going to make a game, I'd suggest use a framework which gives you a solid architecture to work your application on. There's no need to reinvent that unless you're doing it properly. This code snippet contains many bad practices, a bad architecture, code that could be redundant a lot. I could point all this out, but I'd rather tell you to have a look at:

http://symfony.com/doc/current/book/from_flat_php_to_symfony2.html

And then you go right in to learning http://www.laravel.com/ or http://www.ezrpgproject.net/

 

Please don't see this as some negative criticism on that you shouldn't do this, learning by doing is very good, but If you want to use it in a production game I wouldn't recommend doing so.

[Hedge]

Thanks for your input Aventro, but im not really looking to use a framework or game engine.

I understand the benefits of frameworks, as ive read up on all the ones i could find, but in reality its just one more thing to add to the list of s**t the confuses the hell out of me.

At this stage, i think until i have a better understanding of php, im better off making it this way and learning as much as possible along the way.

As for ezrpg, i want to try to make a game thats as different as it can be from all the others out there, so im staying away from free engines at this point in time.

If i fail miserably at my attempt, then i will rethink my approach