PDA

View Full Version : good practice


gavimobile
10-20-2008, 09:02 PM
i can now make scripts which work just fine, when i ask around the web, everyone seems to tell me something different.

not like this, supose to be like that!

its very overwelming.

php.net doesnt have anything about good practice...

here is a code i made just right now. please go over it and tell me what i can do to make this better.. dont let me make the same mistakes you guys did when you started out!

tia
gavi


<?php
require_once("includes/config.inc.php");

//the logged in session - this line will be changed to an actual session once i make a login script
$session = (int) 1000;

$del = $_GET['del'];
$upd = $_GET['upd'];
$confirm = $_GET['confirm'];
?>


<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
<meta name="Description" content="" />
<meta name="Keywords" content="" />
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta name="Distribution" content="Global" />
<meta name="Robots" content="index,follow" />
<title>Main Menu</title>
<link rel="stylesheet" href="css/master.css" type="text/css" />
</head>

<body>
<h3>List of Accounts</h3>

<?php
//escape the strings which are posted
$name = mysql_real_escape_string($_POST['name']);
$num = (int) $_POST['num'];

//search for existing name
$exists = mysql_query("SELECT * FROM accounts WHERE account_num='$session' AND account_name='$name' ");
$existing_rows = mysql_num_rows($exists);

// insert if submit is pressed
if (isset($_POST['submit'])) {
if (empty($_POST['name'])){
$blank = "You cannot leave the field blank!";
}elseif (isset($name)){
//if no matches
if ($existing_rows == 0) {
mysql_query("INSERT INTO accounts (account_name, account_num) VALUES ('$name', '$num') ");
$sucess = "New account has been created!";
}
// if matching
elseif ($existing_rows >= 1) {
$existing = "You already have an account with that name";
}
}else{
$error = "There was a problem creating the new account, please try again";
}
}
//deteing a record
if (isset($del)) {
echo 'Are you sure you want to delete this account?
<br />
<a href="index.php?confirm=' .$del. '" >Yes</a>
<br />
<a href="index.php?confirm=0">No</a>
<br /><br />';
}

//Confirmation - if equals to 0
if (isset($confirm)) {
if ($confirm == 0){
echo "Item was NOT deleted!"."<br /><br />";
}elseif ($confirm >= 1) {
mysql_query("DELETE FROM accounts WHERE id_accounts='$confirm' ");
echo "Item was DELETED!"."<br /><br />";
}elseif ($existing_rows == 0) {
echo "There is no account with that name!"."<br /><br />";
}
}

//info for loggedin user
$user = mysql_query("SELECT * FROM users WHERE user_num='$session'" );
$user_row = mysql_fetch_array($user);

// get the user num
$user_num = (int) $user_row['user_num'];

// get all accounts from logedin user
$accounts = mysql_query("SELECT * FROM accounts WHERE account_num='$user_num' ");

// numbers the rows
$num = 1;

//show results
echo '<table>';
while ($accounts_row = mysql_fetch_array($accounts)) {
echo '<tr><td>'.$num++ .'</td><td>'. $accounts_row['account_name'] . '</td><td> '.
'<a href="index.php?del='.$accounts_row['id_accounts'].'" >Delete</a><td></tr>';
}
echo '</table>';
?>

<br />
<h3>Add New Account</h3>

<?php
if (empty($_POST['name'])){
echo $blank."<br />";
}elseif (isset($name)){
echo $sucess.$existing."<br />";
}else{
echo $error."<br />";
}
?>

<form name="form1" action="" method="POST">
<label>Name</label>
<input type="text" name="name" /> <br />
<input type="hidden" name="num" value="<?php echo $session; ?>" />
<input type="submit" name="submit" value="Add" />
</form>

</body>
</html>

davidj
10-21-2008, 06:11 AM
there is 1000 ways to write the same script and they are all correct if they do whats required. Your job is to make them better the next time you write code.

Your script is needing a error handling function where all your messages are in one place.

gavimobile
10-21-2008, 01:01 PM
there is 1000 ways to write the same script and they are all correct if they do whats required. Your job is to make them better the next time you write code.

Your script is needing a error handling function where all your messages are in one place.
dj, thanks for the reply! can you send me an example of a script which is designed like you described? an error handling function?

btw, i thank you so much for attempting to give me a lesson with sql injections. I hope you werent insulted that we never actually got a chance to do it, but in the meantime i think i got it all sorted out.

also is this insecure? why would onClick be any more secure?
<a href="link?id=<?php $row['id']; ?>" >link1</a>

davidj
10-21-2008, 02:24 PM
also is this insecure? why would onClick be any more secure?
<a href="link?id=<?php $row['id']; ?>" >link1</a>


its visible

so every one can see the ID your passing

gavimobile
10-21-2008, 02:31 PM
its visible

so every one can see the ID your passing


dj in your tutorial you showed how to do it with a button..
my javascript is weak. can u paste an example how to do it with a regular link?

thanks
gavi

davidj
10-21-2008, 07:03 PM
just use posts where posible instead of gets

gavimobile
10-21-2008, 08:56 PM
just use posts where posible instead of gets

but i need the gets, otherwize ill have a bunch of forms or a million pages..

i use gets to allow me to do a bunch of things on one page.

im building an app now with tons and tons of sub menus.
i think gets is the way 2 go

can i have that javascript pretty please?

davidj
10-22-2008, 05:41 AM
gets are not wrong but test what happens when values are changed in the address bar

make sure your not passing any secure data

also if a users rights only allows them to see some data but if your passing any id's then can those users see other data by changing values in the address bar.

also

javascript is not secure so not sure what you want here

gavimobile
10-23-2008, 12:10 PM
gets are not wrong but test what happens when values are changed in the address bar

make sure your not passing any secure data

also if a users rights only allows them to see some data but if your passing any id's then can those users see other data by changing values in the address bar.

also

javascript is not secure so not sure what you want here

i think javascript is what i need, cause as of now users see in the address bar
page.php?id=# #=$id

regarding users not being allowed to see other users content is pretty simple if im doing this correct.
its all in the query
.... WHERE user_id = '$session' AND user_whatever='$whatever'