3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for editing it I guess just don't see why php is wrong and "must" be PHP \$\endgroup\$ – Mike M. May 5 '14 at 20:45
  • \$\begingroup\$ What are you trying to achieve with $last_normal? Multiple spaces in html are displayed as 1 space, so both conditions based on $name != $last_normal would look the same in a web browser? \$\endgroup\$ – bumperbox May 5 '14 at 20:50
  • \$\begingroup\$ the multiple spaces is working great it's to line out in the view-code: also the last_normal gives the last item in the array so while the $name is not the last_normal (last item) then it shows the correct way in the view-code: else it just shows it back as normal with just one next space... (if you get what I mean) \$\endgroup\$ – Mike M. May 5 '14 at 20:54
  • \$\begingroup\$ Instead of view source, you could use Chrome's (or Firefox's or etc.) inspection functionality, which presents the HTML in a formatted fashion (with other features too). Then you won't need all the logic for spacing. \$\endgroup\$ – cbojar May 5 '14 at 23:39
  • \$\begingroup\$ well I thought it would be nicer for that kind of view aswell else it would be like {spaces}item {n} item | now it's: {spaces}item {n} {spaces}item \$\endgroup\$ – Mike M. May 6 '14 at 21:47
3
\$\begingroup\$
<?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>";
\$\endgroup\$
  • \$\begingroup\$ just a few issues with your script array pop bla bla gave me an issue when I added a new item so.. also it may not make a visual difference but the view-code: does and I am going to use it in my forum software (open-source) thanks for the tips tho \$\endgroup\$ – Mike M. May 5 '14 at 21:17
  • \$\begingroup\$ also my nav_type is a class (selected = class part && unselected = class part) regarding to my css rile :) \$\endgroup\$ – Mike M. May 5 '14 at 21:49
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I think an object for each item would be great too so he could loop the items array and check if the url matches the route to flag it as active \$\endgroup\$ – SparK May 6 '14 at 14:13

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.