« Return to Thread: RSS module refactoring

RSS module refactoring

by Bharat Mediratta :: Rate this Message:

Reply to Author | View in Thread


Hey Tim.  I looked over the rss changes and had some comments:

1) It'd be nice to migrate Rss_Controller away from using __call.
Perhaps use feed() instead.  This would change our urls from
/rss/comments to /rss/feed/comments which I think is fine.

2) Naming:
  rss::get_feeds() -> rss::available_feeds()
  rss::process_feed() -> rss::feed_data()

3) rss::item_feed() and rss::tag_feed() are out of place now because
they have domain knowledge.  You probably want to create
tag_theme::head() which generates the /rss/feed/tags url if the rss
module is active.

4) rss::get_feeds() shouldn't know about the sidebar imo.  I assume
you're doing this so that you can have hidden feeds such that the
slideshow module can have its own, right?  In that case let's just have
the slideshow module inject its own feed url using slideshow_theme.
That avoids leaking display info into the code here and resolves our
dependency issue.  So then you can get rid of all the "sidebar"
references everywhere

5) in the various feed functions, you have $feed as a stdClass, but then
you have a $data array inside it.  Why not just use stdClass?  Eg:

  $feed->title
  $feed->description
  $feed->children = array()  // since this has an ordering

etc.

that's all I've got for now.  This is definitely headed in the right
direction IMO.

-Bharat


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing
server and web deployment.
http://p.sf.net/sfu/businessobjects
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

 « Return to Thread: RSS module refactoring