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

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

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

Post 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
Last edited by bob_the _builder on Fri Nov 03, 2006 7:30 pm, edited 2 times in total.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

You want us to do it for you?
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post 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
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post by bob_the _builder »

The word help means "Do it for me" ?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post 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
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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.
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post 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
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post 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?
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post 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
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post 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).
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post 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
Last edited by bob_the _builder on Sat Nov 04, 2006 2:22 pm, edited 1 time in total.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post by volka »

Why stripslashes?
bob_the _builder
Forum Contributor
Posts: 131
Joined: Sat Aug 28, 2004 12:25 am

Post by bob_the _builder »

Hi,

To strip the slashes that were added during the ValidateOutput?


Thanks
Post Reply