Registration Validation

[LSCare]

This is a bit of a lazy post but hey,

Currently i have an awful lot of checks that run the same check on different variables.

Simple Request... Can anyone see an obvious way (which i have clearly missed) to clean up and reduce the amount of code in the below.

<?

// required code for every page.

require($_SERVER['DOCUMENT_ROOT'] . '/scripts/required.php');

// end of required code for every page

if(isset($_POST['sbmt']))
	{
		// get variables
		$new_user = trim($_POST['user']);
		$new_pass = $_POST['newpass'];
		$con_pass = $_POST['conpass'];
		$new_fname = trim($_POST['fname']);
		$new_email = $_POST['email'];
		$con_email = $_POST['conemail'];
		$agree = $_POST['agree'];
		$level = $_POST['level'];
		$ip = @$_SERVER['REMOTE_ADDR'];
		
		// add slashes
		$new_user = addslashes($new_user);
		
		// encrypt
		$new_pass_e = hash('ripemd160',$new_pass);
		$con_pass_e = hash('ripemd160',$con_pass);
		
		// check username exists
		// connect to database
		$con = mysql_connect(DBHOST, DBUSER, DBPASS);
		if(!$con) { die('Could not connect: ' . mysql_error());}
		mysql_select_db(DBNAME, $con);
	
		// get row for user
		$result = mysql_query("SELECT * FROM tUsers WHERE username='$new_user'");
		$row = mysql_fetch_array($result);
	
		// kill connection
		mysql_close($con);
		
		// check username exists
		if($row['username']!=null)
			$user_error = 'Username already in use.<br />';
		else
			$user_error = null;

		// check email is available		
		// connect to database
		$con = mysql_connect(DBHOST, DBUSER, DBPASS);
		if(!$con) { die('Could not connect: ' . mysql_error());}
		mysql_select_db(DBNAME, $con);
	
		// get row for user
		$result = mysql_query("SELECT * FROM tUsers WHERE email='$new_email'");
		$row = mysql_fetch_array($result);
	
		// kill connection
		mysql_close($con);
		
		// check email is available
		if($row['email']!=null)
			$email_error = 'Email already in use.<br />';
		else
			$email_error = null;
		
		// check lengths
		if(strlen($new_user)<3)
			$user_len_error = 'Username must be at least 3 characters<br />';
		else
			$user_len_error = null;
		if(strlen($new_pass)<6)
			$pass_len_error = 'Password must be at least 6 characters<br />';
		else
			$pass_len_error = null;
		if(strlen($new_fname)<6)
			$fname_len_error = 'Your name must be at least 6 characters<br />';
		else
			$fname_len_error = null;
		
		// preg matches
		if(!preg_match('/^[a-zA-Z0-9_]+([.][a-zA-Z0-9_]+)*[@][a-zA-Z0-9_]+([.][a-zA-Z0-9_]+)*[.][a-zA-Z]{2,4}$/', $new_email)) 
			$email_preg_error = "Email is not valid.<br />";
		else
			$email_preg_error = null;
		if(!preg_match('/[a-zA-Z]+\s+[a-zA-Z]+/',$new_fname))
			$fname_preg_error = 'Name must containt at least one space.<br />';
		else
			$fname_preg_error = null;
			
		// check matches
		if($new_pass != $con_pass)
			$pass_match_error = 'The passwords do not match.<br />';
		else
			$pass_match_error = null;
		if($new_email != $con_email)
			$email_match_error = 'The emails do not match.<br />';
		else
			$email_match_error = null;
		
		// build error list
		$errorlist = $user_error;
		$errorlist .= $email_error;
		$errorlist .= $user_len_error;
		$errorlist .= $pass_len_error;
		$errorlist .= $fname_len_error;
		$errorlist .= $email_preg_error;
		$errorlist .= $fname_preg_error;
		$errorlist .= $pass_match_error;
		$errorlist .= $email_match_error;
		
		// set expirey to a month
		$expire = 0;
								
		// set error list cookie
		setcookie('errorlist',$errorlist,$expire,'/');
		
		// reload register.php
		header('Location:' . URL . '/register.php');
		
	}
	
?>

The required file is just a series of defines for items like the dbhost, dbusername, dbname, url etc etc.

Cheers

p.s.

I want to add an if statement for if $errorlist=null then do bla else bla... in order for errorlist to be null... do i actually need to define the individual error messages as null.

That could get rid of a lot of:


else
$<error name>=null;


I'm pretty sure I can but i'd like to check first.

[bullant]

Simple Request... Can anyone see an obvious way (which i have clearly missed) to clean up and reduce the amount of code in the below.

One thing you could shorten is changing

PHP Code:

<?php

// get row for user
$result = mysql_query("SELECT * FROM tUsers WHERE username='$new_user'");
$row = mysql_fetch_array($result);

// kill connection
mysql_close($con);

// check username exists
if ($row['username'] != null)
    $user_error = 'Username already in use.<br />';
else
    $user_error = null;
?>

to simply this

PHP Code:

<?php

$result = mysql_query("SELECT * FROM tUsers WHERE username='$new_user'");
$user_error = (mysql_num_rows($result) == 1)? 'Username already in use.<br />' : null;

?>

Assuming usernames are unique then you only need to check if the number of rows in $result = 0 or 1 to determine if the username is already taken.

Also, I'm not sure why you close the db connection above and then reopen it further down to check the email address. Instead of closing the connection, you could just free the result set using mysql_free_result(). Just closing the db connection still leaves the data in $result in RAM.

With your error messages, you could store them in an array something like this:

PHP Code:

<?php

$errors = array();

// then as you validate each user input, if an input is invalid you could add 
// an appropriate message to the array like

$errors[] = 'Usrname already taken';

// and when all user inputs have been validated
if(count($errors) > 0) { //errors occured
   //do something
} else {  //all user inputs are valid
   //do something else
}
?>

btw: I think this thread should be in a php forum.

[LSCare]

Originally Posted by bullant

btw: I think this thread should be in a php forum.

indeed it should... i thought it was! Whoops

I have not before seen the application of the below syntax...

PHP Code:

<?php

$result = mysql_query("SELECT * FROM tUsers WHERE username='$new_user'");
$user_error = (mysql_num_rows($result) == 1)? 'Username already in use.<br />' : null;

?>

specifically the bottom line... mind explaining it to me?

[LSCare]

Originally Posted by bullant

Also, I'm not sure why you close the db connection above and then reopen it further down to check the email address. Instead of closing the connection, you could just free the result set using mysql_free_result()

And this is just one way i have in place to ensure i close connections properly and to ensure i section my code appropriately... Also allows code to be taken in chunks and quickly applied elsewhere.

=)

[bullant]

The bottom line is a ternary (scroll down a bit in the link to get to description).

In summary, it's similar to an

PHP Code:

if(test_condition){
   // assign one value
} else {
   // assign another value
} 


[bullant]

Originally Posted by LSCare

And this is just one way i have in place to ensure i close connections properly and to ensure i section my code appropriately... Also allows code to be taken in chunks and quickly applied elsewhere.

no problem

but normally I just open a connection at the top of the script and at the bottom I run mysql_close() and mysql_free_result()

[LSCare]

Cheers for that link... Could save a fair bit of time. Surprised I've not run into it before on various code snatching exorcises.

[bullant]

you're welcome