0
\$\begingroup\$

Currently I'm displaying a news feed from an XML file and using a counter to limit the number of news articles that are displayed. But I'm unsure if this is the best way to do it.

Here is my code.

<?php
function get_news($feed) {

$xml = new SimpleXmlElement(uwe_get_file($feed));
$counter = 0;
foreach ($xml->channel->item as $item) {
    echo '<a href="' . $item->link . '" target="_blank">' . $item->title . '</a><br />' . '<i>' . $item->description . '</i>' . '<strong>' . $item->pubDate . '</strong>' . '<br />' . '<br />';
    ++$counter;
    if ($counter == 5) {
        break;
    }
}
}

?>

Any suggestions on how I could improve this counter?

\$\endgroup\$
  • \$\begingroup\$ You could shorten your ++$counter; and if($counter == 5) line to if (++$counter == 5) \$\endgroup\$ – Blender Feb 19 '12 at 19:58
  • \$\begingroup\$ You could shorten it by one line like this if(++$counter === 5) { \$\endgroup\$ – Vague Feb 19 '12 at 19:59
  • \$\begingroup\$ Your loop is about as efficient as it will get, unless your XML file contains radically more than 5 elements, such as 2,000. Your reader is DOM-based and a SAX-style parser may save you from scanning the whole input. That is about the only optimization I can think of. \$\endgroup\$ – Duane Gran Feb 19 '12 at 20:05
  • \$\begingroup\$ @Graham The counter is working fine just thought that there may have been a better way of coding it. \$\endgroup\$ – Frank_B Feb 19 '12 at 20:22
  • \$\begingroup\$ I'd use printf($fmt) instead of echo this . that . the_other. But unless there's more than one way to skin any cat, and whatever method is going to be most readable in 6 or 12 months is probably the one you should pick. Supporting your code is always more work than writing it. \$\endgroup\$ – Graham Feb 20 '12 at 4:47
1
\$\begingroup\$

Assuming standard key values (0-based integers). You could change the loop to:

for($i=0;$i<5;$i++)
{
    $item = $xml->channel->item[$i];
    echo '<a href="' . $item->link . '" target="_blank">' . $item->title . '</a><br />' . '<i>' . $item->description . '</i>' . '<strong>' . $item->pubDate . '</strong>' . '<br />' . '<br />';
}

But I can't see it performing any better, just less code.

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

That's a perfectly good and valid way of doing it. Personally, I'd write $counter++ rather than ++$counter, but that's just my personal preference.

\$\endgroup\$
0
\$\begingroup\$

If you use a for loop you could get rid of the if($counter == 5) check:

function get_news($feed) {

   $xml = new SimpleXmlElement(uwe_get_file($feed));
   $count = count($xml->channel->item);
   for ($x = 0; ($x < $count && $x <= 5); $x++) {
     echo '<a href="' . $xml->channel->item[$x]->link . '" target="_blank">' . $xml->channel->item[$x]->title . '</a><br />' . '<i>' . l->channel->item[$x]->description . '</i>' . '<strong>' . l->channel->item[$x]->pubDate . '</strong>' . '<br />' . '<br />';
    }
 }
\$\endgroup\$

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.