Optimise my PHP code

Vulcan

Vulcan

Vulcan

Soldato
Joined
15 Jan 2004
Posts
10,206
This code adds a category to the database categories with the fields c_id (auto), c_order, c_name, c_image

It works, but could probably be made more efficient.

I've used a URL for the code instead of BB tags because of the pretty colours :p

http://matstudios.com/temp/code.html

All criticism is welcome!

Thanks.
 
Can't understand it. But don't mangle random HTML with PHP. Use the handy format 'SELECT column_with_nasty_name AS nice_name' in queries. Do some error checking. Any one of those queries could fail and leave you with a horrible mess.

Write a function to do SQL queries and handle errors. Write a function to clean variables.

What's this bit?

Code: 
            if ($order < $count) {
                // Got to change all the orders
 
Not tested, but how about (this could still be improved a lot, it was a 2 minute job)


Code: 
<?php
ConnectToDb($server, $username, $password, $database);
@$sql = mysql_query("SELECT COUNT(c_id) FROM categories") or die (mysql_error());


if(@mysql_num_rows($sql) > 0)
{
	
	$row = mysql_fetch_array($sql);
	$count = $row['COUNT(c_id)'];

		if (isset($_POST['order']))
		{
			//Add data to the database.
			$order = mysql_real_escape_string($_POST['order']);
			$name = mysql_real_escape_string($_POST['name']);
			$image = mysql_real_escape_string($_POST['image']);
	
			if (empty($order) || empty($name) || empty($image)) { 
				echo"Missing field data. Go <a href=\"javascript:history.go(-1)\">back</a>.";
			}
			else
			{
	
				if ($order < $count)
				{
					// Got to change all the orders
					while ($count >= $order)
					{
						$neworder = $count + 1;
						mysql_query("UPDATE `categories` SET `c_order` = '$neworder' WHERE `c_order` = $count");
						$count = $count - 1;
					}
					mysql_query("INSERT INTO categories VALUES (NULL ,'$order','$name','$image')");
					echo"Category <b>$name</b> added. You can now <a href=\"add_product.php\">add</a> products to this category, or <a href=\"add_category.php\">create</a> another category.";
	
				}
				else
				{
					// Nothing special here, just add it on the end.
					mysql_query("INSERT INTO categories VALUES (NULL ,'$order','$name','$image')");
					echo"Category <b>$name</b> added. You can now <a href=\"add_products.php\">add</a> products to this category.";
				}
			}
		}
		else
		{ 
		
			$order = $count + 1;
	
			?> <form action="add_category.php?add" method="post">
			Order:<br/>
			<select name="order">
			<?
	
			$i=1;
			while ($i <= $count)
			{
				echo("<option value=\"$i\">$i</option><br/><br/>\n");
				$i++;
			}
			
			echo("<option value=\"$order\" selected>$order</option><br/><br/>");
			
			?>
			</select><br/><br/>
			Title:<br/><input type="text" size="35" name="name"/><br/><br/>
			Image:<br/><input type="text" size="25" name="image"/><br/><br/>
			<input type="submit" name="send" value="Add" onclick="this.disabled=true;"/>
			</form>
			<?
		
		}
		
	mysql_close();			
}
else
{
	echo "no orders";
}


?>

It's neater and removes some redundant code. You may want to remove HTML as Beansprout says.
 
Last edited:
The random HTML is just the form which then returns the data back to the top of the page.

The code you are referring to checks if the order (of the category) entered by the user is lower than highest order number in the database.

And if so, all the rows above that users order have to moved up.

So If I have 4 categories, and I add another with the order '2', than the categories with order, 2, 3 & 4 have to move up (+1).
 
That's a bad way to do it, assuming the ID is automatically generated you may end up with problems if you delete records. Automatic IDs get assigned in order, i.e.:

1
2
3

however if you delete a record, you'll have say:

1
3

your count will show two rows when the highest value is actually 3.
 
Longbow said:
Yes, so when a row is deleted, I will have to do the reverse, and lower the order fields.
Click to expand...
Why? Unique IDs are totally unique; if you delete a record then it's not 'Never Existed', it's "this record has been deleted". Zero and NULL exist for good reasons in computer science; my view is that unique IDs are totally and utterly unique and should be left that way :)

IDs are useless anyway - they don't 'mean' anything as such.
 
Yes, but the c_order will not longer be in use.

I think what I am trying to achieve and what you are interpreting are different.

The ID's are unique and only their for a reference point. The categories will shown in order by c_order.

Like this:

ID | Order
2 | 1
4 | 2
1 | 3
3 | 4

If I remove ID 4, where order = 3 needs to be moved down, same with the 4th order.

Is there another way to do it?
 
Why do you want sequential numbers at all? What is the significance of them?

Are you just using the sequential numbers for display convenience?
 
Longbow said:
Is there another way to do it?
Click to expand...
Aha, I see - the order column lets you alter the display order on a whim - but you can order the array numerically via 'ORDER BY field [ASC|DESC]' in SQL, so the only time you need to fill gaps is when you change the display order and update the new positions.

It's too late to work out that code again so I don't know how it currently works, but that's how I think it should work.
 
Yeah, after much thought though, I've decided to drop the row idea, and just order by name.

One more question:

Is this the most effective way of retrieving everything from a table? Row by row.

Code: 
<?php require_once("../inc/core.php");
ConnectToDb($server, $username, $password, $database);
$sql = mysql_query("SELECT * FROM categories") or die (mysql_error());
$num_rows = mysql_num_rows($sql);

if ($num_rows > 0)
{
	$i=0;
	while ($i < $num_rows) {
	$id=mysql_result($sql,$i,"c_id");
	$name=mysql_result($sql,$i,"c_name");
	$image=mysql_result($sql,$i,"c_image");
	echo("$id $name $image <br/>");
	$i++;
	}
}
else
{
	echo("No categories created yet.");
}
mysql_close();
?>
 
You must log in or register to reply here.
Back
Top Bottom