Page 1 of 1

is my code crappy?!

Posted: Sun Feb 08, 2009 6:36 am
by dave_wilford
Hey guys/girls, firstly it's a pleasure to be here and this is my first post.

I haven't much experience with database and php work and have a feeling my code below is a little crappy.
It's basically a form handler for adding a new item to the database. One of the form fields is a drop down list which contains the product category that the new item will be within.
I am currently creating a separate query to get the product category id by using the product category name as a reference for the query in order for the add item query to insert the item under the correct product category. I hope that sort of makes sense!

Here's my code:

Code: Select all

 
<?php 
 
    // get form data
    $item_name = $_GET['name'];
    $item_desc = $_GET['description'];
    $item_price = $_GET['price'];
    $item_product = $_GET['product'];
        
    // connect to db
    $con = mysql_connect("shares","username","password");
    if (!$con)
      {
      die('Could not connect: ' . mysql_error());  
      }
 
    // select db  
    mysql_select_db("dbname", $con);
    
    // Get prod_id for selected product name
    // *this is the bad code I think!*
    $sql = mysql_query("SELECT prod_id FROM product WHERE name='$item_product'")
 
    or die(mysql_error());
    
    // set $item_product as prod_id
    while($row = mysql_fetch_array($sql)) {
            $item_product = $row['prod_id'];
            echo $item_product;
            } 
    // end bad code!
    
    // insert new record into database
    mysql_query("INSERT INTO `item` ( `item_id` , `name` , `desc` , `price` , `prod_id` ) 
                VALUES ('', '$item_name', '$item_desc', '$item_price', '$item_product')");
                
                
mysql_close($con);
 
 
?> 
 
 
Thanks,

Dave

Re: is my code crappy?!

Posted: Sun Feb 08, 2009 7:07 am
by Ziq
This code contains very dangerous errors.
1) This code is not protected from SQL-Injection.
2) This code is not protected form XSS
It can absolutely destroy all storage data.

You should create a new file (db.php, for example) and put all connection to database part to it.

Code: Select all

 
// connect to db
    $con = mysql_connect("shares","username","password");
    if (!$con)
      {
      die('Could not connect: ' . mysql_error());  
      }
 
    // select db  
    mysql_select_db("dbname", $con);
 
And then include it in page.
It will help you to maintain your project because now if you want to change the connection information, you must change all files.

This I cannot understand.

Code: Select all

 
// set $item_product as prod_id
    while($row = mysql_fetch_array($sql)) {
            $item_product = $row['prod_id'];
            echo $item_product;
            }
 
Why do you rewrite the $item_product in a loop?

Re: is my code crappy?!

Posted: Sun Feb 08, 2009 2:44 pm
by dave_wilford
Thanks for your help Ziq. I'm not worried about security at the moment as I am just trying to get the main functions working.
I managed to solve the problem, I was being completely stupid.. (php newb!!)

Here's what I wanted to do..

Code: Select all

 
    // Get prod_id for selected product name
    $query = "SELECT prod_id FROM product WHERE name='$item_product'";  
    $item_product = mysql_result(mysql_query($query), 0);
 
    
    mysql_query("INSERT INTO `item` ( `item_id` , `name` , `desc` , `price` , `prod_id` ) 
                VALUES ('', '$item_name', '$item_desc', '$item_price', '$item_product')");
 
Thanks