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.

• Lando: Thanks for the link. We have our own standard that is slightly more compact and still quite readable. You are right about the $hidden variable. The part I was unsure of was using foreach with a if x break sentence.

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.

• Lando: We like to keep the HTML output as separated from the PHP logic as we can. This way we generate valid HTML and can see most of the errors or unclosed tags inside the editor, it also generates a tidier output that is easier to audit. We also use comments but I removed them for the sake of brevity ;-). As I said before the part I was unsure of was using foreach with a if x break sentence