2
\$\begingroup\$

Background: A client side I made recently has a component that is made from several (20+) rss-feeds, and the feeds are mashed into one using FreshRSS hosted on our servers. For some reason, FreshRSS sorts it in a weird order instead of the standard date latest to oldest.

So the output looks weird with the latest post that has timestamp for today, but the second post has timestamp for last week, and the third again for today.

So I'm capturing the output with output buffer, storing the output in an array and then storing the output array key in another array that will get sorted by date. What could I do better?

<?php

$feed = file_get_contents("secret-feed-url");

$rowArray = array();
$sortArray = array();

foreach($feed->channel->item as $item){

  ob_start();

  $media = ucfirst(str_replace([".fi", ".com", ".net"], "", parse_url($item->link, PHP_URL_HOST)));
  $date = date("d.m.Y", strtotime($item->pubDate));

  ?>

  <tr>
    <th>
      <a href="<?php echo $item->link; ?>" target="_blank" rel="noopener">
        <?php echo $item->title; ?>
        <span class="mobile-only">
          <?php echo $media . " &bull; " . $date; ?>
        </span>
      </a>
    </th>
    <td><?php echo $media; ?></td>
    <td><?php echo $date; ?></td>
  </tr>

  <?php

  $row = ob_get_clean();

  $rowArray[] = $row;

  end($rowArray);
  $rowKey = key($rowArray);

  $sortArray[] = [strtotime($item->pubDate), $rowKey];

}

usort($sortArray, function($a, $b){
  return $a[0] - $b[0];
});

$sortArray = array_reverse($sortArray);

$counter = 0;

foreach($sortArray as $row){

  if($counter > 10){
    break;    
  }

  echo $rowArray[$row[1]];
  $counter++;
}

I could store the "output" in a variable instead of using output buffer, but that would result in ugly code & template that is pain to update compared to this. I'm not too conserned about performance, as this is cached data.

Is there a better method for sorting this? I've done this several times on several sites, but it's usually a 1-time operation so using a framework like Symfony is a bit overkill.

\$\endgroup\$
2
\$\begingroup\$

You may not be to concerned about performance, but there actually is a lot of room for improvement here. Have a look at the refactored code first:

<?php

$feed = file_get_contents("secret-feed-url");

// just interested in the channel items, so drop the rest
$feed = $feed->channel;

// reverse sort by publication date
usort($feed, function ($a, $b) {
    return $b->pubDate - $a->pubDate;
});

// only need to render 10 items
$feed = array_slice($feed, 0, 10);

foreach ($feed->item as $item) :
    $media = ucfirst(str_replace([".fi", ".com", ".net"], "", parse_url($item->link, PHP_URL_HOST)));
    $date  = date("d.m.Y", strtotime($item->pubDate));

    ?>

    <tr>
        <th>
            <a href="<?php echo $item->link; ?>" target="_blank" rel="noopener">
                <?php echo $item->title; ?>
                <span class="mobile-only">
          <?php echo $media . " &bull; " . $date; ?>
        </span>
            </a>
        </th>
        <td><?php echo $media; ?></td>
        <td><?php echo $date; ?></td>
    </tr>

<?php endforeach;

The biggest issue with your code is that you are potentially rendering hundreds of items, while you use only 10 of them. So why not start of with sorting and limiting your data, and then start rendering it. And as a bonus you'll only need one loop and a lot less variables.

As you'll notice this also eliminates the need for the (ab)use of the output buffer.

And for the sorting and then reversing, I am all for readable code, but the way you do it is just a waste of resources. This is one of those cases you should clarify what you are doing with a comment, and go for the correct solution right away.

Note that the code is untested (I don't have the secret feed url ;) ), but from what I can deduce from your code I think the result should be the same. Do feel free to ask if you encounter any issues or if anything is unclear.

I hope it helps, and happy coding!

\$\endgroup\$
  • \$\begingroup\$ Thanks! I've had some troubles in the past when sorting the rss directly, but now it works flawlessly. Luckily I had only 20 items in the feed so it wasn't that bad. I started doing it like this when I had to resort existing html, not just feed data and it just got stuck. \$\endgroup\$ – Christian Apr 30 '16 at 18:17
2
\$\begingroup\$
usort($sortArray, function($a, $b){
  return $a[0] - $b[0];
});

$sortArray = array_reverse($sortArray);

Minor thoughts:

  • Instead of sorting by $a[0]-$b[0] and reversing, why not sort by $b[0]-$a[0] in the first place? However...

  • You could just return $b[0] <=> $a[0].

\$\endgroup\$
  • \$\begingroup\$ I think it's more clear when I explicitly reverse it, but maybe that's just me. Also I'm not using PHP 7 (and probably never will) so the spaceship operator is not an option. \$\endgroup\$ – Christian Apr 28 '16 at 22:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.