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\">\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;
 }
}

+1 Even if I have had some points of critique most of them are not that important. The only violation of OO principles is the use of `public` for the properties ;) But apart from that everything else is only about better coding style ;) — NikiC, Oct 22 '10 at 18:33
Yeah, some hipster-hood needed in your code. It sounds like a professor's formal personal introduction in a new university. So breathless in the tuxedo and tie! Just relax a bit, and let the parser do your work... esp. declaring and re-declaring everything... — Aditya M P, May 08 '11 at 10:51

NikiC · Accepted Answer · 2011-03-18T13:52:25.093

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

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?

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.

1. Gotcha - I'm still a little rusty with using public vs protected. 2. Good point! 3. Makes sense. 4. My mistake. 5. Didn't know you could do that - thanks! — Yev, Oct 22 '10 at 18:52

score 3 · Answer 2 · answered Oct 22 '10 at 18:18

3

Here are some points:

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?
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.

answered Oct 22 '10 at 18:18

Tesserex

17,166
5
66
106

1. Good point! I need to re-read that chapter in the book. :) 2. Using your suggestion, I would add an "addItem" method to RSS which accepts an RSSItem object? Is that correct? – Yev Oct 22 '10 at 18:46
could do that, or you could still take the same parameters and construct the RSSItem object inside the function. But the way you suggest is probably better so other code can interact with the items individually. – Tesserex Oct 22 '10 at 19:14

score 3 · Answer 3 · answered Oct 22 '10 at 18:21

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.

score 1 · Answer 4 · answered Oct 22 '10 at 18:21

1

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.

answered Oct 22 '10 at 18:21

Claudiu

3,261
1
15
27

The data is processed elsewhere from a db query. I might be mistaking, but as far as I know the purpose of objects is to make them reusable. So if I was to include all the data processing inside the RSS class - that would limit it to just the data that I am processing inside of it. Perhaps I should make a separate class to fetch and process the data? Hope that makes sense :) – Yev Oct 22 '10 at 19:01
Depends, as long as the table structure may vary (don't actually know what you're throwing in that RSS) sure, it's wiser to have two objects, not just one. I ussually tend to try and keep everything related to an object together, even if that means creating two or more classes, but keep them related, so I don't have code split all over the place. I hope you understand what I mean. – Claudiu Oct 22 '10 at 19:04

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

4 Answers4