Page 1 of 2

Messy code .. Can somone help cut it down to minimal code?

Posted: Fri Nov 03, 2006 2:49 pm
by bob_the _builder
Hi,

Im sure the following code can be writen better maybe cutting the amount of code in half?

Code: Select all

<?php

$manufacturer = addslashes($_REQUEST['manufacturer']);
$choice = addslashes($_REQUEST['choice']);
$search = addslashes($_REQUEST['search']);
$under = addslashes($_REQUEST['under']);
$over = addslashes($_REQUEST['over']);

?>

<table style="margin: 2px; border: 1px solid #000000;" width="99%" align="center" cellpadding="0" cellspacing="0">
  <tr>
    <th colspan="2">Search for vehicle</th>
  </tr>
  <tr><br />
    <td align="center"><br />
	  <form name="manufacturer" action="../index.php?pages=sh_cars" method="post">Show
       <select name="manufacturer" class="input-box"><?php $sql = mysql_query("SELECT * FROM manufacturer ORDER BY make ASC"); while ($row = mysql_fetch_array($sql)) { ?>
         <option <?php echo $manufacturer==$row['make'] ? 'selected' : ''?>><?php echo $row['make'] ?></option><?php } ?>
       </select>
       <input type="submit" name="Submit" value="Submit" />
	  </form>
    </td>
    <td align="center"><br />
	  <form name="choice" action="../index.php?pages=sh_cars" method="post">Or search
       <select name="choice" class="input-box"><?php if ($_SESSION['user_level'] == 3) { ?>
         <option <?php echo $choice=='Code' ? 'selected' : ''?> value="Code">Number Plate</option><?php } ?>
         <option <?php echo $choice=='Colour' ? 'selected' : ''?>>Colour</option>
         <option <?php echo $choice=='Fuel' ? 'selected' : ''?>>Fuel</option>
         <option <?php echo $choice=='Model' ? 'selected' : ''?>>Model</option>
         <option <?php echo $choice=='Transmission' ? 'selected' : ''?>>Transmission</option>
         <option <?php echo $choice=='Year' ? 'selected' : ''?>>Year</option>
       </select>for
        <input name="search" type="text" size="18" class="input-box" value="<?php echo $search; ?>" />
        <input type="submit" name="Submit" value="Submit" />
	  </form>
	</td>
  </tr>
</table>

<?php

if ($_SESSION['user_level'] == 3) {
	$availability = "('available' , 'sold')";
}else{
	$availability = "('available')"; }

if (!isset($_GET['page'])) { $page = 1; }else{ $page = $_GET['page']; }

	$max_results = 1; 
	$numcols = 4;
	$numcolsprinted = 0;
	$from = (($page * $max_results) - $max_results); 

if (isset($_REQUEST['search']) && trim($search) !== "") {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND (".$choice." LIKE('".$search."'))"),0);
	$total_pages = ceil($total_results / $max_results); 
	$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    	    JOIN manufacturer as m ON m.id = c.manufacturer		
			LEFT JOIN car_images as p ON p.car_id = c.id
			WHERE c.availability IN $availability AND (".$choice." LIKE('".$search."')) 
			GROUP BY c.id 
			LIMIT $from, $max_results";

}elseif (isset($_REQUEST['manufacturer']) && trim($manufacturer) !== "") {

    $total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars as c JOIN manufacturer as m ON m.id = c.manufacturer WHERE availability IN $availability AND make ='".$manufacturer."'"),0);
    $total_pages = ceil($total_results / $max_results);
    $sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    		JOIN manufacturer as m ON m.id = c.manufacturer
			LEFT JOIN car_images as p ON p.car_id = c.id
    		WHERE c.availability IN $availability AND m.make='".$manufacturer."'
    		GROUP BY c.id
    		LIMIT $from, $max_results";
			
}elseif (isset($_REQUEST['under'])) {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND price <= '".$under."'"),0);
	$total_pages = ceil($total_results / $max_results); 
	$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    	    JOIN manufacturer as m ON m.id = c.manufacturer		
			LEFT JOIN car_images as p ON p.car_id = c.id
			WHERE availability IN $availability AND price <= '".$under."' 
			GROUP BY c.id 
			LIMIT $from, $max_results";
			
}elseif (isset($_REQUEST['over'])) {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND price > '".$over."'"),0);
	$total_pages = ceil($total_results / $max_results); 
	$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    	    JOIN manufacturer as m ON m.id = c.manufacturer		
			LEFT JOIN car_images as p ON p.car_id = c.id
			WHERE availability IN $availability AND price > '".$over."' 
			GROUP BY c.id 
			LIMIT $from, $max_results";

}else{

	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability"),0); 
	$total_pages = ceil($total_results / $max_results);
	$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    		JOIN manufacturer as m ON m.id = c.manufacturer
			LEFT JOIN car_images as p ON p.car_id = c.id
			WHERE c.availability IN $availability
    		GROUP BY c.id ORDER BY c.id DESC
    		LIMIT $from, $max_results";
}	

?>

<table style="margin: 2px; border: 1px solid #000000;" width="99%" align="center" cellpadding="0" cellspacing="0">
 <tr>
  <th>Listed Cars </th>
 </tr>
 <tr>
  <td align="center">

<?php
	
if ($_SESSION['user_level'] == 3) {
	echo '<br /><center><a href="../index.php?pages=add_cars">ADD NEW</a></center>';
}
	echo '<br /><center><b>'.$total_results.'</b> cars matching your search | ';
	echo 'Displaying Page <b>'.$page.'</b> of <b>'.$total_pages.'</b><br /><br />';
	echo '<a href="../index.php?pages=sh_cars">SHOW ALL CARS</a><br /><br />';
	$result = mysql_query($sql);
	$num_rows = mysql_num_rows($result);
?>

    <table border="1" bordercolor="FFFFFF" cellspacing="5" cellpadding="0" align="center">

<?php

if ($num_rows > 0 ) {
	while($row = mysql_fetch_array($result)){ 
if($row['photo_filename'] == 0) {
	$picture = 'No Images<br />';
} else {
	$picture =  '<a href="/index.php?pages=sh_car&id='.$row['id'].'"><img src="../'.$images_dir.'/tb_'.$row['photo_filename'].'" border="1"></a><br />';
}
if ($numcolsprinted == $numcols) {

?>

     <tr>
	 
<?php

	$numcolsprinted = 0;
}

?>
      <td style="border:1 dashed #999999;" width="130" height="130" align="center" valign="bottom"><br />
	   <?php echo $picture; ?><br />
       <b><?php echo ValidateOutput($row['year']); ?></b><br />
       <?php echo ValidateOutput($row['make']); ?> <?php echo ValidateOutput($row['model']); ?></b><br />
       <br />
       <a href="../index.php?pages=sh_car&id=<?php echo $row['id']; ?>">Read more...</a><br /><br />
       <?php

if ($_SESSION['user_level'] == 3) {
	  
	  echo '<a href="/index.php?pages=add_cars&id='.$row['id'].'&edit=edit">edit</a> - <a href="/index.php?pages=del_cars&del_all&id='.$row['id'].'">delete</a><br /><br />';
}

?>

      </td>
	  
<?php

	$numcolsprinted++;
}
	$colstobalance = $numcols - $numcolsprinted;
	for ($i=1; $i<=$colstobalance; $i++) {
} 

?>

     </tr>
    </table>
	
<?php

}else{
	echo '<center>No results found</center>';
}
	echo '<br /><center>';
if ($page > 1) {
	$prev = ($page - 1); 
    echo "<a href=\"../index.php?pages=sh_cars&page=$prev&manufacturer=$manufacturer&choice=$choice&search=$search\">Previous</a>&nbsp|&nbsp"; 
}
	for ($i = 1; $i <= $total_pages; $i++) {
if (($page) == $i) {
	echo "[$i]&nbsp;"; 
}else{ 
	echo "<a href=\"../index.php?pages=sh_cars&page=$i&manufacturer=$manufacturer&choice=$choice&search=$search\">$i</a>&nbsp"; 
 }
}

if ($page < $total_pages) {
	$next = ($page + 1);
	echo "&nbsp;|&nbsp;<a href=\"../index.php?pages=sh_cars&page=$next&manufacturer=$manufacturer&choice=$choice&search=$search\">Next</a>"; 
}
	echo "</center><br />";

?>

   </td>
 </tr>
</table>

Thanks

Posted: Fri Nov 03, 2006 3:04 pm
by feyd
You want us to do it for you?

Posted: Fri Nov 03, 2006 3:13 pm
by bob_the _builder
Hi,

Wasnt particually asking for it to be done for me .. But feel free to do so :wink: Altho pointers on how to clean it up would be appreciated as well.

Thanks

Posted: Fri Nov 03, 2006 3:19 pm
by feyd
Your post makes it sound like you want someone to do it for you. That's just not likely to happen with most of us.

Posted: Fri Nov 03, 2006 3:21 pm
by bob_the _builder
The word help means "Do it for me" ?

Posted: Fri Nov 03, 2006 3:27 pm
by feyd
No, "help" by itself doesn't mean you want it done for you, but your choice of other words and previous history suggested it. At any rate, I'll leave it be now.

Posted: Fri Nov 03, 2006 3:32 pm
by bob_the _builder
Well gettin it done for me wasnt my intension ..

Given the crap start to this thread :oops: I would still like pointers on how the code can be writen tidyer if anyone is interested in helping.


Thanks

Posted: Fri Nov 03, 2006 4:31 pm
by RobertGonzalez
The only thing I'd recommend is to try to minimize that massive if/elseif/elseif/elseif/elseif/yadda yadda into something more managable.

Posted: Fri Nov 03, 2006 5:50 pm
by bob_the _builder
Hi,

Thanks .. Yep those are my thoughts, but im not really to sure how I should manage it, or the best way. Those are the hints im looking for


Thanks

Posted: Sat Nov 04, 2006 1:24 am
by volka
if (isset($_REQUEST['search']) && trim($search) !== "") {
$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND (".$choice."
[...]
}else{

$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability"),0);
$total_pages = ceil($total_results / $max_results);
$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
FROM cars as c
JOIN manufacturer as m ON m.id = c.manufacturer
LEFT JOIN car_images as p ON p.car_id = c.id
WHERE c.availability IN $availability
GROUP BY c.id ORDER BY c.id DESC
LIMIT $from, $max_results";
}
The SELECT statements are all almost the same.
You can pull the recuring parts out of the if/else if/else construct and keep only the variable parts in there. Then you build the complete query only once after the whole if/else...
another hint: indent more carefully/consistent, e.g.

Code: Select all

if (!isset($_GET['page'])) { $page = 1; }else{ $page = $_GET['page']; }

        $max_results = 1;
        $numcols = 4;
        $numcolsprinted = 0;
        $from = (($page * $max_results) - $max_results);

if (isset($_REQUEST['search']) && trim($search) !== "") {
Why is max_results, $numcols... more indented then the ifs before and after?

Posted: Sat Nov 04, 2006 3:33 am
by bob_the _builder
I guess you mean set the WHERE part of the query as a variable within the if's, elseif and set the variable in the query after the if's.

Code: Select all

if (isset($_REQUEST['search']) && trim($search) !== "") {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND (".$choice." LIKE('".$search."'))"),0);
	$where = "WHERE c.availability IN $availability AND (".$choice." LIKE('".$search."'))";

}elseif (isset($_REQUEST['manufacturer']) && trim($manufacturer) !== "") {
    $total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars as c JOIN manufacturer as m ON m.id = c.manufacturer WHERE availability IN $availability AND make ='".$manufacturer."'"),0);
    $where = "WHERE c.availability IN $availability AND m.make='".$manufacturer."'";
			
}elseif (isset($_REQUEST['under'])) {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND price <= '".$under."'"),0);
	$where = "WHERE availability IN $availability AND price <= '".$under."'"; 
			
}elseif (isset($_REQUEST['over'])) {
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability AND price > '".$over."'"),0);
	$where = "WHERE availability IN $availability AND price > '".$over."'"; 

}else{
	$total_results = mysql_result(mysql_query("SELECT COUNT(*) as Num FROM cars WHERE availability IN $availability"),0); 
	$where = "WHERE c.availability IN $availability";
}

	$total_pages = ceil($total_results / $max_results); 
	$sql = "SELECT c.id, c.year, c.manufacturer, c.model, c.availability, p.photo_filename, m.make
    		FROM cars as c
    	    JOIN manufacturer as m ON m.id = c.manufacturer		
			LEFT JOIN car_images as p ON p.car_id = c.id
			".$where."
			GROUP BY c.id 
			LIMIT $from, $max_results";
Is that the best way to go .. or is there a better way than using if, elseif alltogether?

The indentment .. No real reason, just to seperate the variables from the if's so they can easlily be seen at a glance. But when its pasted into php tags in the forum the indentment is all thrown out to how it is in my document.


Thanks

Posted: Sat Nov 04, 2006 4:43 am
by timvw
Since security is part of quality i presume that your application is open for a couple of serious improvements:

- addslashes just doesn't cut it in order to prepare the data for use in a mysql query (lookup the various disucssions on http://www.php.net/mysql_real_escape_string for more info).

- The same story applies for using the user input as output in your html. Your page is open for XSS attacks, eg: $_REQUEST['search'] or $search is outputted without any precaution whatsoever. (lookup http://www.php.net/htmlentities).

Posted: Sat Nov 04, 2006 2:08 pm
by bob_the _builder
Hi,

Yer that was just for posting .. I run code through the following function before querys or inserting into database:

Code: Select all

function ValidateInput($value) {
$value = mysql_real_escape_string(strip_tags(trim($value))); 
return $value; 
}

then:

Code: Select all

ValidateInput($_REQUEST['manufacturer']);
and when calling from the db:

Code: Select all

function ValidateOutput($value) { 
$value = stripslashes(nl2br($value));
return $value; 
}

Try to get my head around

htmlentities should be added to the ValidateOutput function, then add htmlspecialchars_decode to ValidateOutput?


Is there anything else that should be considered?


Thanks

Posted: Sat Nov 04, 2006 2:22 pm
by volka
Why stripslashes?

Posted: Sat Nov 04, 2006 2:25 pm
by bob_the _builder
Hi,

To strip the slashes that were added during the ValidateOutput?


Thanks