homepage Welcome to WebmasterWorld Guest from 54.211.138.180
register, free tools, login, search, subscribe, help, library, announcements, recent posts, open posts,
Subscribe to WebmasterWorld

Home / Forums Index / Code, Content, and Presentation / PHP Server Side Scripting
Forum Library, Charter, Moderators: coopster & jatar k

PHP Server Side Scripting Forum

    
Cleaner - professional code
adammc




msg:3708856
 11:34 pm on Jul 27, 2008 (gmt 0)

Hi Guys,

I have self taught myself PHP / MYSQL over the past year, learning just enough and what I need to for each job that comes along.

Can I please get some constructive criticism on the following code ?

Am I doing things correctly / securely enough, the least time consuming way ?

[php]
// check if the form was submitted
if(isset($_POST['submitted']))
{

// Show errors, if any
ini_set ('display_errors', 1);
error_reporting (E_ALL & ~E_NOTICE);

include 'db-connect.php';

// filter posted variables to guard against header injection
function cleaner($data)
{
if(is_array($data))
{
$ret = array();
foreach($data as $key=>$value)
{
$ret[$key] = cleaner($value);
}
return $ret;
}
else
{
if(!is_numeric($data))
{
if(get_magic_quotes_gpc())
{
$data = stripslashes($data);
}
$data = mysql_real_escape_string($data);
}
return $data;
}
}

$clean = cleaner($_POST);
// we then need to access each POSTED variable like this: $clean['name']

// declare the variables that werent required (by the form) and or declare integars that didnt need to be cleaned
$security = $_POST['security'];
$job_description = $clean['job_description'];
$vet_reg_num = $clean['vet_reg_num'];
$fax_number = (int) $_POST['fax_number'];
$type = $clean['type'];

// format the checkbox data - seperate with commas
foreach($clean['services'] as &$value) {
$value="".$value."";
}
$services=implode(", ", $clean['services']);

// Initialize error array.
$errors = array();

if(empty($clean['username'])){
$errors[] = 'A username is required and must be alpha-numeric.';
}

// validate the username
$username = $clean['username'];
if (!preg_match('/^[a-z\d_]{4,28}$/i', $username)) {
$errors[] = 'A username is required and must be alpha-numeric.';
}

// Make sure the username is available.
include 'db-connect.php';

$query = "SELECT username FROM Members WHERE username='$username'";
$result= mysql_query($query) or die(mysql_error());

if (mysql_num_rows($result)) {
$errors[] = 'Sorry, that username is already in use.';
}
mysql_close($db);

// Check for a password and match against the confirmed password.
if (eregi ("^[[:alnum:]]{4,20}$", stripslashes(trim($clean['password'])))) {

if ($clean['password'] == $clean['password_confirm']) {
$password = $clean['password'];
} else {
$errors[] = 'Your password did not match the confirmed password!';
}
} else {
$errors[] = 'Your password must be alpha-numeric.';
}

if(empty($clean['password_confirm'])){
$errors[] = 'Please confirm your password.';
} else{
$password_confirm = $clean['password_confirm'];
}

if(empty($clean['title'])){
$errors[] = 'Please enter your title.';
} else{
$title = $clean['title'];
}

if(empty($clean['first_name'])){
$errors[] = 'Please enter your first name.';
} else{
$first_name = $clean['first_name'];
}

if(empty($clean['last_name'])){
$errors[] = 'Please enter your last name.';
} else{
$last_name = $clean['last_name'];
}

if(empty($clean['occupation'])){
$errors[] = 'Please enter your occupation.';
} else{
$occupation = $clean['occupation'];
}

if(empty($clean['prac_name'])){
$errors[] = 'Please enter your practice name.';
} else{
$prac_name = $clean['prac_name'];
}

if(empty($clean['prac_type'])){
$errors[] = 'Please enter your practice type.';
} else{
$prac_type = $clean['prac_type'];
}

// Check for an email address & make sure there are no errors.
$email_address = $clean['email_address'];
if (empty($email_address))
{
if (!eregi("^.+@.+\\..+$", $email_address))
{
$errors[] = 'Your email address contains errors.';
}
}

// Make sure the email address is available.
include 'db-connect.php';

$query = "SELECT email FROM Members WHERE email='$email_address'";
$result= mysql_query($query) or die(mysql_error());

if (mysql_num_rows($result)) {
$errors[] = 'Sorry, that email address is already in use.';
}
mysql_close($db);

if(empty($clean['phone_number']) && ($clean['mobile_number'])){
$errors[] = 'Please enter a phone number';
} else{
$phone_number = $clean['phone_number'];
$mobile_number = $clean['mobile_number'];
}

if(empty($clean['street'])){
$errors[] = 'Please enter a street name.';
} else{
$street = $clean['street'];
}

if(empty($clean['city'])){
$errors[] = 'Please enter a city.';
} else{
$city = $clean['city'];
}

if(empty($clean['state'])){
$errors[] = 'Please select a state.';
} else{
$state = $clean['state'];
}

if(empty($clean['postcode'])){
$errors[] = 'Please enter your postcode.';
} else{
$postcode = $clean['postcode'];
}

if(empty($clean['country'])){
$errors[] = 'Please enter your country.';
} else{
$country = $clean['country'];
}

// Check the security field has been answered correctly
if(($security) !== "2"){
$errors[] = "You answered the security question wrongly. 1 + 1 = 2)";
}

// If everything went okay and there were no errors, continue.
if (empty($errors)) {

// format the date & time
$now = time();
$thisYear = date("Y-m-d H:i:s", $now);

// Create the activation code
$a = md5(uniqid(rand(), true));

require 'db-connect.php';

$sql = "INSERT into Members (
username,
pass,
title,
first_name,
last_name,
occupation,
job_description,
vet_reg_num,
email,
prac_name,
street,
city,
state,
postcode,
country,
phone,
mobile,
fax,
type,
services,
activation_code,
date_reg)
VALUES (

'$username',
SHA('$password'),
'$title',
'$first_name',
'$last_name',
'$occupation',
'$job_description',
'$vet_reg_num',
'$email_address',
'$prac_name',
'$street',
'$city',
'$state',
'$postcode',
'$country',
'$phone_number',
'$mobile_number',
'$fax_number',
'$prac_type',
'$services',
'$a',
'$thisYear') ";

mysql_query($sql) or die(mysql_error());

echo 'data added!';

mysql_close($db);
exit();

// if errors array contains a value
} else {
echo '<table align="center"><tr><td><b>The following error(s) occurred:</b><br /><blockquote>';
foreach ($errors as $msg) {
echo "* $msg<br />\n";
}
echo '</font></blockquote></td></tr></table><br /><br />';
}


// if the form wasn't submitted - display the reg form
}

[/php]

 

eelixduppy




msg:3709482
 6:09 pm on Jul 28, 2008 (gmt 0)

I only looked at this very quickly, however, there are few things that I would do to make this neater and more "professional". The first is that your function declarations should be separate from this file entirely. I know you only have one function here, however, if you ever needed this function in another script you'd have to rewrite the entire thing, which is something coders don't like to do, plus it's sloppy. So declare functions related to a certain task in one file, so for this instance, let's say you define the cleaner() function in a file called "db-func.php". Then you would require it in the scripts that you needed to use db-specifics functions in.

The second thing I saw is all the instances of if(empty($clean[...])) { are really space consuming and can probably be easily done with a loop through all of the post variables. I would have made an array of error messages in case each was empty, and looped through the array of POSTed variables, and if one was empty then used the respective error message from an error-msg array defined earlier. Again, if you have functions or code specific to checking for certain things in the validation process, you might want to make place those functions (or if they aren't functions make them into functions) and place them in a separate file, as well, called, for example, "validate-func.php" and require them into your script again.

Last, but probably not least, is you are including the db-connect.php script multiple times within this script which is not necessary at all. After each query that you run you are closing the link between mysql and php by calling mysql_close. You do not need to do this after each query, so if you plan on having additional queries sent to the database after the initial, you don't need to close it until after the last query. So I would remove all of these includes and just have one require at the top of the script for the database connection with credentials.

Actually, one more thing to note is the use of comments. While you have comments throughout the script, which is very good so I'm glad to see it, you should also include a quick summary at the top of the script saying, for example, what the script does, when it was written, when it was modified, etc etc... This is good for when you have to come back to the script, say a year from now, you know everything about the script. Any information you might think you'll need in the future you should add in a comment at the top. You'll find that this is an important step in writing larger applications, and it is good to get used to doing it now.

Anyway, that's all I have right now. Maybe someone else will contribute something. :)

adammc




msg:3709661
 8:27 pm on Jul 28, 2008 (gmt 0)

thank you for the very informative post :)

"The second thing I saw is all the instances of if(empty($clean[...])) { are really space consuming and can probably be easily done with a loop through all of the post variables. I would have made an array of error messages in case each was empty, and looped through the array of POSTed variables, and ........."

Can you give an example of how this would be done, sorry Im not sure exactly what you mean?

Demaestro




msg:3709668
 8:32 pm on Jul 28, 2008 (gmt 0)

I know this is a little off the PHP topic but to me cleaner code would have client side javascript validation rather then server side validation that requires page loads every time they submit the form incorrectly.

MattAU




msg:3709925
 3:23 am on Jul 29, 2008 (gmt 0)

Your code looks pretty good to me, though you could make it signficantly neater by creating a class to do the validation. If you don't have much experience with classes, this is would be an excellent time to look into it... Classes aren't for everything, or even everyone, but validation classes make a lot of sense.

.. client side javascript validation rather then server side validation ..

You should never use client side validation as a replacement for server side. What if the user has javascript turned off, is a bot, is doing something malicious etc.?

Ideally you'd use both forms of validation, but if you're only going to use one then it really has to be server side.

Too be honest though, I don't really think this is related to cleaner code. You can have clean code using either method, it's just about implementation.

adammc




msg:3709927
 3:35 am on Jul 29, 2008 (gmt 0)

Hi Matt,

Can you please direct me on the right path to info about classes?

eelixduppy




msg:3709928
 3:39 am on Jul 29, 2008 (gmt 0)

>> Can you give an example of how this would be done

From what you have, I would do something like this, which to me, is much more concise and easy to add-on to:

$error_msgs = array(
'password_confirm' => 'Please confirm your password.',
'title' => 'Please enter your title',
'first_name' => 'Please enter your first name',
'last_name' => 'Please enter your last name'
);
#
$errors = array();
#
foreach($clean as $name => $value) {
if(array_key_exists($name, $error_msgs)) {
if(empty($value)) {
$errors[] = $error_msgs[$name];
} else {
$$name = $value;
}
}
}

This doesn't include all of the fields that you want to check but it does get you started.

eelixduppy




msg:3709935
 3:48 am on Jul 29, 2008 (gmt 0)

Although not related to clean code, there's another thing that I missed with the first pass. Your secuirty validation:

// Check the security field has been answered correctly
if(($security) !== "2"){
$errors[] = "You answered the security question wrongly. 1 + 1 = 2)";
}

Will always be the same. I would at least generate 2 random numbers between, say, 1 and 10 and sum them, then pass the actual sum in a session variable and check it here. This will be a little more secure than what you currently have in place. :)

>> Can you please direct me on the right path to info about classes?

[php.net...]

adammc




msg:3709990
 5:55 am on Jul 29, 2008 (gmt 0)

Thanks for the great advice eelixduppy :)

"Will always be the same. I would at least generate 2 random numbers between, say, 1 and 10 and sum them, then pass the actual sum in a session variable and check it here. This will be a little more secure than what you currently have in place. :)"

So you mean to not get user input, perform as a hidden input?

eelixduppy




msg:3710153
 11:57 am on Jul 29, 2008 (gmt 0)

>> So you mean to not get user input, perform as a hidden input?

No. For example, you could have something like this on the form page:

[url=http://www.php.net/session_start]session_start[/url]();
$num1 = [url=http://www.php.net/rand]rand[/url](1, 10);
$num2 = rand(1, 10);
$_SESSION['secure'] = $num1 + $num2;
echo $num1 . ' + ' . $num2 . ' = <input type="text" name="secure" value="?" />';

This still accepts user input, just uses random numbers for the math. Then on the action page, which you have reproduce above, you'd check the submitted value against the value in the sessions variable:

session_start();
if($secure == $_SESSION['secure']) ....

As I said before, this will be more secure. Obviously you could add more validation, like image-based CAPTCHA, however, that isn't really necessary as this method will be more than good enough for most applications. Good luck :)

adammc




msg:3710629
 8:41 pm on Jul 29, 2008 (gmt 0)

Very nice trick !
thank you :)

Global Options:
 top home search open messages active posts  
 

Home / Forums Index / Code, Content, and Presentation / PHP Server Side Scripting
rss feed

All trademarks and copyrights held by respective owners. Member comments are owned by the poster.
Terms of Service ¦ Privacy Policy ¦ Report Problem ¦ About
© Webmaster World 1996-2014 all rights reserved