2
\$\begingroup\$

I have the following script and I think it's a bit inefficient. With a array from 1 to 3, this isn't a real problem, but this array can get way bigger. So how to boost it's performance/optimize this code?

<?php 
$array = array(1, 2, 3); 
foreach ($array as $tabid)
    {
    echo '<div id="tab'. $tabid. '" class="tab_content"> 
    <ul class="columns">'; 

        $result = mysql_query("SELECT * FROM TabContent WHERE TabID = $tabid");
        while($row = mysql_fetch_array($result))
          {
          echo '<li><a href="#"><img src="images/layouts/'. $row['LayoutName'] .'.png" alt="" /></a>
                    <div class="info"> 
                        <h2>'. $row['LayoutName'] .'</h2> 
                        <p>'. $row['LayoutTiles'] . 'Tiles, '. $row['LayoutLayers'] . 'Layers</p>
                    </div> 
                </li>';
          }  
   echo ' </ul>
    <div class="clear"></div>
    <div class="bottom">
        <div class="clearfix">
            <div class="lfloat"></div>
            <div class="rfloat"><a class="hide" href="#" onclick="return false">Cancel</a></div>
        </div>
    </div>  
    </div>  '; 
    }
?>

Kind regards, Maurice

edit, optimized code I have so far:

<?php
    $array = array(1, 2, 3); 
    $result = mysql_query("SELECT * FROM TabContent WHERE TabID IN (".implode(",",$array).") ORDER BY TabID");
    $tab = -1;
    while($row = mysql_fetch_assoc($result)) {
        if ($tab != $row{'TabID'}) {
            if ($tab > -1) {
                 echo '</ul>
                        <div class="clear"></div>
                        <div class="bottom">
                            <div class="clearfix">
                                <div class="lfloat"></div>
                                <div class="rfloat"><a class="hide" href="#" onclick="return false">Cancel</a></div>
                            </div>
                        </div>  
                    </div>  ';
            }
            $tab = $row{'TabID'}; 
            echo '<div id="tab'. $tab. '" class="tab_content">
                    <ul class="columns">'; 
        }
        echo '<li><a href="#"><img src="images/layouts/'. $row['LayoutName'].'.png" alt="" /></a> 
                <div class="info">
                    <h2>'.$row['LayoutName'].'</h2>
                    <p>'.$row['LayoutTiles'].'Tiles, '.$row['LayoutLayers'].'Layers</p>
                </div>
              </li>';
    } ?>
\$\endgroup\$
  • \$\begingroup\$ I obviously have no idea how many columns TableContent has, but it's generally considered bad practice to select * in actual product code. It's more readable and (depending on how many other unused columns you are returning) more performant to explicitly call out each Column you wish to select. i.e. "SELECT LayoutNames, LayoutTiles, LayoutLayers from TableContent .... " \$\endgroup\$ – Brian Gianforcaro Dec 11 '11 at 21:23
4
\$\begingroup\$

just skip the foreach and do it in 1 query, there are number of ways to do this for example by using IN http://www.techonthenet.com/sql/in.php

     $result = mysql_query("SELECT * FROM TabContent WHERE TabID IN (".implode(",",$array).") ORDER BY TabID");

and keep track of the tabID when to print the foreach divs and etc

EDIT: some example code, this is one of the many ways you can do this

# code is not tested
define('OPEN',1);
define('CLOSE',2);
function ul($type,$tabid = 0) {
    switch ($type) {
    case OPEN:
        return '<div id="tab'.$tabid.' class="tab_content"><ul class="columns">';
        break;
    case CLOSE:
    default:
        return '</ul><div class="clear"></div><div class="bottom"><div class="clearfix"><div class="lfloat"></div><div class="rfloat"><a class="hide" href="#" onclick="return false">Cancel</a></div></div></div>  </div>';
        break;
    }
}

$array = array(1, 2, 3); 
$result = mysql_query("SELECT * FROM TabContent WHERE TabID IN (".implode(",",$array).") ORDER BY TabID");
$tab = -1;
$opened = 0;
while($row = mysql_fetch_assoc($result)) {
    if ($tab != $row{'TabID'}) {
        if ($tab > -1) {
             echo ul(CLOSE);
             $opened--;
        }
        $tab = $row{'TabID'}; 
        echo ul(OPEN,$tab);
        $opened++;
    }
    echo '<li><a href="#"><img src="images/layouts/'. $row['LayoutName'].'.png" alt="" /></a> <div class="info"><h2>'.$row['LayoutName'].'</h2><p>'.$row['LayoutTiles'].'Tiles, '.$row['LayoutLayers'].'Layers</p></div></li>';
}
if ($opened > 0)
    echo ul(CLOSE);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Okay I understand what you are saying (sort of), but I have no idea how to do this code wise \$\endgroup\$ – Maurice Dec 11 '11 at 21:04
  • \$\begingroup\$ Okay, played around with this and did some small changes and it all works. Except for 1 little part: if ($tab != $row{'TabID'}) { if ($tab > -1) { echo '</ul> <div class="clear"></div> <div class="bottom"> <div class="clearfix"><div class="lfloat"></div><div class="rfloat"><a class="hide" href="#" onclick="return false">Cancel</a></div></div></div></div> '; } This adds the closing part of each div, this is added for div 1 and 2, but not the final div 3. I have added my code in my first post. \$\endgroup\$ – Maurice Dec 11 '11 at 22:55
  • \$\begingroup\$ Hmm doesn't get it working, why doesn't it show the last echo '</ul> part for the 3rd div? \$\endgroup\$ – Maurice Dec 12 '11 at 19:34
  • \$\begingroup\$ @Maurice hey again, the idea is to know when the tabid changes, so you can print the open/close ul (first you open it) and then if $tab > -1 (init value) you close and open again), at least that is the idea. the final /ul you must add below the code, because there wont be $tab != $row{'TabID'} for the last tabid, i will modify the code to illustrate it in a sec. \$\endgroup\$ – jackdoe Dec 12 '11 at 19:46
  • \$\begingroup\$ Thanks for the help, your code is a bit advanced, but I understand what it does. This will help me a lot understanding coding things like this. For now I simply added the html part at the bottom of the original script (this works too) and implement your code later when I have had a better look at it. Thanks again, you helped me a lot and I actually learned something too :) (and all others who contributed too ofcourse) \$\endgroup\$ – Maurice Dec 12 '11 at 21:47
2
\$\begingroup\$

You could do this SQL

$result = mysql_query("SELECT * FROM TabContent WHERE TabID IN (".implode(',',$array).")");
foreach($row = mysql_fetch_assoc($result)) ....

and then run your foreach. Then you have less SQL requests, which could lead to better performance

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I think you are all saying the same thing, but how would the complete code look like? How would my foreach look like? i understand the foreach as I've build it, but I understand that the query in the foreach is the slowing factor. No idea how to do this different though \$\endgroup\$ – Maurice Dec 11 '11 at 21:06
2
\$\begingroup\$

Quering to database server is intensive task instead of that modify your query to

$result = mysql_query("SELECT * FROM TabContent WHERE TabID IN ( $valueSet) SORT BY TabID ";

You can get $valueSet by joining above array, and you are getting sorted ouput, so you can use this foreach loop to iterate over all the rows.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Pulling the query out of the loop is absolutely the biggest optimization you can make, SQL queries are relatively expensive, doing them in a loop will be more expensive still! In nine cases out of ten, figuring out how to grab all the data in a single query is faster than doing the query in a loop.

SELECT * 
FROM TabContent 
WHERE TabID IN (1, 2, 3) 
ORDER BY TabID;
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I was also thinking the SQL query in the loop was inefficient, unfortunately I have no idea how to this another way \$\endgroup\$ – Maurice Dec 11 '11 at 20:57
  • \$\begingroup\$ Edited my answer with a single query that will fetch all the tabs \$\endgroup\$ – GordonM Dec 12 '11 at 8:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.