Reaching around my head to scratch my nose?

[sharp.mac]

Greetings all, I have been developing in php for only a few months now and I need to bounce this code off someone to see if I am doing this the most efficient way possible. (I'm still weary of OOP php, so try not to confound me too much with objects, just stick to straight method scripting if you don't mind)

 

so here is my table.

 

id name connects

---------------------------------------------------------------

1 Bradenton 9,11

2 Cocoa Beach 11

3 Daytona Beach 11

4 Fort Lauderdale 1,5,9,10,11,14,15

5 Fort Myers 9,11

6 Gainesville 11

7 Jacksonville 11

8 Melbourne 11

9 Miami 1,4,5,10,11,14,15

10 Naples 4,9,11

11 Orlando 1,2,3,4,5,6,7,8,9,10,12,13,14,15,16

12 Palm Bay 11

13 Port Canaveral 11

14 Sarasota 4,9,11

15 Tampa 4,9,11

16 West Palm Beach 11

 

 

Here is the form. I have 2 combo boxes that populate as follows.

<form name="myform" action="#" method="post">

<select name="departure" size="1" onchange="setOptions(document.myform.departure.options[document.myform.departure.selectedIndex].value);">
<option value=" " selected="selected">Departure City</option>
<?php
	$result = mysql_query("SELECT * FROM `cities`");
	while ($row = mysql_fetch_array($result)) {
?>
	<option value="<?php echo $row['name']; ?>"><?php echo $row['name']; ?></option>
<?php }
	mysql_free_result($result);
?>
</select>

<select name="arrival" size="1">
<option value=" " selected="selected"> </option>
</select>

<input type="submit" id="submit" value="Go" />

</form>

 

Now for the fun stuff, Javascript updates the second combo box and it requires each city to follow the following format.

if (chosen == "Bradenton") {

    selbox.options[selbox.options.length] = new Option('Miami','Miami');

    selbox.options[selbox.options.length] = new Option('Orlando','Orlando');

}

 

//step & repeat for each city

 

 

So here is the PHP code I created to query the database for all cities, pull down the `connects`, split the string & then re-query the database for the IDs listed. IS THIS GOING AROUND MY HEAD TO SCRATCH MY NOSE?

<?php
//run initial cities list
$cities = mysql_query("SELECT * FROM `cities`");
while ($row = mysql_fetch_array($cities)) {

	echo 'if (chosen == "'. $row['name'] .'") {' . "\n";

	$arrq = mysql_query("SELECT `connects` FROM `cities` WHERE `id` = " . $row['id']);
	$ids =  mysql_result($arrq, 0, "connects");

	//seperates the values inside connects
	$array = split(',', $ids);

	foreach ($array as $value) {
		//query the database for each new connect as id (pulls each city id, then querys the database again
		$qname = mysql_query("SELECT `name` FROM `cities` WHERE `id` = " . $value);
		$name =  mysql_result($qname, 0, "name");

		echo "selbox.options[selbox.options.length] = new Option('".$name."','".$name."');";
	}
	echo "}\n\n";
};
mysql_free_result($cities);
?>

 

 

 

 

The script works just fine, it has no errors and outputs exactly as it is intended. I am looking for user feedback on my logic, am i taking too many steps, is it a run around? Feedback is greatly appreciated. Thank you for your time.

 

[bulrush]

From what I understand, sometimes you have to make a lot of PHP code to do something. This isn't MS Access (that I have used before) where most of the work is done for you.

 

OTOH, I don't know why you are quoting field names in your SQL statement. I have never seen that before. Generally you quote the values, not the field names. That might cause problems in the future, especially if you run this on another server for some reason.

 

[Psycho]

@bulrush

 

You shouldn't comment on something that you don't understand. MS Access is a proprietary database and front end rolled into a single application. It is more like FileMaker than a true database that can be repurposed for many outputs. And, using backquotes around database table/field names is absolutely valid and would be the "most" proper method since it prevents table/field name conflicts with reserved words.

 

@sharp.mac

 

You shouldn't use * in your queries unless you need every field. List out the fields you need to make the queries more efficient. And, you should never do "looping" queries. I.e. do one query and then use the results to run several more queries. Do some searching to find a tutorial or two on using JOINS and get all the information you need in one query. Doing many queries like this is a huge overhead on the server.

 

However, because you are listing the connect ids as a comma separated value you have really destroyed the value of using a database. You should really create another table (say 'connections') with two fields origin_id and connect_id. If you had that you would just need one query to get all the data you need:

 

SELECT origin.name as origin, destination.name as destination
FROM cities as origin
JOIN connections ON cities.id = connections.origin_id
JOIN cities as destination ON connections.destination_id = destination.id
ORDER BY origin, destination

 

That is how it should be done. However, with your current implementation you could see some improvement by doing a single query on the comma separated list (using "IN") instead of querying for each item in the list.

 

Here is an improved version of your original code:

<?php

//Run query to get list of origin cities and connection IDs
$query = "SELECT name as origin, connects FROM `cities`"
$cities = mysql_query($query);

$jscript = '';
while ($cityRow = mysql_fetch_array($cities))
{
    //Run query to get connection cities
  	$query = "SELECT `name` as connection FROM `cities` WHERE `id` IN ({$cityRow['connects']})";
  	$connections = mysql_query($query);

    $jscript .= "if (chosen == '{$cityRow['origin']}') {\n";
    while($connectRow = mysql_fetch_array($connections))
    {
        $connection = $connectRow['connection'];
    $jscript .= "selbox.options[selbox.options.length] = new Option('{$connection}','{$connection}');\n";
    }
	$jscript .= "}\n\n";
};

mysql_free_result($cities);
echo $jscript;

?>

 

Note: I would also suggest creating javascript arrays to store the data and have your function utilize those arrays instead of having to write out all the "new Option" lines.

 

[andrewgauger]

back-ticks (the ~ without a shift) is the appropriate way to mitigate field names that contradict reserved work (such as "group" or "interval", and "password" is debated).  My opinion is to not use reserved words AND to use back ticks.

 

That said, no you are not doing this according to "industry standard" practices.  Your table should look like this:

cities

city_id  city

1          Bradenton

2          Cocoa Beach

 

And have a table that defines references

connects

city_id  connect_id

1          9

1          11

2          11

3          11

 

Your solution does not scale well.  This may work for your implementation, but next project create a referential table  You then use join in your select syntax to return multiple rows for each reference.

 

SELECT a.`cities` FROM `cities` as a JOIN `connects`as b ON (a.`city_id`=b.`connect_id`) WHERE a.`id` = " . $row['id']

 

Will return city names that connect given the id of the original city.

 

@mjdamato nailed it.

 

[sharp.mac]

Excellent!

 

Being 100% self taught has it's downfalls, I am very grateful for your time in responding to me mjdamato.

 

Thank you andrewgauger for elaborating and giving a table structure example.

 

I do believe since my project is still in it's infancy I will structure things differently in my next build.

 

I will let you know how things turned out.

[solved]  for now