New to OOP PHP, need critique on first Geo RSS Class

Question

I'm completely new to OOP PHP and currently reading "PHP Objects, Patterns and Practice". I needed to develop something that will generate a GeoRSS feed. This is what I have (it works perfectly, I would just like some critique as to what I could do different / more efficiently / safer):

class RSS {
 public $channel_title;
 public $channel_description;
 public $channel_link;
 public $channel_copyright;
 public $channel_lang;
 public $item_count;
 public function __construct ($channel_title, $channel_description, $channel_link, $channel_copyright, $channel_lang) {
  $this->channel_title = $channel_title;
  $this->channel_description = $channel_description;
  $this->channel_link = $channel_link;
  $this->channel_copyright = $channel_copyright;
  $this->channel_lang = $channel_lang;
  $this->items = "";
  $this->item_count = 0;
    }
 public function setItem ($item_pubDate, $item_title, $item_link, $item_description, $item_geolat, $item_geolong) {
  $this->items[$this->item_count]['pubDate'] = date("D, j M Y H:i:s T",$item_pubDate);
  $this->items[$this->item_count]['title'] = $item_title;
  $this->items[$this->item_count]['link'] = $item_link;
  $this->items[$this->item_count]['description'] = $item_description;
  $this->items[$this->item_count]['geo:lat'] = $item_geolat;
  $this->items[$this->item_count]['geo:long'] = $item_geolong;
  $this->items[$this->item_count]['georss:point'] = $item_geolat." ".$item_geolong;
  $this->item_count++;
 }
 public function getFeed () {
  foreach ($this->items as $item => $item_element) {
   $items .= "    <item>\n";
   foreach ($item_element as $element => $value) {
    $items .= "      <$element>$value</$element>\n";
   }
   $items .= "    </item>\n";
  }
  $feed = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
    . "<rss version=\"2.0\" xmlns:geo=\"http://www.w3.org/2003/01/geo/wgs84_pos#\" xmlns:georss=\"http://www.georss.org/georss\"&gt;\n"
    . "  <channel>\n"
    . "    <title>".$this->channel_title."</title>\n"
    . "    <description>".$this->channel_description."</description>\n"
    . "    <link>".$this->channel_link."</link>\n"
    . "    <copyright>Copyright ".date("Y")." ".$this->channel_copyright.". All rights reserved.</copyright>\n"
    . "    <lang>".$this->channel_lang."</lang>\n"
    . $items
    . "  </channel>\n"
    . "</rss>";
  return $feed;
 }
}

Answer 1

Here are some points:

1. Why are all the members public? You set them in the constructor, and then use them to build the feed. So it's probably not a good idea to let anyone change them at will. Shouldn't they be final / unchanging for each instance anyway?
2. Your items should probably be a separate class. Whenever you see that you're creating a big associative array like you have there in setItem, it indicates you have another object in play. So make a class RSSItem or something like that, with those things as members.

If I think of more I'll edit my answer.

Answer 2

1. Properties shall always be protected if there isn't a compelling reason to make them public or private.
2. Declare or initiate all variables before you use them: You are missing a protected $items in the class body and a $items = '' in getFeed.
3. Initiate the variables right: $this->items = array(); in __construct.

4. Don't manage an own item_count. Better rely on PHP's own array appending facilities:

   $this->items[] = array(
       'pubDate'      => date("D, j M Y H:i:s T",$item_pubDate),
       'title'        => $item_title;
       'link'         => $item_link;
       'description'  => $item_description;
       'geo:lat'      => $item_geolat;
       'geo:long'     => $item_geolong;
       'georss:point' => $item_geolat." ".$item_geolong,
   );
   

   Much nicer, isn't it?

5. Don't declare more variables then you need:

   foreach ($this->items as $item) {
       $items .= "    <item>\n";
       foreach ($item as $element => $value) {
            $items .= "      <$element>$value</$element>\n";
       }
       $items .= "    </item>\n";
   }
   

   Here you didn't need the array key. So don't fetch it in the foreach loop ;) Instead use $item for the value, which is better then $item_element.

Answer 3

Looks good for a first timer! Where do you process the data that you send as parameters? Personally, I would process everything withing the class methods, the purpose of objects is to contain, well, objects. That means that all the processing related to them should happen inside the class itself.

Also, maybe it's a good ideea to play with inheritance and public, private members, classes that use other classes, like Tesserex suggested. Still, nice start on OOP there.

Answer 4

The only qualm I have with this class is in your setItem function, you should just use [] notation to push an associative array like this:

 public function setItem ($item_pubDate, $item_title, $item_link, $item_description, $item_geolat, $item_geolong) {
  $this->items[] = array(
                         'pubDate' => date("D, j M Y H:i:s T",$item_pubDate),
                         'title' => $item_title,
                         'link' => $item_link,
                         'description' => $item_description,
                         'geo:lat' => $item_geolat,
                         'geo:long' => $item_geolong,
                         'georss:point' => $item_geolat.' '.$item_geolong);
 }

This way you can ditch your $item_count index variable.

Also, letting your properties be public is usually a Very Bad Idea.