Navigation bar using PHP

Question

I would like to know what you think:

<?php
 $nav_normal = array("1. Home","2. Read Me","3. License Agreement","4. General Information","5. Database Installer","6. Create an Account","7. Create Config","8. Successfully installed"
    );

$last_normal = array_pop(array_keys($nav_normal));

foreach($nav_normal as $name){

    if($Nav_ID == $name){
            $Nav_Type = "Selected";
    }else{
            $Nav_Type = "Unselected";
    }

    ?><li class="<?php echo($Nav_Type); ?>"><?php echo($name); ?></li><?php
            //this part is only for the (google chrome) view-source:\\
            //this will not be shown into the normal layout/output\\
            if($name != $last_normal){
                    echo("\n                                        ");
            }else{
                    echo("\n");
            }

}  

On each page I write at the place of the navigation bar:

<?php
 $Nav_ID = "1. Home"; //etc. etc.\\
 include_once("navigation.php");
?>

I hope you can give me a review on what you think is either good or bad. Just give your opinion.

Answer 1

<?php

// first up i have removed the numbers, those numbers can be automatically added using an ordered list,
// then if you add a menu option later you don't have to renumber everything and look through every file to check the $Nav_ID matches
$nav_normal = array("Home","Read Me","License Agreement","General Information","Database Installer","Create an Account","Create Config","Successfully installed");

// removed as documented below
// $last_normal = array_pop(array_keys($nav_normal));

// we need to start the list somewhere?, using an ordered list will automatically number the menu options
echo "<ol>";

foreach($nav_normal as $name){

    if($Nav_ID == $name){
        // $Nav_Type is not very descriptive
        // $Nav_Type = "Selected";
        $Nav_Selected = "Selected";
    } else {
        $Nav_Selected = '';
        // it is not necessary to have a state for unselected
//        $Nav_Type = "Unselected";
    }

    // the last if/else block could be replaced by this single line
    $Nav_Selected = ($Nav_ID == $name) ? "Selected" : '';



    // it is not worth escaping php for a single, line, it is easier just to embed the html and echo it
    // make sure we call htmlspecialchars or characters like >< & " etc will stuff up your html
    echo "<li class=\"$Nav_Selected\">".htmlspecialchars($name)."</li>";

    // this makes no visible difference, multiple spaces are rendered as one in html
//    if($name != $last_normal){
//        echo("\n                                        ");
//    }else{
        echo "\n";
//    }

}

// if we want extra spaces after the last item in the loop, just echo them after the loop has finished
echo "                                        ";

// don't forget to close the ordered list too
echo "</ol>";

Answer 2

Make better use of the array for the configuration of your items.

<?php
// Your array
$navigation[
  id => 'my-navigation',
  class => 'my-nav-class',
  items => [
    [label => 'Home', href => '/home/', active => true],
    [label => 'Read me', href => '/readme/'],
    [label => 'License Agreement', href => '/agreement/'],
  ]
];


// The output
echo '<ul id="' . $navigation['id'] . '" class="' . $navigation['class'] . '">';

foreach( $navigation['items'] as $item  ){  
  // Build up other item classes
  if( $item['active'] ) $class += ' active';
  if( $item['class'] ) $class += ' ' . $item['class'];

  echo '<li class="' . $class . '">';
    echo '<a href="' . $item['href'] ?: '#' . '">' . $item['label'] . '</a>';
  echo '</li>';
}

echo '</ul>'

You'll end up with a more flexible solution this way, although there is also nothing wrong with how you've done it really.

Considering you're controlling the content I wouldn't bother using htmlspecialchars() i'd instead build the array with the appropriate markup.

Certainly wouldn't break out of the tags for the markup, it's harder to read and more error prone. It would be better to have true separation of markup with a template language like twig.

Without knowing context of how it's going to be used it hard to give anymore pointers really.