Show first n elements of array, and count elements not shown

Question

This code mimics the "likes" tooltip from Facebook.

Given an array with the list of users that liked the content, it shows the first n users and displays a final item with the count of the users not shown.

It works as expected, but I'm sure it could be better written.

<?  if ($count = $this->likes->count()) { 
        $visible = 12;
        $hidden = $count > $visible ? $count - $visible : 0;
?>
        <div class="likes_detail">
<?      $r = 0;
        foreach ($this->likes as $like) {
?>
            <span><?= $like->name ?></span>
<?          $r++;
            if ($r >= $visible) { break; };
        }
        if ($count > $visible) { 
?>
            <span class="more_users">and <?= $hidden ?> more&hellip;</span>
<?      } ?>
        </div>
<?  } ?>

Edit: Removed revised code after reading this meta post.

Answer 1

<?  if ($count = $this->likes->count()) { 

It is more common to use the long form of the opening tag. The second tag is only optionally supported by PHP installations. Also, it can suddenly stop working if your host adjusts the configuration (possibly accidentally during an upgrade).

<?php
    if ($count = $this->likes->count()) {

It's also more common to put the code on a new line after the opening tag. Unless you're opening and closing the PHP block on the same line.

If you're interested, there is a PHP Coding Standard. Personally I don't always follow it, but I find it a good way to make decisions on issues that aren't important to me. It also makes your code more portable.

            if ($r >= $visible) { break; };

I'd break this up into multiple lines for readability.

            if ($r >= $visible) {
                break;
            }

The final semi-colon is unnecessary there. It doesn't harm anything (perhaps a wasted clock cycle on the null instruction), but neither does it contribute anything.

        if ($count > $visible) { 

I might write this as

        if ( $hidden > 0 ) { 

The reason being that you then go on to output $hidden. You also might consider moving the definition of $hidden to be directly before this if statement. That puts all the code related to $hidden together and makes it easier to follow the logic.

Alternately, you could stay with the original if and replace $hidden with $count - $visibility. Then you don't need to define $hidden at all.

Answer 2

Without going into the content of your code, I don't like the intermingling of HTML and PHP. Perhaps it's me? I would write your code something like this:

if ($count = $this->likes->count()) { 
    $visible = 12;
    $hidden  = $count > $visible ? $count - $visible : 0;
    echo '<div class="likes_detail">'.PHP_EOL;
    $r = 0;
    foreach ($this->likes as $like) {
        echo "<span>{$like->name}</span> ".PHP_EOL;
        $r++;
        if ($r >= $visible) break; 
    }
    if ($count > $visible) { 
        echo '<span class="more_users">and '.$hidden.' more&hellip;</span>'.PHP_EOL;
    } 
    echo '</div>'.PHP_EOL;
}

Now I probably would not use echo's all over the place, and I would explain my code in comments. Why $visible = 12; for instance? It might be obvious to you, but I have not idea. Ah, there will be a maximum of 12 visible names! So, let's restructure this:

// set the maximum of names to display
$maxNames = 12; 
// if there are any likes we display them
if ($total = $this->likes->count()) { 
    // get the maximum number names of likes, or less
    foreach ($this->likes as $like) {
        $names[] = $like->name;
        if (count($names) == $maxNames) break; 
    }
    // generate html output
    echo '<div class="likes_detail"><span>'.implode('</span> <span>',$names).'</span>';
    if ($total > $maxNames) {
        echo '<span class="more_users">and '.($total-$maxNames).' more&hellip;</span>';
    }
    echo '</div>';
}

I've tried to seperate the processing of information from the HTML output, but since this is such a short piece of code, there's only so much I can do. Is this code part of a method in a class? Or is it just hanging out there somewhere in a HTML generating page? I use a HTML support class to create HTML. This prevents many common errors in HTML. The output section would look something like this:

    // push likes names into html
    $likesTag = $bodyTag->division('likes_detail');
    foreach ($names as $name) $likesTag->span('',$name.' ');
    if ($total > $maxNames) {
        $likesTag->span('more_users','and '.($total-$maxNames).' more…');
    }

This also means I don't have to worry about what is PHP and what is HTML, I can work in pure PHP. It makes for cleaner code, and fewer HTML errors.