Code Check

rhals

Hello!

Thanks in advance for you time. The code posted below works as intended. However, being new to this I am sure it was not written correctly or may be easily hacked.

Any help would be appreciated!!

Thanks

<?php

    $page_set   = mysql_prep($page_set);
	$page       = mysql_prep($page);
	$page['id'] = mysql_prep($page['id']);
	$floor_set  = mysql_prep($floor_set);
	$floor      = mysql_prep($floor);

	$page_set   = strip_tags($page_set);
	$page       = strip_tags($page);
	$page['id'] = strip_tags($page['id']);
	$floor_set  = strip_tags($floor_set);
	$floor      = strip_tags($floor);


	$page_set = mysql_query("SELECT * FROM pages WHERE ready = 1 ORDER BY position ASC", $connection);
	if(!$page_set)  {
	die("Database query failed: " . mysql_error());
	}
	while ($page = mysql_fetch_array($page_set)) {
	echo "<div class=\"about_head\">";
	echo "<a href=\"listing.php?page={$page['id']}\">{$page['menu_name']}</a>";
	echo "</div>";
	echo "<div style=\"margin:0 0 10px 0;\">";	
	echo $page['short_desc'];
	echo "</div>";


	echo "<table class=\"sortable\" id=\"anyid\" cellpadding=\"0\" cellspacing=\"0\">";
	echo "<tr> 
	<th>Floor Plan</th>
	<th>Address</th>
	<th>Move in Date</th>
	<th>Bed</th>
	<th>Bath</th>
	<th>Garage</th>
	<th>Sq.Ft.</th>
	<th style=\width:120px;\">Priced From</th>
	</tr>";


	    $floor_set = mysql_query("SELECT * FROM floors WHERE page_id = {$page['id']} AND ready = 1 ORDER BY position ASC", 		                                                                                                                   $connection);
		if(!$floor_set)  {
		die("Database query failed: " . mysql_error());
		}
		while ($floor = mysql_fetch_array($floor_set)) {

		echo "<tr><td style=\"padding:5px;\">";
		echo "<a href=\"floorplan.php?page={$page['id']}&floor={$floor['id']}\">{$floor['menu_name']}</a>";
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['address'];
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['move_date'];
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['bed'];
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['bath'];
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['garage'];
		echo "</td><td style=\"padding:5px; text-align:center;\">";
		echo $floor['square_feet'];
		echo "</td><td style=\"padding:5px;  text-align:center;\">";
		echo $floor['price'];
		echo "</td></tr>"; 

	}
echo "</table>";

echo "<div style=\"height: 10px;\"></div>";



	}

?>

bradgrafelman

Thread moved to Code Critique forum. And now for my critique...

Welcome to PHPBuilder! 🙂

This entire block of code:

        $page_set   = mysql_prep($page_set); 
        $page       = mysql_prep($page); 
        $page['id'] = mysql_prep($page['id']); 
        $floor_set  = mysql_prep($floor_set); 
        $floor      = mysql_prep($floor); 

    $page_set   = strip_tags($page_set); 
    $page       = strip_tags($page); 
    $page['id'] = strip_tags($page['id']); 
    $floor_set  = strip_tags($floor_set); 
    $floor      = strip_tags($floor);

has no effect (and could/should be removed and/or placed elsewhere). You start off by attempting to use variables that don't exist, so I'm not sure what you were trying to accomplish here?

Although you haven't shown any such code (or include/require lines), I'm assuming you've handled connecting to the DB server and selecting the correct database elsewhere...?
In a SELECT query, it's usually best to actually name the columns from which you need data (even if that's all of the columns at the time).
Feel free to post your DB schema/structure (with or without a couple sample INSERTs) for critique as well.
Note that it's generally recommended that you never output things such as mysql_error() on a production server. Instead, you should print a user-friendly (purposely generic and non-revealing) error message and instead log (see [man]log_errors[/man] in the PHP manual) all of the technical messages/details for later review.
As a matter of personal preference, I usually avoid using the generic [man]mysql_fetch_array/man function and instead stick with [man]mysql_fetch_assoc/man if I'm only going to be using the associative array indexes (which is just about always :p).
While it may or may not be an issue, be conscious when outputting data from a DB into HTML markup. Consider whether or not you want HTML markup to be parsed in the data from the DB and, if not, how you want to handle such data (e.g. either stripping it out, using [man]strip_tags/man, or simply displaying it safely using something like [man]htmlentities/man).
Whenever I see a MySQL query being executed in a loop (especially nested inside the loop of another query), I immediately raise a mental red flag and look to see how the structure can be optimized.

In your case, you could reduce the queries down to a single query/loop (reducing overhead) using a JOIN instead of multiple queries.

rhals

Thank you!! I will rewrite and resubmit.