Tips on streamlining code

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
stark-johan
Forum Newbie
Posts: 3
Joined: Tue Feb 10, 2009 4:29 pm

Tips on streamlining code

Post 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
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Tips on streamlining code

Post 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.
(#10850)
stark-johan
Forum Newbie
Posts: 3
Joined: Tue Feb 10, 2009 4:29 pm

Re: Tips on streamlining code

Post 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;
 
?>
stark-johan
Forum Newbie
Posts: 3
Joined: Tue Feb 10, 2009 4:29 pm

Re: Tips on streamlining code

Post 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;
 
?>
Post Reply