Refactoring Suggestion - Remove Format from Writer Class

[ghost]

Just expanding on the previous patch. A better idea would be to remove the responsibility of the writer class having to valid the format is valid. So in the class below i have removed that responsibility to a new format class, which can then make use of type hinting to enforce the correct type is given to the constructor of the writer class.

Not tested this code and it's almost 1am for me, but it's just to show you a rough idea of how it would work and how it would make adding new formats a little easier in the future. Hope it helps or at least gives you more ideas.

/**
* use this class to enforce that a valid instance of feed_format is passed to the feed_writer
* this removes the responsibility of the feed_writer checking the feed format and encapsulates the formats
* might want to consider refactoring some other features out of the single writer class and use inheritance
* to give each format specific features (i.e. feed_writer = abstract class, feed_writer_rss1 + 
* rss_writer_atom + feed_writer_rss2 all extend feed_writer making it easy to add future feed formats) - 
* but one step at a time ;)
*/
class feed_format {

    /**
    * formats available
    */
    const ATOM = 'ATOM';
    const RSS1 = 'RSS 1';
    const RSS2 = 'RSS 2';

    /**
    * default format to use if given an invalid format on construct
    */
    const DEFAULT = 'RSS 2';

    /**
    * currently selected format
    */
    private $format;

    /**
    * what format to request
    */
    public function __construct($format) {

        //uppercase so it will still match the constants used
        $this->format = strtoupper($format);

        //check it's valid, else use default 
        switch ($this->format) {
            case self::ATOM:
            case self::RSS1:
            case self::RSS2: 
                break;
            default: 
                trigger_error("Incorrect feed format used. Defaulting to: " . self::DEFAULT, E_USER_NOTICE);
                $this->format = self::DEFAULT;
        }

    }

    /**
    * returns the currently selected format so the feed_writer class can ask questions about it's type via methods
    */
    public function get_format() {
        return $this->format;
    }

}

/**
* writer class construct signature
*/
class feed_writer {
    /**
    * now you can't give it an incorrect format as it will only accept an instance of feed_format
    */
    public function __construct(feed_format $format) {
        //...code here
    }
}

//example
$feed = new feed_writer(new feed_format(feed_format::RSS));

Just a thought.

Kind regards,
Scott

[mibe]

A IMHO simpler approach would be to use a derived class for each type of feed:

class RSS2FeedWriter extends FeedWriter
{
    function __construct()
    {
        parent::__construct(RSS2);
    }
}

Important would be to change the visibility of the constructor of the FeedWriter class to protected. This would enforce the use of the three sub-classes from outside.

I've just tested this, and it works flawlessly. The only change is to create an RSS2FeedWriter instead of an FeedWriter instance when using the code.

[ghost]

Hey up, yeah I'd go with that, but u might want to consider extracting the responsibilities out of the super class ;) though that would be quite a big job & I don't think it'd be worth anyone's time doing that refactoring yet. Maybe if more feed formats start to take over RSS and atom, maybe then ;)

[ghost]

Oh might also want to set the super class to abstract - since u can't instantiate the class without extending it :) though setting the class to abstract and the constructor to protected would break backward compatibility.... Meaning maybe hold that back until the next major release?

[mibe]

Yes, I have the same opinion: The base class should be independent from any feed type and offering just functions for the core XML stuff. Are there some new feed types on the horizon?

Since this is a relatively small project I never thought about "real" releases - HEAD should always work...

[ghost]

Might be worth while just creating a quick branch for version 2 that includes the proper refactoring - if you got the time that is. At the end of the day the code that's there now works perfectly fine so it's really not a big problem :)

Regarding the feed types... thinking more of custom feeds or internal syndication feeds such as a private feed format specialised for providing transcripts for podcasts where the internal readers can understand them but public ones wouldn't. Since you have called it the "Universal" feed writer ;)

P.S. Whens the Universal Feed Reader coming? Complete set then!