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\$
4
  • \$\begingroup\$ How a programmer is supposed to choose which method to use, showLive or showCached? \$\endgroup\$ Commented Aug 29, 2018 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\$ Commented Aug 29, 2018 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\$ Commented Aug 29, 2018 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\$ Commented Aug 30, 2018 at 9:05

1 Answer 1

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\$
2
  • \$\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\$ Commented Aug 30, 2018 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\$ Commented Aug 30, 2018 at 9:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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