I'll spare you my lecture about the virtues of separating your display from your application logic, but I've got to comment on the long 'if' blocks here: if you're typing basically the same thing again and again, chances are you're making things too hard and your code could benefit from some refactoring. Computers are great at doing repetitive stuff, and you can almost always factor out tedium and save yourself and future maintainers time.
For example, if you look at all these conditions, they're very similar:
PHP Code:
foreach($pages_array as $key => $page)
{
if($page['position'] == "between" && !isset($page['active']))
{
$pages_array[$key]['li'] = '<li>' . $page['URL'] . '</li>' . "\n";
}
if($page['position'] == "between" && isset($page['active']))
{
$pages_array[$key]['li'] = '<li id="active">' . $page['URL'] . '</li>' . "\n";
}
// Left
if($page['position'] == "far_left" && !isset($page['active']))
{
$pages_array[$key]['li'] = '<li class="far_left">' . $page['URL'] . '</li>' . "\n";
}
if($page['position'] == "far_left" && isset($page['active']))
{
$pages_array[$key]['li'] = '<li id="active" class="far_left">' .
$page['URL'] . '</li>' . "\n";
}
// Right
if($page['position'] == "far_right" && !isset($page['active']))
{
$pages_array[$key]['li'] = '<li class="far_right">' . $page['URL'] . '</li>' . "\n";
}
if($page['position'] == "far_right" && isset($page['active']))
{
$pages_array[$key]['li'] = '<li id="active" class="far_right">' .
$page['URL'] . '</li>' . "\n";
}
// Top
if($page['position'] == "top" && !isset($page['active']))
{
$pages_array[$key]['li'] = '<li class="top">' . $page['URL'] . '</li>' . "\n";
}
if($page['position'] == "top" && isset($page['active']))
{
$pages_array[$key]['li'] = '<li id="active" class="top">' .
$page['URL'] . '</li>' . "\n";
}
// Bottom
if($page['position'] == "bottom" && !isset($page['active']))
{
$pages_array[$key]['li'] = '<li class="bottom">' . $page['URL'] . '</li>' . "\n";
}
if($page['position'] == "bottom" && isset($page['active']))
{
$pages_array[$key]['li'] = '<li id="active" class="bottom">' .
$page['URL'] . '</li>' . "\n";
}
}
The only difference in effect is whether or not the active id is assigned and the name of the class. So, if you cater to these differences you can simplify the whole thing and remove redundant tests by concentrating on only the dynamic part and consolidating the static:
PHP Code:
foreach($pages_array as $key => $page)
{
// this is always going to be tested, why not move it out of the conditionals and get it over with?
$activeid = (isset($page['active'])) ? ' id="active"' : '';
// what's different about these assignments? the class, and whether or not the active id is assigned
// in every case but between, the class is the same as $page['position'], so...
if ($page['position'] == 'between')
{
$pages_array[$key]['li'] = '<li' . $activeid . '>' . $page['URL'] . '</li>' . "\n";
}
else
{
$pages_array[$key]['li'] = '<li' . $activeid . ' class="' . $page['position'] . '">' . $page['URL'] . '</li>' . "\n";
}
}
In a somewhat related issue, it doesn't make sense to test an iterative index against a known static to perform some action. Here:
PHP Code:
foreach($pages_array as $key => $page)
{
if($key == 1 && $this->aSettings['orientation'] == "horizontal")
{$pages_array[$key]['position'] = 'far_left';}
if($key == 1 && $this->aSettings['orientation'] == "vertical")
{$pages_array[$key]['position'] = 'top';}
if($key != 1 && $key != $num_of_pages)
{$pages_array[$key]['position'] = 'between';}
if($key == $num_of_pages && $this->aSettings['orientation'] == "horizontal")
{$pages_array[$key]['position'] = 'far_right';}
if($key == $num_of_pages && $this->aSettings['orientation'] == "vertical")
{$pages_array[$key]['position'] = 'bottom';}
}
If you want to assign something to $array[$key] when $key is 1, cut out the middle man and assign to $array[1]:
PHP Code:
// set the starting point. no need to test iteratively since the value is known
$pages_array[1]['position'] = ($this->aSettings['orientation'] == 'horizontal') ? 'far_left' : 'top';
// fill in the middle
for ($key = 2; $key < $num_of_pages; ++$key)
{
$pages_array[$key]['position'] = 'between';
}
// set the endpoint
$pages_array[$num_of_pages]['position'] = ($this->aSettings['orientation'] == 'horizontal') ? 'far_right' : 'bottom';
Hope this helps, or at least that it isn't completely insufferable.