Neu in OOP PHP, brauche Kritik an der ersten Geo RSS Klasse

Question

Ich bin ein absoluter Neuling in Sachen OOP PHP und lese gerade "PHP Objects, Patterns and Practice". Ich musste etwas entwickeln, das einen GeoRSS-Feed erzeugt. Dies ist, was ich habe (es funktioniert perfekt, ich würde nur gerne einige Kritik, was ich anders / effizienter / sicherer tun könnte):

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. Eigenschaften müssen immer protected wenn es nicht einen zwingenden Grund gibt, sie zu machen public oder private .

2. Deklarieren oder initiieren Sie alle Variablen, bevor Sie sie verwenden: Sie vermissen eine protected $items im Klassenkörper und eine $items = '' en getFeed .

3. Setzen Sie die Variablen richtig ein: $this->items = array(); en __construct .

4. Verwalten Sie keine eigenen item_count . Verlassen Sie sich lieber auf die PHP-eigenen Möglichkeiten zum Anhängen von Arrays:

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

   Das ist doch viel schöner, oder?

5. Deklarieren Sie nicht mehr Variablen, als Sie benötigen:

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

   Hier brauchten Sie den Array-Schlüssel nicht. Holen Sie ihn also nicht in der foreach Schleife ;) Verwenden Sie stattdessen $item für den Wert, was besser ist als $item_element .

Answer 2

Hier sind einige Punkte:

1. Warum sind alle Mitglieder öffentlich? Man legt sie im Konstruktor fest und verwendet sie dann zum Aufbau des Feeds. Es ist also wahrscheinlich keine gute Idee, sie nach Belieben ändern zu lassen. Sollten sie nicht ohnehin für jede Instanz endgültig/unveränderlich sein?
2. Ihre Gegenstände sollten wahrscheinlich eine eigene Klasse sein. Wann immer Sie sehen, dass Sie ein großes assoziatives Array erstellen, wie Sie es in setItem zeigt an, dass Sie ein anderes Objekt im Spiel haben. Machen Sie also eine class RSSItem oder so ähnlich, mit diesen Dingen als Mitglieder.

Wenn mir noch mehr einfällt, werde ich meine Antwort ändern.

Answer 3

Das einzige Problem, das ich mit dieser Klasse habe, liegt in Ihrer setItem Funktion, sollten Sie einfach die [] Notation, um ein assoziatives Array wie dieses zu schieben:

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

Auf diese Weise können Sie Ihre $item_count Index-Variable.

Auch die Vermietung Ihrer Immobilien an public es in der Regel eine sehr schlechte Idee.

Answer 4

Sieht gut aus für einen Neuling! Wo verarbeiten Sie die Daten, die Sie als Parameter senden? Ich persönlich würde alles in den Methoden der Klasse verarbeiten, denn der Zweck von Objekten ist es ja, Objekte zu enthalten. Das bedeutet, dass die gesamte Verarbeitung im Zusammenhang mit ihnen sollte innerhalb der Klasse selbst geschehen.

Vielleicht ist es auch eine gute Idee, mit Vererbung und öffentlichen und privaten Mitgliedern zu spielen, mit Klassen, die andere Klassen verwenden, wie Tesserex vorgeschlagen hat. Trotzdem, ein guter Anfang für OOP.