Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.
Popular code excerpts may be moved to "Code Snippets" by the moderators.
Moderator: General Moderators
devnetworkphp
Forum Newbie
Posts: 3 Joined: Tue Oct 02, 2007 1:35 am
Post
by devnetworkphp » Sun Oct 14, 2007 9:02 am
Code: Select all
<?php
require_once('db.php');
$act=$_GET['act'];
$id=$_GET['id'];
if($act=='insert')
{
$username=$_POST['username'];
if(empty($username))
$error='Please enter username';
if(!empty($error))
{
$act='form';
}
else
{
if(empty($id) && empty($error))
$query=mysql_query("UPDATE userlist SET username='$username' WHERE id=$id");
else
$query=mysql_query("Insert INTO userlist SET username='$username'");
if(!empty($query) && !empty($id)) $message='Updated';
elseif(!empty($query) && empty($id)) $message='Inserted';
}
}
if($act=='delete')
{
$query=mysql_query("DELETE FROM username WHERE id=$id");
if($query) $message='Deleted';
}
if($act=='form')
{
if(!empty($id))
{
$query=mysql_query("SELECT username FROM userdetails WHERE id=$id");
$fetch_details=mysql_fetch_array($query);
$username=$fetch_details['username'];
}
$form="<form method='post' action='userdet.php?act=insert&id=$id'>
<input type='text' name='username' value='$username'>
<input type='submit' name='submit' value='submit'>
</form>";
}
if($act=='page')
{
// paging
}
echo "<table><tr><td>$message $error</td></tr><tr><td>$form</td></tr></table>";
?>
Please comment if there is any modifications that should be done in the above design.
RobertGonzalez
Site Administrator
Posts: 14293 Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA
Post
by RobertGonzalez » Sun Oct 14, 2007 9:42 am
Are you asking for a code review?
devnetworkphp
Forum Newbie
Posts: 3 Joined: Tue Oct 02, 2007 1:35 am
Post
by devnetworkphp » Sun Oct 14, 2007 10:15 am
Yes, any modifications, better design etc... from experts like you.
matthijs
DevNet Master
Posts: 3360 Joined: Thu Oct 06, 2005 3:57 pm
Post
by matthijs » Sun Oct 14, 2007 1:06 pm
I would 1) validate the incoming data 2) escape the data going into the database (mysql_real_escape_string()) and 3) escape the output to the browser (htmlentities())
superdezign
DevNet Master
Posts: 4135 Joined: Sat Jan 20, 2007 11:06 pm
Post
by superdezign » Sun Oct 14, 2007 2:19 pm
I'd just organize the if statements as a switch, and add in a default action.
I'd write the whole thing differently, but readability is a big issue with me, and I've noticed that different programmers read code differently.