Page 1 of 1

Tips on streamlining code

Posted: Tue Feb 10, 2009 5:09 pm
by stark-johan
I've been looking for an active PHP forum for a while and I just stumbled over this one. I hope this will become my go to place for PHP related info. I'm rather new when it comes to anything but basic PHP but I think it's really fun to work with and I hope to learn more in the near future.

In this, my first post, I'll present a working but perhaps not so smart way of generating a dropdown javascript menu using mysql. The menu is written by Michael Leigeber, is very lightweight and can be found over at his site: http://www.leigeber.com/2008/11/drop-down-menu/

I'm sure my script can be improved and that's why I'm posting it here. I think that feedback from more experienced programmers is the best way to acquire knowledge so here goes.

The database

Code: Select all

+---------------+
| Tables_in_cea |
+---------------+
| eng_menu      | 
| eng_sub_cea   | 
| eng_sub_join  | 
| eng_sub_who   | 
+---------------+
 
The main menu table (eng_menu). All the other submenus have the same structure, that way it's easy to change the menu tree and even moving a submenu up/down a level if needed.

Code: Select all

+----+---------------+-------------+--------------+
| id | name          | link        | expand_table |
+----+---------------+-------------+--------------+
|  1 | Who are we?   | who.php     | eng_sub_who  | 
|  2 | Stake Holders | stake.php   |              | 
|  3 | Join us!      | join.php    | eng_sub_join | 
|  4 | Events        | events.php  |              | 
|  5 | Contact       | contact.php | eng_sub_cea  | 
+----+---------------+-------------+--------------+
 

Code: Select all

<?php
function display_menu()
{
    $sql = "SELECT * FROM eng_menu";
    $result = mysql_query($sql);
    
$menu .= "<ul class=\"menu\" id=\"menu\">\r";
 
while ($row = mysql_fetch_assoc($result))
{
    if (empty($row["expand_table"])) // Does not expand first level
    {
        $menu .= "<li><a href=\"$row[link]\" class=\"menulink\">$row[name]</a></li>\r";
    }
    else // Does expand first level
    {
        $menu .= "<li><a href=\"$row[link]\" class=\"menulink\">$row[name]</a>\r<ul>\r";
        $sql2 = "SELECT * FROM ".$row["expand_table"];
        $result2 = mysql_query($sql2);
        
        while ($row2 = mysql_fetch_assoc($result2))
        {
            if (empty($row2["expand_table"])) // Does not expand second level
            {
                $menu .= "<li><a href=\"$row2[link]\">$row2[name]</a></li>\r";
            }
            else // Does expand second level
            {
                $menu .= "<li><a href=\"$row2[link]\" class=\"sub\">$row2[name]</a>\r<ul>\r";
                $sql3 = "SELECT * FROM ".$row2["expand_table"];
                $result3 = mysql_query($sql3);
                $i = 1;
                while ($row3 = mysql_fetch_assoc($result3))
                {
                    if ($i == 1)
                    {
                        $menu .= "<li class=\"topline\"><a href=\"$row3[link]\">$row3[name]</a></li>\r";
                    }
                    else
                    {
                        $menu .= "<li><a href=\"$row3[link]\">$row3[name]</a></li>\r";
                    }
                    $i++;
                }
                $menu .= "</ul>\r</li>\r";
            }
        }
        $menu .= "</ul>\r</li>\r";
    }
}
mysql_free_result($result);
mysql_free_result($result2);
mysql_free_result($result3);
mysql_close();
 
echo $menu;
}
 
display_menu();
?>
 
I'm sure this can be done more efficiently but at least it does the trick. Any feedback is greatly appreciated. Here's a demo: http://www.snutt.net/testzon/cea/bygg_meny.php

Re: Tips on streamlining code

Posted: Tue Feb 10, 2009 7:57 pm
by Christopher
I think the fact that you have mysql calls both inside and outside the function could be improved. I would move all the database stuff outside the function and just pass in an array. It would make it more focused on a single task.

Re: Tips on streamlining code

Posted: Wed Feb 11, 2009 5:15 pm
by stark-johan
I rewrote the script today with a different approach. I think the code is a bit neater but the new version executes slightly slower than my original one.

Any comments on the new one compared to the first one?

Code: Select all

// Main menu table name
$table = "eng_menu";
 
function makearray($table)
{
    // Collect data from main table
    $sql = "SELECT * FROM ".$table;
    $result = mysql_query($sql);    
 
    // Construct array from table data
    while ($arr = mysql_fetch_array($result))
    {
        $arr1[$arr[1]]['name'] = $arr[1];
        $arr1[$arr[1]]['link'] = $arr[2];
        $arr1[$arr[1]]['sub'] = $arr[3];
    }
    return $arr1; 
    mysql_free_result($result);
    mysql_close();
}
 
$top = makearray($table);
 
 
$out .= "<ul class=\"menu\" id=\"menu\">\r";
 
foreach ($top as $menu)
{
    $out .= "<li><a href=\"$menu[link]\" class=\"menulink\">$menu[name]</a>";
    
    if (empty($menu['sub']))
    {
        $out .= "</li>\r";
    }
    else
    {
        $top = makearray($menu['sub']);
        $out .= "\r<ul>\r";
        foreach ($top as $menu)
        {
            if (empty($menu['sub']))
            {
                $out .= "<li><a href=\"$menu[link]\">$menu[name]</a></li>\r";
            }
            else
            {
                $out .= "<li><a href=\"$menu[link]\" class=\"sub\">$menu[name]</a>\r<ul>\r";
                $top = makearray($menu['sub']);
                $i = 0;
                foreach ($top as $menu)
                {   
                    $i++;
                    if ($i == 1) {$class = "topline";}else{$class = "none";}
                    $out .= "<li class=\"$class\"><a href=\"$menu[link]\">$menu[name]</a></li>\r";
                }
                $out .= "</ul>\r</li>\r";   
            }
            
        }
        $out .= "</ul>\r</li>\r";
    }
 
}
 
echo $out;
 
?>

Re: Tips on streamlining code

Posted: Thu Feb 12, 2009 1:25 pm
by stark-johan
Ok, a third version that is as fast as the first one and pretty nice if I may say so myself. It can easily be extended to third level and beyond as they're identical to the second level.

Any comments?

Code: Select all

// Main menu table name, submenus are looped to second level (main-first-second).
$table = "eng_menu";
 
function makeMenuArray($table)
{
    // Collect data from main table
    $sql = "SELECT * FROM ".$table;
    $result = mysql_query($sql);    
 
    // Construct array from table data
    while ($array = mysql_fetch_array($result))
    {
        $arr[$array[0]]['name'] = $array[1];
        $arr[$array[0]]['link'] = $array[2];
        $arr[$array[0]]['sub'] = $array[3];
    }
    return $arr; 
    mysql_free_result($result);
    mysql_close();
}
 
// Sets main array ($tbl_"table_name")
${"tbl_$table"} = makeMenuArray($table);
 
// Sets submenu arrays
foreach (${"tbl_$table"} as $menu)
{
    if (!empty($menu['sub']) && !isset(${"tbl_.$table"}))
    {
        // First level
        ${"tbl_.$menu[sub]"} = makeMenuArray($menu['sub']);
        foreach (${"tbl_.$menu[sub]"} as $menu)
        {
            if (!empty($menu['sub']) && !isset(${"tbl_.$menu[sub]"}))
            {
                // Second level
                ${"tbl_.$menu[sub]"} = makeMenuArray($menu['sub']);
            }       
        }   
    }
}
 
// Display menu, main level
$out .= "<ul class=\"menu\" id=\"menu\">\r";
foreach (${"tbl_$table"} as $menu)
{
    $out .= "<li><a href=\"".$menu['link']."\" class=\"menulink\">".$menu['name']."</a>";
    
    if (empty($menu['sub']))
    {
        $out .= "</li>\r";
    }
    else
    {
        // Display first level
        $out .= "\r<ul>\r";
        foreach (${"tbl_.$menu[sub]"} as $menu)
        {
            if (empty($menu['sub']))
            {
                $out .= "<li><a href=\"".$menu['link']."\">".$menu['name']."</a></li>\r";
            }
            else if (!empty($menu['sub']))
            {
                // Display second level
                $out .= "<li><a href=\"".$menu['link']."\" class=\"sub\">".$menu['name']."</a>\r<ul>\r";
                $i = 0;
                foreach (${"tbl_.$menu[sub]"} as $menu)
                {   
                    ++$i;
                    if ($i == 1)
                    {
                        $class = "topline";
                    }
                    else
                    {
                        $class = "none";
                    }
                    $out .= "<li class=\"$class\"><a href=\"".$menu['link']."\">".$menu['name']."</a></li>\r";
                }
                $out .= "</ul>\r</li>\r";
            }
        }
        $out .= "</ul>\r</li>\r";
    }
}
 
echo $out;
 
?>