3

私はOOPPHPに完全に慣れておらず、現在「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;
 }
}
4

4 に答える 4

4
  1. protectedプロパティを作成するやむを得ない理由がない場合、プロパティは常に存在するものとしpublicますprivate。
  2. protected $items使用する前に、すべての変数を宣言または開始します。クラス本体にaがなく$items = ''、 。がありませんgetFeed。
  3. 変数を正しく開始します：$this->items = array();in __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。

于 2010-10-22T18:21:19.463 に答える
3

このクラスで私が持っている唯一の問題は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は、通常、非常に悪い考えです。

于 2010-10-22T18:21:42.960 に答える
3

ここにいくつかのポイントがあります：

  1. メンバー全員が公開されているのはなぜですか？それらをコンストラクターで設定し、それらを使用してフィードを作成します。したがって、誰でも自由に変更できるようにすることは、おそらく良い考えではありません。とにかく、インスタンスごとに最終的/不変であるべきではありませんか?
  2. あなたのアイテムはおそらく別のクラスでなければなりません。のような大きな連想配列を作成している場合はsetItem、別のオブジェクトが使用されていることを示しています。メンバーとしてそれらを使用して、class RSSItemまたはそのようなものを作成します。

もっと考えたら、答えを編集します。

于 2010-10-22T18:18:12.120 に答える
1

初めてのタイマーに似合います！パラメータとして送信するデータはどこで処理しますか？個人的には、クラスメソッドを使用してすべてを処理します。オブジェクトの目的は、オブジェクトを含めることです。つまり、それらに関連するすべての処理は、クラス自体の内部で行われる必要があります。

また、Tesserexが提案したように、継承やパブリック、プライベートメンバー、他のクラスを使用するクラスで遊ぶのは良い考えかもしれません。それでも、そこでOOPを開始するのは良いことです。

于 2010-10-22T18:21:29.587 に答える