Новичок в ООП PHP, нужна критика в первом классе Geo RSS

Question

Я совершенно новичок в ООП PHP и сейчас читаю "Объекты, шаблоны и практика PHP".Мне нужно было разработать что-то, что будет генерировать канал GeoRSS.Это то, что у меня есть (это работает отлично, я просто хотел бы получить некоторую критику относительно того, что я мог бы сделать иначе / более эффективно / безопаснее):

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

Answer 1

1. Свойства всегда должны быть protected, если нет веских причин для их создания public или private.
2. Объявите или инициируйте все переменные перед их использованием:отсутствует protected $items в теле класса и $items = '' в getFeed.
3. Инициируйте переменные правильно: $this->items = array(); в __construct.

4. Неуправлять своим item_count.Лучше полагаться на собственные средства добавления массивов в PHP:

   $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,
   );
   

   Намного приятнее, не так ли?

5. Не объявляйте больше переменных, чем нужно:

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

   Здесь вам не нужен ключ массива.Так что не извлекайте его в цикле foreach;) Вместо этого используйте $item для значения, которое лучше, чем $item_element.

Answer 2

Единственное, что у меня есть с этим классом, это ваша функция setItem, вы должны просто использовать нотацию [], чтобы выдвинуть ассоциативный массив, подобный этому:

 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);
 }

Таким образом, вы можете отказаться от своего$item_count индексная переменная.

Кроме того, если ваши свойства равны public, это обычно Очень плохая идея.

Answer 3

Вот несколько моментов:

1. Почему все участники публичны? Вы устанавливаете их в конструкторе, а затем используете их для создания канала. Поэтому, вероятно, не стоит позволять кому-либо менять их по своему желанию. Разве они не должны быть окончательными / неизменными для каждого экземпляра?
2. Ваши предметы, вероятно, должны быть отдельным классом. Всякий раз, когда вы видите, что вы создаете большой ассоциативный массив, как у вас в setItem, это указывает на то, что у вас есть другой объект в игре. Так что создайте class RSSItem или что-то в этом роде, используя эти элементы в качестве членов.

Если я еще подумаю, я отредактирую свой ответ.

Answer 4

Хорошо выглядит для первого таймера!Где вы обрабатываете данные, которые вы отправляете в качестве параметров?Лично я обработал бы все с помощью методов класса, цель объектов состоит в том, чтобы содержать объекты.Это означает, что вся связанная с ними обработка должна происходить внутри самого класса.

Кроме того, может быть, это хорошая идея играть с наследованием и публичными, закрытыми членами, классами, которые используют другие классы, как предложил Tesserex.Тем не менее, хорошее начало на ООП там.