Good coding practice?

[farkewie]

Hi,

 

I have been coding php for a couple of years now(self taught), I have always managed to get things working but i am trying to concentrate on good coding practice now.

 

I have just written a script to register a user on a website, what i am looking for is feedback on

• layout
  
• format
  
• efficency
  
• security
  
• any other advice
  

 

So if a few people could have a read that would be great, dont be shy let me know what you think. But please dont just say its crap unless you have a reason or some advice to follow.

<?php

//Register the user
//Checking all details are submitted and the passwords/emailss are valid.

if ( isset($_POST) )
{
$error = array();
if ( ! empty($_POST['username']) )
{
	//Check if username exists in the database.
	include_once "sql.php";
	$query = sprintf( "SELECT users.username FROM users WHERE username='%s'",
		mysql_real_escape_string($_POST['username'], $link_identifier) );
	$result = mysql_query( $query, $link_identifier ) or die( mysql_error() );
	$numrows = mysql_num_rows( $result );
	//If query returns a result then display username exists error.
	if ( $numrows > 0 )
	{
		$error[] = 'Username is in use';
	}
	mysql_close( $link_identifier );

}
else
{
	$error[] = 'Please enter username';
}
if ( ! empty($_POST['email']) )
{
	if ( eregi("^[a-zA-Z0-9_]+@[a-zA-Z0-9\-]+\.[a-zA-Z0-9\-\.]+$]", $email) )
	{
		$error[] = 'Email address not valid';
	}
}
else
{
	$error[] = 'Please enter email address';

}
if ( ! empty($_POST['confirmEmail']) )
{
	if ( ! $_POST['confirmEmail'] == $_POST['email'] )
	{
		$error[] = 'Email addresses do not match';
	}
}
else
{
	$error[] = 'Please confirm email address';

}
if ( ! empty($_POST['password']) )
{
	if ( strlen(trim($_POST['password'])) < 6 )
	{
		$error[] = 'Password is to short';
	}
}
else
{
	$error[] = 'Please enter password';
}
if ( ! empty($_POST['confirmPassword']) )
{
	if ( ! $_POST['confirmPassword'] == $_POST['password'] )
	{
		$error[] = 'Passwords do not match';
	}
}
else
{
	$error[] = 'Please confirm password';
}
if ( empty($_POST['dob']) )
{
	$error[] = 'Please select year of birth';
}

// If there are any errors do not proccess form and display the errors instead
if ( count($error) > 0 )
{
	print '<ul>';
	foreach ( $error as $er )
	{
		print "<li>$er</li>";

	}
	print '</li>';
}
else
{
	include_once "sql.php";
	$password = md5( $_POST['password'] );
	$query = sprintf( "INSERT INTO users (username, password, email,group_id) VALUES (%s,%s,%s,%d)",
		mysql_real_escape_string($_POST['username'], $link_identifier),
		mysql_real_escape_string($password, $link_identifier),
		mysql_real_escape_string($_POST['email'], $link_identifier), 0 );
	$result = mysql_query( $query, $link_identifier );
	if ( mysql_affected_rows($link_identifier) > 0 )
	{
		header( "Location:login.php" );
	}
	else
	{
		print "There was an error logging in";
	}
	mysql_close( $link_identifier );

}
}

?>

 

 

 

 

Thanks Guys.

 

[RichardRotterdam]

ok here goes,

 

layout

Don't see any layout related things in your code and this question can be interpreted  on more then one way. But I'm guessing you don't mean  html and css.

If you mean in a php way then there are a couple of things you could do.

 

• Use a framework which will separate logic, models and html output
  
• Use a template engine like smarty
  
• Try to avoid echoing out html
   
  

 

One other thing which could be handy is using alternative php syntax for if statements and loops etc.

I'll give one example

<?php
$items=array('item 1','item 2','item 3','item 4','item 5','item 6','item 7');
?>
<table>
<?php
foreach($items as $item):
?>
<tr>
	<td><strong>Name:</strong></td>
	<td><?php echo $item; ?></td>
</tr>
<?php
endforeach;
?>
</table>

In this example I use endforeach to end the loop. Now imagine if there is a lot more html between the foreach loop and it ends with a } curly bracket. Then you'd see a curly bracket but have to scroll back up in your code to see what the curly bracket ends.

 

efficency

That depends on your code if it is very complex and you use a database you try not to use more queries then you need. And you could think of using a break to only loop when it is necessary. Otherwise it's more about readability rather then efficiency.

 

Security

The best way to learn about security is by understanding the basics of hacking. Try some basic hacking tutorials on a site like hackthissite.org . It will give you a better understanding why you need to secure something and you'll be more aware of the security leaks when building something.

 

In your code I also noticed you check if the password posted was empty or not. Why not restrict it a little more by accepting only alphanumeric values.

 

any other advice

Last tip is to look for a form validation class. When building webapps building form validation can be quite time consuming and repetitive. Why not safe yourself some time and use a form validation class.

 

 

hope it was usefull and if I am wrong on any point feel welcome to point it out.

[Mchl]

Move to mysqli

[RichardRotterdam]

Move to mysqli

That or PDO making it less database dependent

[allworknoplay]

When I code, I always like to comment my brackets like so:

 

 

function whatever() { ###START WHATEVER FUNCTION

 

 

 

} ###END WHATEVER FUNCTION