Is there a better way to do this PHP code?

Answer 1

Your HTML can't be simplified much further, but the PHP can probably be done better. It's generally a bad idea to use global variables, but you could probably do something like this instead:

function current_tab($page) {
    global $p;
    // Check if $page is a prefix for $p
    if ($page == substr($p, 0, strlen($page))) {
        return "current";
    }
}

Then in your HTML, use this:

<?=current_tab('account');?>

For the non-conforming page names, you're going to need to use special conditions, or you could change the naming convention.

Answer 2

First, I would suggest creating a menu class to handle the creation of this menu and call the class method in your index.php. This would separate the code from the presentation, provide abstraction, etc. When you call it you could pass it the page, and the class takes care of the implementation and passes the complete menu back.

Second, I would suggest using a switch statement instead of if...else with lots of or conditions. You can cascade cases by not including the break keyword:

switch( $page) {
  case: 'home':
    $current_home = 'current';
    break;
  case: 'users.online':
  case: 'users.location':
  case: 'users.featured':
  .......
    $current_users = 'current';
    break;
}

As far as your PHP use goes, you're not using much. As I said with your if statements, I think a switch would do much better, the $page would be evaluated once and matched once, unlike each if. If you could use an loop for the menu, it could cut down on the amount of actual literal HTML in your code. If you created arrays to hold each main menu piece (maybe what the others meant), then you could loop over the array and combine the array content, some html, and your variables to create the menu system. Less literal HTML and more efficient use of the PHP.

Answer 3

There's a whole lot you can improve here (maintainability and extensibility are pretty low), but I'd start off with eliminating a few of those conditionals... for example, in the first one, you can check whether the string starts with 'users', which will take care of all the specific cases of 'user'.

if(strpos($p, 'users') !== false) { ... }

Same holds for a few of the other cases.

Anyhow, I suggest you take a look at your server-side script structure. Usually, you can find a more generic solution for this stuff.

Answer 4

I would recommend making the whole thing more data driven. Create a multi-dimensional array of your menu options and then iterate through the array, outputting the correct HTML.

Here's how I would do it (this hasn't been tested, but the idea is what I'm trying to convey):

<?
 $p = $_GET['p'];

   $navArray = array();
    $navArray['home'] = array('main' => array('id'=>'home', 'url'=>'/', 'title'=>'Home'));
    $navArray['more'] = array('main' => array('id'=>'more', 'url'=>null, 'title'=>'More'), 
         'pages'=> array(
        array('id'=>'widgets', 'url'=>'/more-widgets.php', 'title'=>'Widgets'),
        array('id'=>'news', 'url'=>'/news.php', 'title'=>'News')));

?>


<div id="bottomrow"> 
      <div class="pad"> 
        <ul class="menu left">

   <? foreach($navArray as $navHeading) : ?>
    <? $current = $navHeading['main']['id'] == $p ? 'current' : ''; ?>
            <li class="first <?=$current?>"><a href="<?=$navHeading['main']['url']?>"><em><?=$navHeading['main']['title']?></em></a></li>

    <? if (!empty($navHeading['pages'])) : ?>
     <ul id="sub-<?=$navHeading['main']['id']?>">
     <? foreach($navHeading['pages'] as $navPage) : ?>
      <? $current = $navPage['id'] == $p ? 'current' : ''; ?>
      <li class="<?=$current?>"><a href="<?=$navPage['url'];?>"><em><?=$navPage['title']?></em></a></li>
     <? endforeach; ?>
     </ul>
    <? endif; ?>
   <? endforeach; ?>
  </ul>
 </div>
</div>

Answer 5

I would suggest having a look at a MVC framework like CodeIgniter (although there are many in the PHP space, CI is my choice).

You are on the path to writing your own from the ground-up ... with a framework like CI you can re-use nearly all of your existing logic, and just structure it a little different gaining clean separation of concerns and solid templating.