2
\$\begingroup\$

I am looking for better ways to write this code. At the moment it grabs a list of all navigation items and places them into an array (using PDO).

However, as you can see, it is using two foreach loops to grab the subnavigation items under each parent. Is there a better way that this code can be written?

<ul id="nav">
    <?php
    if($grabNav = $db->prepare("SELECT caption,url,visibility,id,class,parent_id FROM navigation ORDER BY parent_id ASC, order_id ASC"))
    {
        $grabNav->execute();
        $grabNav = $grabNav->fetchAll();
        foreach($grabNav as $nav)
        {
            $visibility = true;
            switch($nav['visibility'])
            {
                default:
                case 0: $visibility = false; break;
                case 1: $visibility = true; break;
                case 2: if(LOGGED_IN && isset($cUser)) { $visibility = true; } else { $visibility = false; } break;
                case 3: if(LOGGED_IN && isset($cUser)) { $visibility = false; } else { $visibility = true; } break;
            }
            if(!$visibility) { continue; }
            if($nav['parent_id'] != -1) { continue; }

            $class = $core->output($nav['class']);
            if($nav['id'] == PAGE_ID) { $class .= ' selected'; }
            echo '<li class="'.$class.'"><a href="'.$core->output($nav['url']).'">'.$core->output($nav['caption'],true).'</a><ul id="subNav">';
            foreach($grabNav as $sub)
            {
                if($sub['parent_id'] == $nav['id'])
                {
                    $visibility = true;
                    switch($sub['visibility'])
                    {
                        default:
                        case 0: $visibility = false; break;
                        case 1: $visibility = true; break;
                        case 2: if(LOGGED_IN && isset($cUser)) { $visibility = true; } else { $visibility = false; } break;
                        case 3: if(LOGGED_IN && isset($cUser)) { $visibility = false; } else { $visibility = true; } break;
                    }
                    if(!$visibility) { continue; }
                    $subClass = $core->output($sub['class']);
                    echo'<li class="'.$subClass.'"><a href="'.$core->output($sub['url']).'">'.$core->output($sub['caption'],true).'</a></li>';
                }
            }
            echo'</ul></li>';
        }
    }
    ?>
</ul>
\$\endgroup\$
1
\$\begingroup\$

Return Early

If you extract the PHP code to a function, you can return early, and thus save a level of nesting:

<ul id="nav">
<?php printNavigation(); ?>
</ul>

<?php
function printNavigation() {
    if(!($grabNav = $db->prepare("SELECT caption,url,visibility,id,class,parent_id FROM navigation ORDER BY parent_id ASC, order_id ASC"))) {
        return;
    }
    [...]
    ?>
}

Remove Duplication and further simplifications

Currently, your visibility check and your one long echo line are duplicates. You can extract these to functions:

function isVisible($visibilityLevel, $cUser) {
    switch($visibilityLevel, $cUser)
    {
        case 0: return false;
        case 1: return true;
        case 2: return LOGGED_IN && isset($cUser);
        case 3: return !LOGGED_IN || !isset($cUser);
        default: throw new Exception('invalid visibility level.');
    }
}

And then use it as

    foreach($grabNav as $nav)
    {
        if(!isVisible($nav['visibility'], $cUser) || $nav['parent_id'] != -1) { continue; }
        [...]

Note also that I removed the useless default from the beginning of the switch, added it again at the end, and threw an exception for invalid values.

I also simplified cases 2 and 3 (if (cond) true else false can be phrased as cond), and I combined the two continue conditions.

\$\endgroup\$

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.