2
\$\begingroup\$

I write this class to process and output xml/rss feeds. I'm still working on, but any suggestion will be appreciated. As a little trick if the xml file come from an online resource, I've added a method to cache the xml file as a json file.

<?php
class NewsFeed{

    public $url;
    private $cachedFeed;
    private $cachedFile;
    private $feed;
    private $xml;
    private $json;

    public function __construct($url){
        $this->url = $url;
    }

    public function showLiveFeed(){
        $feed = $this->processFeed();
        return json_decode($feed, true);
    }

    public function showCachedFeed(){
        $cachedFeed = $this->loadCachedFile();
        return json_decode($cachedFeed, true);     
    }

    private function processFeed(){
        $feed = $this->url;
        $xml = simplexml_load_file($feed, 'SimpleXMLElement', LIBXML_NOCDATA);
        $json = json_encode($xml);
        $cacheFeed = file_put_contents('cachedFeed.json', $json);
        return $json;
    }

    private function loadCachedFile(){
        if(file_exists('cachedFeed.json')){
          $cachedFile = file_get_contents('cachedFeed.json');
          return $cachedFile;
        }

    }
}
?>
\$\endgroup\$
  • \$\begingroup\$ How a programmer is supposed to choose which method to use, showLive or showCached? \$\endgroup\$ – Your Common Sense Aug 29 '18 at 7:06
  • \$\begingroup\$ It's a choice of the programmer, I don't thought why choose a method instead of another. The best scenario is when the feed is not available live due to technical problems or upgrade. But honestly I'm considering to remove the method. \$\endgroup\$ – user9741470 Aug 29 '18 at 13:03
  • 1
    \$\begingroup\$ Why such a scenario (use cached if a live version is not available) is not implemented in the class? Why not to use a cache just to make a faster response and lessen the load on the feed source? And which method you want to remove? \$\endgroup\$ – Your Common Sense Aug 29 '18 at 13:10
  • \$\begingroup\$ Since I want to make a simple XML/RSS reader class, some details like the automated choice of output the cached contents are not implemented. I want to remove the caching, but I think it can be useful in some case. Maybe to improve the class I need to think an automated choice of what date to render, so if the live feed is not available, the class will render the cached json version. \$\endgroup\$ – user9741470 Aug 30 '18 at 9:05
1
\$\begingroup\$

A caching strategy is not well thought. There is no clear scenario, whether a cached or a non-cached version should be used. Besides, a cache must have a timeout after which it is considered staled and must be renewed.

There is also a filename issue. With only a filename, which makes a relative path, you will end up with having a cachedFeed.json file in the every directory. The filename must be unique and certain, which means it must include an absolute path to the file.

Another possible issue is a case when 2 feeds are used on the same site. What would happen to a cache file in this case? A filename must be unique. the simplest solution would be just md5() from the feed url.

Other minor issues include excessive naming. your class already have the word Feed in its name, no need to duplicate it in the method names. Also, there are a lot of class variables that a never actually used.

So the final code could be like

<?php
class NewsFeed
{
    protected $url;
    protected $cacheFilename;
    protected $cacheTimeout = 3600;

    public function __construct($url)
    {
        $this->url = $url;
        $this->cacheFilename = $this->buildCacheFilename();
    }

    public function get()
    {
        $json = $this->readCached();
        if(!$json) {
            $json =  $this->download();
        }
        return json_decode($json, true);
    }

    protected function download()
    {
        $xml = simplexml_load_file($this->url, 'SimpleXMLElement', LIBXML_NOCDATA);
        $json = json_encode($xml);
        file_put_contents($this->cacheFilename, $json);
        return $json;
    }

    protected function readCached()
    {
        if (!file_exists($this->cacheFilename)) {
            return false;
        }
        if (filectime($this->cacheFilename) < (time() - $this->cacheTimeout)) {
            return false;
        }
        return file_get_contents($this->filename);
    }

    protected function buildCacheFilename()
    {
        return $_SERVER['DOCUMENT_ROOT'] . "../cache/feed" . md5($this->url);
    }
}

So we have just a single method get() to read the feed contents, which is first checking for the cached version and then reads the feed online if the cache is not available. A timeout is implemented to make sure that the cached data doesn't get too old and outdated.

\$\endgroup\$
  • \$\begingroup\$ Nice caching implementation, I will consider this when i will start reworking the code. I just have a question, how do you check if the feed is live or not? \$\endgroup\$ – user9741470 Aug 30 '18 at 9:09
  • 1
    \$\begingroup\$ well I would use CURL instead of file_get_contents, it won't raise a warning if the feed is not available. \$\endgroup\$ – Your Common Sense Aug 30 '18 at 9:19

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.