Must be an easier/more efficient way to code this

[beckerdt]

Hello, I'm new to PHP and I have come up with the following code for my navigation but I am sure there has to be an easier and more efficient way to do this. Someone please start me on the right path. Thanks

<?php 
//Get selected page/sub page
if (isset($_GET['page'])) {
		$sel_page = $_GET['page'];
		$sel_sub_page = '';
	} elseif (isset($_GET['sub_page'])) {
			$sel_page = '';
			$sel_sub_page = $_GET['sub_page'];
		      } else {
				$sel_page = '';
				$sel_sub_page = '';
			}
?>

	<!--Begin Navigation-->
<ul>
<?php
//run the loop to get the page titles  
$query = "SELECT * FROM pages ORDER BY position ASC";
	$page_set = mysql_query($query);
		confirm_query($page_set);
			while($page = mysql_fetch_array($page_set)){
				echo '<li><a href="content.php?page=' . urlencode($page['id']) . '">' . $page['page_name'] . '</a></li>';
					//run sub page query to see if the ul tag is needed
					$query = "SELECT * FROM sub_pages WHERE page_id = {$page['id']} ORDER BY position ASC";
						$sub_page_set = mysql_query($query);
							confirm_query($sub_page_set); 
								$sub_page = mysql_fetch_array($sub_page_set);
									if ($sub_page !=''){
											echo '<ul>';
											}
								//run the sub page loop again to display the sub page title
								$query = "SELECT * FROM sub_pages WHERE page_id = {$page['id']} ORDER BY position ASC";
									$sub_page_set = mysql_query($query);
										confirm_query($sub_page_set); 
								while($sub_page = mysql_fetch_array($sub_page_set)) {
											echo '<li><a href="content.php?sub_page=' . urlencode($sub_page['id']) . '">' . $sub_page['page_name'] . '</a></li>';
								}
										//run the sub page loop for the last time to see if the end ul tag is needed
										$query = "SELECT * FROM sub_pages WHERE page_id = {$page['id']} ORDER BY position ASC";
											$sub_page_set = mysql_query($query);
												confirm_query($sub_page_set); 
										$sub_page = mysql_fetch_array($sub_page_set);
											if ($sub_page !='') {
													echo '</ul>';
												}
				}
?>	
</ul>
<!--End Navigation-->

[SamT_]

You're really mixing your HTML logic with PHP logic and (as you can see) it's getting really hairy.

 

I would try an approach where you have all your PHP stuff executed prior to even printing HTML. Just store your navigation in an array and loop it out. on the template. It looks like you have a query in a loop as well, which can really add up in terms of execution time as you don't always know the efficiency behind iterating within your algorithm.

 

What's your SQL schema (just the columns) for the sub_pages table? Ideally, we can squash this down to into a single query.

[beckerdt]

Thanks for the help. Here is my SQL schema:

mysql> SHOW COLUMNS FROM pages;

+-----------+-------------+------+-----+---------+----------------+

| Field    | Type        | Null | Key | Default | Extra          |

+-----------+-------------+------+-----+---------+----------------+

| id        | int(11)    | NO  | PRI | NULL    | auto_increment |

| position  | int(11)    | NO  |    | NULL    |                |

| visible  | tinyint(1)  | NO  |    | NULL    |                |

| page_name | varchar(30) | NO  |    | NULL    |                |

| content  | text        | NO  |    | NULL    |                |

+-----------+-------------+------+-----+---------+----------------+

5 rows in set (0.01 sec)

 

mysql> SHOW COLUMNS FROM sub_pages;

+-----------+-------------+------+-----+---------+----------------+

| Field    | Type        | Null | Key | Default | Extra          |

+-----------+-------------+------+-----+---------+----------------+

| id        | int(11)    | NO  | PRI | NULL    | auto_increment |

| page_id  | int(11)    | NO  |    | NULL    |                |

| position  | int(11)    | NO  |    | NULL    |                |

| visible  | tinyint(1)  | NO  |    | NULL    |                |

| page_name | varchar(30) | NO  |    | NULL    |                |

| content  | text        | NO  |    | NULL    |                |

+-----------+-------------+------+-----+---------+----------------+

6 rows in set (0.01 sec)

[SamT_]

Ah, so a nested navigation. If I were you, I would remove the sub_pages table and put a parent ID column on the pages table. When you have a sub page, simply specify the parent ID as the page ID to your parent.

 

Please note, I have not tested this code out, so be sure to create a new table with fake data while you test so it doesn't mess with your current setup.

 

+-----------+-------------+------+-----+---------+----------------+
| Field     | Type        | Null | Key | Default | Extra          |
+-----------+-------------+------+-----+---------+----------------+
| id        | int(11)     | NO   | PRI | NULL    | auto_increment |
| parent_id | int(11)     | NO   |     | NULL    |                |
| position  | int(11)     | NO   |     | NULL    |                |
| visible   | tinyint(1)  | NO   |     | NULL    |                |
| page_name | varchar(30) | NO   |     | NULL    |                |
| content   | text        | NO   |     | NULL    |                |
+-----------+-------------+------+-----+---------+----------------+

<?php

// connect to mysql here

$sql = 'SELECT id, parent_id, page_name 
FROM pages
WHERE visible = 1
ORDER BY position ASC';
$result = mysql_query($sql);

// Pull the results out of the query 
$nav = array();
while($row = mysql_fetch_assoc($result))
{
$nav[] = $row;
}
mysql_free_result($result);

// Organize the data so we can easily foreach() it out
// NOTE this will only work if you have only one level of sub pages
$navary = array();

// Bring the parent pages in
foreach($nav as $item)
{
if($item['parent_id'] == 0)
{
	$navary[(int) $item['id']] = $item;
}
}
unset($item);

// Bring the child pages into the array
foreach($nav as $item)
{
if($item['parent_id'] != 0)
{
	$navary[(int) $item['parent_id']]['subpages'][]  = $item;
}
}
unset($item);

// Nowe we're looping to the template:

echo '<ul>';
foreach($navary as $item)
{
echo '<li><a href="content.php?id='.$item['id'].'">' . $item['page_name'] . '</a>';
if(sizeof($item['subpages']))
{
	echo '<ul>';
	foreach($item['subpages'] as $subpage)
	{
		echo '<li><a href="content.php?id='.$item['id'].'">' . $item['page_name'] . '</a></li>';
	}
	echo '<ul>';
}
echo '</li>';
}
echo '</ul>';