Reputation: 48983
Is there a better way to do this PHP code?
My PHP site has header,body,footer files that are included into the INDEX.PHP page.
On the header.php file I have a menu like this
<div id="bottomrow">
<div class="pad">
<ul class="menu left">
<li class="first <?=$current_home?>"><a href="/"><em>Home</em></a></li>
<li class="users drop <?=$current_users?>"><a href=""><em>Users</em></a><span class="drop"> </span>
<ul id="moreheader">
<li></li>
<li></li>
</ul>
</li>
<li class="<?=$current_forum?>"><a href=""><em>Forums</em></a></li>
<li class="drop <?=$current_more?>"><a href="/moreheader"><em>More</em></a><span class="drop"> </span>
<ul id="moreheader">
<li><a href=""><em>Widgets</em></a></li>
<li><a href=""><em>News</em></a></li>
<li><a href=""><em>Promote</em></a></li>
<li><a href=""><em>Development</em></a></li>
<li><a href=""><em>Bookmarks</em></a></li>
<li><a href=""><em>About</em></a></li>
</ul>
</li>
<li class="moneytabmenu <?=$current_money?>"><a href="/moneytabmenu"><em>Money:<span class="moneytabmenu-total">$0.00</span></em></a></li>
</ul>
<ul class="menu right">
<li class="drop myaccount <?=$current_myaccount?>"><a href="" class="first"><img class="avatar" src="http://gravatar.com/avatar.php?gravatar_id=7ab1baf18a91ab4055923c5fd01d68a2&rating=pg&size=80&default=" height="19" width="19" alt="you" /><em>My
Account</em></a><span class="drop"> </span>
<ul id="myaccount">
<li><a href=""><em>Dashboard</em></a></li>
<li><a href=""><em>Account Settings</em></a></li>
<li><a href=""><em>Settings</em></a></li>
</ul>
</li>
<li class="drop"><a href=""><em>Mail</em></a><span class="drop"> </span>
<ul id="mailboxheader">
<li><a href=""><em>InBox</em></a></li>
<li><a href=""><em>SentBox</em></a></li>
<li><a href=""><em>Trash</em></a></li>
<li><a href=""><em>Post Bulletin</em></a></li>
<li><a href=""><em>View Bulletins</em></a></li>
</ul>
</li>
<li class="drop <?=$current_more?>"><a href=""><em>More</em></a><span class="drop"> </span>
<ul id="moreheader">
<li><a href=""><em>Widgets</em></a></li>
<li><a href=""><em>News</em></a></li>
<li><a href=""><em>Promote</em></a></li>
<li><a href=""><em>Development</em></a></li>
<li><a href=""><em>Bookmarks</em></a></li>
<li><a href=""><em>About</em></a></li>
</ul>
</li>
</ul>
</div>
</div>
<!-- END div#bottomrow -->
</div>
The menu is similar to above, the above menu is not completed but you can see how it is set up, there are
If a user is on any of the pages in a sub-menu, it should add the "current" css class to the parent menu item.
Below is my index.php page where I can determine which page I am on using $_GET for the variable index.php?p=PAGE-NAME
I posted a question earliar similar to this but it was before I did the code below and it didnt get much response, the response it did get all mentioned using arrays but that was beofre they new that I had sub-menus.
So does anyone see a better way to add the css class "current" to a list item above based on which page I am on?
Code on INDEX.php page that processes which menu item should be highlighted
//set variables for menu item to appear as being SELECTED
$p = $_GET['p'];
$current_home = '';
$current_users = '';
$current_forum = '';
$current_more = '';
$current_money = '';
$current_myaccount = '';
$current_mail = '';
//if home then highlight home menu
if($p === 'home'){
$current_home = 'current';
}
if($p === 'users.online' || $p === 'users.location' || $p === 'users.featured' || $p === 'users.new' || $p === 'users.browse' || $p === 'users.search' $p === 'users.staff'){
$current_users = 'current';
}
if($p === 'forum'){
$current_forum = 'current';
}
if($p === 'widgets' || $p === 'news' || $p === 'promote' || $p === 'development' || $p === 'bookmarks' || $p === 'about'){
$current_more = 'current';
}
if($p === '=account.money' || $p === 'account.store' || $p === 'account.lottery' || $p === 'users.top.money'){
$current_money = 'current';
}
if($p === 'account'){
$current_myaccount = 'current';
}
if($p === 'mail.inbox' || $p === '=mail.sentbox' || $p === 'mail.trash' || $p === 'bulletins.post' || $p === 'bulletins.my' || $p === 'bulletins'){
$current_mail = 'current';
}
Upvotes: 0
Views: 254
Answers (5)
Reputation: 35607
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.
Upvotes: 1
Reputation: 37143
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.
Upvotes: 0
Reputation: 1695
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.
Upvotes: 1
Reputation: 1507
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>
Upvotes: 2
Reputation: 1831
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.
Upvotes: 0