current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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>
share|improve this question

1 Answer 1

up vote 1 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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