CODEWITHSUNDEEP
phphtmlrefactoring
John Smith
John Smith

Reputation: 8811

How to refactor these if statements?

I have this code:

<div id="menu">
    <ul>
        <li><span id="menulabel">Your Languages</span></li>
        <?php
        $hotClass = '';
        $newClass = '';
        $topClass = '';
        if ($sort == 'hot')
            $hotClass = 'active';
        else if ($sort == 'new')
            $newClass = 'active';
        else if ($sort == 'top')
            $topClass = 'active';
        ?>
        <li><a id="Hot" href="index.php?sort=hot&page=1" class="<?php echo $hotClass; ?>">Hot</a></li>
        <li><a id="New" href="index.php?sort=new&page=1" class="<?php echo $newClass; ?>">New</a></li>
        <li><a id="Top" href="index.php?sort=top&page=1" class="<?php echo $topClass; ?>">Top</a></li>
    </ul>
</div>

Depending on which sorting the page is using a different menu item is highlighted to show the user. However, I really dislike this code because the class is empty instead of not present when the list item is not active. Also, every if statement does basically the same thing. Is it possible to refactor this in to something more elegant and readable? Thanks.

Upvotes: 1

Views: 256

Answers (7)

Ateş G&#246;ral
Ateş G&#246;ral

Reputation: 140042

Since you're dealing with presentation here (i.e. adding a CSS class that most likely just changes appearance), you could also apply the styling with JavaScript (ala Progressive Enhancement):

<div id="menu" data-sort="<?php echo $sort; ?>">
    <ul>
        <li><span id="menulabel">Your Languages</span></li>
        <li><a id="hot" href="index.php?sort=hot&page=1">Hot</a></li>
        <li><a id="new" href="index.php?sort=new&page=1">New</a></li>
        <li><a id="top" href="index.php?sort=top&page=1">Top</a></li>
    </ul>
</div>

<script type="text/javascript">
var sort = $("#menu").attr("data-sort"); // HTML5 FTW!
$("#menu li." + sort).addClass("active");
</script>

Upvotes: 0

orip
orip

Reputation: 75427

Here's another suggestion with some less repetition.

<?php
function sortClass($val) {
  return ($sort == $val) ? 'class="active"' : '';
}
?>
<li><a id="Hot" href="index.php?sort=hot&page=1" <?=sortClass('hot');?> >Hot</a></li>
<li><a id="New" href="index.php?sort=new&page=1" <?=sortClass('new');?> >New</a></li>
<li><a id="Top" href="index.php?sort=top&page=1" <?=sortClass('top');?> >Top</a></li>

Upvotes: 0

Eelvex
Eelvex

Reputation: 9133

Maybe use a loop and array?

<div id="menu">
  <ul>
    <li><span id="menulabel">Your Languages</span></li>
      <?php 
    foreach(array("Hot","New","Top") as $Id) {
      $class = '';
      if ($sort == strtolower($Id)) $class = 'active';
      echo "<li><a id='$Id' href='index.php?sort=".strtolower($Id)."&page=1' class='$class'>$Id</a></li>";
    }
    ?>
    </li>
  </ul>
</div>

Upvotes: 0

jondavidjohn
jondavidjohn

Reputation: 62392

You might consider using a ternary here...

<div id="menu">
    <ul>
        <li><span id="menulabel">Your Languages</span></li>
        <li><a id="Hot" href="index.php?sort=hot&page=1" class="<?php echo ($sort == 'hot') ? 'active' : '' ?>">Hot</a></li>
        <li><a id="New" href="index.php?sort=new&page=1" class="<?php echo ($sort == 'new') ? 'active' : '' ?>">New</a></li>
        <li><a id="Top" href="index.php?sort=top&page=1" class="<?php echo ($sort == 'top') ? 'active' : '' ?>">Top</a></li>
    </ul>
</div>

definitely saves a lot of code

In case you're not familiar with the Ternary Operator

(condition) ? 'result if true' : 'result if false'

EDIT

As Matijs Points out

you could even put the entire class declaration inside the operator to avoid ending up with class=""

<li><a id="Hot" href="index.php?sort=hot&page=1" <?php echo ($sort == 'hot') ? 'class="hot"' : '' ?> >Hot</a></li>`

Upvotes: 2

Mlynch1985
Mlynch1985

Reputation: 101

Another possible way that I can think of would be to change your URL parameters. Try This:

<div id="menu">
    <ul>
        <li><span id="menulabel">Your Languages</span></li>
        <li><a id="Hot" href="index.php?hot=active&page=1" class="<?php echo $_GET['hot']; ?>">Hot</a></li>
        <li><a id="New" href="index.php?new=active&page=1" class="<?php echo $_GET['new']; ?>">New</a></li>
        <li><a id="Top" href="index.php?top=active&page=1" class="<?php echo $_GET['top']; ?>">Top</a></li>
    </ul>
</div>

Upvotes: -1

Chris
Chris

Reputation: 1579

<div id="menu">
<ul>
    <li><span id="menulabel">Your Languages</span></li>
    <li><a id="Hot" href="index.php?sort=hot&page=1" class="<?php echo ($sort=='hot')?'active':''; ?>">Hot</a></li>
    <li><a id="New" href="index.php?sort=new&page=1" class="<?php echo ($sort=='new')?'active':''; ?>">New</a></li>
    <li><a id="Top" href="index.php?sort=top&page=1" class="<?php echo ($sort=='top')?'active':''; ?>">Top</a></li>
</ul></div>

If "short_tags" are not enabled. If they are enabled, you can replace <?php echo " with "<?="

Upvotes: 0

Jake Kalstad
Jake Kalstad

Reputation: 2065

Make a switch with 'hot' 'new' and 'top as cases, or, a method that takes two strings, if the first string matches the '$sort' value, then set the second string to active

Upvotes: 0

Related Questions