Small Scrip effeciency

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

Post Reply
User avatar
Weasel5-12
Forum Commoner
Posts: 37
Joined: Tue Sep 16, 2008 6:58 am

Small Scrip effeciency

Post by Weasel5-12 »

Sorry guys, hah forgot 2 explain myself.

I've written the following as pt2 of an processing form.. i was wondering if any new eyes could see how the effeciency could be better.
I'm always up for good clean code, so please dont hesitate to call it fugly, as long as u can explain why:D

Code: Select all

<?php 
/* Program: uploader.php
* Description: Uploads data takem from addGlassInfo.php
*/
 
[color=#008000]include[/color]([color=#FF0000]"misc.inc"[/color]);
 
$connection = [color=#0000FF]mysql_connect[/color]($host, $user, $password) [color=#0000FF]or die[/color] ([color=#FF0000]"Couldn't connect to server"[/color]);
$db = [color=#0000FF]mysql_select_db($database, $connection)[color=#0000FF]or die[/color] ([color=#FF0000]"Couldn't connect to database"[/color]);
 
// Variable Declaration
$name = [color=#0000FF]$_POST[/color]['name'];
$description = [color=#0000FF]$_POST[/color]['description'];
$instructions = [color=#0000FF]$_POST[/color]['instructions'];
$glass_type = [color=#0000FF]$_POST[/color]['glass_type'];
$pic = [color=#0000FF]$_POST[/color]['pic'];
$ingredientNum = [color=#0000FF]$_POST[/color]['numOfIngredients'];
 
$process = [color=#FF0000]"addDrinkProcess_2.php"[/color];
 
/* Checks if drink already exists in drinklist table. If not, add it to table */
$query = [color=#FF0000]"SELECT drinkName FROM drinklist WHERE drinkName='$name'"[/color];
 
$result = [color=#0000FF]mysql_query[/color]($query)[color=#0000FF]or die[/color] ([color=#0000FF]mysql_error()[/color]);
 
$ntype = [color=#0000FF]mysql_num_rows[/color]($result);
[color=#008000]if[/color]($ntype != 0){
    [color=#0000FF]echo[/color] [color=#FF0000]"<span class='style2'><h4>NOTE: The following already exist in the database:</h4></span>"[/color];
    [color=#0000FF]echo[/color] [color=#FF0000]"<hr>"[/color];
    [color=#008000]include[/color]([color=#FF0000]"dataOutputTable_1.inc"[/color]);
 
}[color=#008000]else[/color]{
    $query = [color=#FF0000]"INSERT INTO drinklist (drinkID, drinkName, drinkInstructions, drinkDescription, drinkPicture, glassID) VALUES ('NULL' , '$name', '$instructions', '$description', '$pic', '$glass_type')"[/color];
    $result = [color=#0000FF]mysql_query[/color]($query)[color=#0000FF]or die[/color] ([color=#0000FF]mysql_error()[/color]);
    [color=#0000FF]echo[/color] [color=#FF0000]"<h2>The following Have been added to the database:</h2>"[/color];
 
    // Brings in external table file for output.
    [color=#008000]include[/color]([color=#FF0000]"dataOutputTable_1.inc"[/color]);
    
    [color=#0000FF]echo[/color] [color=#FF0000]"<br><hr><br>"[/color];
    
    // .......
    //[color=#0000FF]echo[/color] [color=#FF0000]"<form id='form1' name='form1' method='post' action=''>"[/color];
    [color=#008000]for[/color]($x = 0; $x < $ingredientNum; $x++){
    $result_1 = [color=#0000FF]mysql_query[/color]([color=#FF0000]"SELECT * FROM ingredientlist"[/color]) [color=#0000FF]or die[/color] ([color=#0000FF]mysql_error()[/color]);
    $result_2 = [color=#0000FF]mysql_query[/color]([color=#FF0000]"SELECT * FROM setamount"[/color]) [color=#0000FF]or die[/color] ([color=#0000FF]mysql_error()[/color]);
    [color=#0000FF]echo[/color] [color=#FF0000]"Ingredient: "[/color];
    [color=#0000FF]echo[/color] [color=#FF0000]"<select name='ingredient[]'><br>\n"[/color];
    
    [color=#008000]while [/color]($row1 = [color=#0000FF]mysql_fetch_array[/color]($result_1)){
        extract($row1);
        [color=#0000FF]echo[/color] [color=#FF0000]"<option value='"[/color].$ingredientID.[color=#FF0000]"'>"[/color].$ingredientName.[color=#FF0000]"<br>\n"[/color];
    }
    
    [color=#0000FF]echo[/color] [color=#FF0000]"</select>"[/color];
    [color=#0000FF]echo[/color] [color=#FF0000]"Amount:\n"[/color];
    [color=#0000FF]echo[/color] [color=#FF0000]"<select name='amount[]'><br>\n;"[/color];
    
    [color=#008000]while[/color]($row2 = [color=#0000FF]mysql_fetch_array[/color]($result_2)){
        extract($row2);
        [color=#0000FF]echo[/color] [color=#FF0000]"<option value='"[/color].$amount.[color=#FF0000]"'>"[/color].$amount.[color=#FF0000]"<br>\n"[/color];
        }
        [color=#0000FF]echo[/color] [color=#FF0000]"</select><br />"[/color];
    
    }
    
    [color=#0000FF]echo[/color] [color=#FF0000]"</form>"[/color];
}               
?>
Cheers guys/gals
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Re: Small Scrip effeciency

Post by Maugrim_The_Reaper »

In your variable description stage you should apply some input filtering to ensure the data you get is what you expect it to be. You should also escape the verified data with something like mysql_real_escape_string() before putting it into an SQL statement. On anything you output to HTML you should also escape using htmlspecialchars().

I'd read up on basic PHP security - it's not as complex as people think and it's just a set of habits you need to adhere to for all code you write.
User avatar
Weasel5-12
Forum Commoner
Posts: 37
Joined: Tue Sep 16, 2008 6:58 am

Re: Small Scrip effeciency

Post by Weasel5-12 »

thx alot dude. appreciated
Post Reply