Klassendesign

[SilentSight]

Hallo Leute,

ich arbeite im Moment an einem neuen, objektorientierten Ansatz für ein kleines Probe-CMS, das ich mir zur Aufgabe gemacht hab.
Falls der Ansatz funktioniert will ich künftig alle meine Websites nach diesem Prinzip aufbauen.

Ich würde dazu gern eine Frage stellen:
Ich habe eine Klasse für eingehende Anfragen:

class Enquiry
{
    private $id;                // int
    private $mediumType;        // int / MediumType
    private $publicationDate;    // DateTime
    private $topic;                // String
    private $remarks;            // String
    private $attachment;        // String
    private $answeredPress;        // bool
    private $answeredFrom;        // String
    private $answerType;        // int / AnswerType
    private $depRedirect;        // String
    private $dirRedirect;        // String
    private $coopRating;        // String
    private $reportTerminated;    // bool
    private $dateCreated;        // DateTime
    private $creator;            // String --> Benutzertabelle???
    private $dateLastEdited;    // DateTime
    private $lastEditor;        // String --> Benutzertabelle???
    private $finished;            // bool
    private $deleted;            // bool
    
    public function __construct($id)
    {
        $dbh = DB::getHandle();
        $row = $dbh->query('SELECT * FROM `pm_new_enquiries` WHERE `id`='.$id)->fetch(PDO::FETCH_NUM);
        
        list(
            $this->id,
            $this->mediumType,
            $this->publicationDate,
            $this->topic,
            $this->remarks,
            $this->attachment,
            $this->answeredPress,
            $this->answeredFrom,
            $this->answerType,
            $this->depRedirect,
            $this->dirRedirect,
            $this->coopRating,
            $this->reportTerminated,
            $this->dateCreated,
            $this->creator,
            $this->dateLastEdited,
            $this->lastEditor,
            $this->finished,
            $this->deleted
        ) = $row;
    }
    
    public function getId() { return $this->id; }
    public function getMediumType() { return $this->id; }
    //...
    
    public function fetchList($start, $length)
    {
        $ret = array();
        $dbh = DB::getHandle();
        $sth = $dbh->query('SELECT * FROM `pm_new_enquiries` LIMIT '.$start.','.$length);
        
        while ($row = $sth->fetch(PDO::FETCH_NUM)) {
            list(
                $this->id,
                $this->mediumType,
                $this->publicationDate,
                $this->topic,
                $this->remarks,
                $this->attachment,
                $this->answeredPress,
                $this->answeredFrom,
                $this->answerType,
                $this->depRedirect,
                $this->dirRedirect,
                $this->coopRating,
                $this->reportTerminated,
                $this->dateCreated,
                $this->creator,
                $this->dateLastEdited,
                $this->lastEditor,
                $this->finished,
                $this->deleted
            ) = $row;
            
            $ret[] = $this;
        }
        
        return $ret;
    }
} 

Die Methode fetchList würde natürlich nur funktionieren, wenn ich zuerst eine leere Instanz von Enquiry erstellen würde und die Operation darauf ausführen würde.
Das scheint mir kein sehr guter Stil zu sein.
Mein zweiter Ansatz wäre gewesen, im Konstruktor alle Parameter, die in der Klasse vorhanden sind, zu übergeben. Dann könnte ich die fetchList-Methode als statisch deklarieren und es würde wieder um einiges besser aussehen.

Jetzt würde ich halt gern wissen, ob dieser zweite Ansatz sinnvoll ist (immerhin werden ja so alle Variablen im Prinzip doppelt angelegt, was aber mit der momentanen Methode fetchList auch nicht anders wäre) oder ob beide Ansätze totaler Mist sind und ich mir lieber was anderes suchen sollte...

Ich danke schonmal im Voraus für alle Antworten

mfg SilentSight

[Dark Guardian]

Die Methode fetchList würde natürlich nur funktionieren, wenn ich zuerst eine leere Instanz von Enquiry erstellen würde und die Operation darauf ausführen würde.
Das scheint mir kein sehr guter Stil zu sein.

Das scheint ein ActiveRecord-Pattern zu werden oder?

Demnach wäre, wenn überhaupt, das erzeugen einer Objektinstanz notwendig weil du sonst immer die gleiche Objektinstanz in deinem Array hast.

Allerding sist der Ansatz: 1 Objekt = 1 Datenzeile. Methoden für die Manipulation oder Abfrage mehrerer Objekte würde ich in eine Klasse EnquiryManager auslagern.

Mein zweiter Ansatz wäre gewesen, im Konstruktor alle Parameter, die in der Klasse vorhanden sind, zu übergeben. Dann könnte ich die fetchList-Methode als statisch deklarieren und es würde wieder um einiges besser aussehen.

Und dein Konstruktor hat dann eine Parameterliste von hier bis nach Meppen? Davon abgesehen das du innerhalb einer statischen Methode sowieso keinen Zugriff auf nicht-statische Membervariablen hast womit sich das wohl erledigen würde.

Eine Kombination aus beiden Ansätzen wäre aber denkbar. Also die fetchList() Methode wie oben benannt aber eben statisch, da keine Objektinstanz benötigt wird um sie auszuführen

[draco88]

Das Pattern, was du bei fetchList versuchst anzuwenden, nennt sich afaik Prototype. Das funktioniert aber nur, wenn du den "Prototyp" klonst und nicht nur mehrere Referenzen auf das gleiche Object in das Array speicherst.

Ansonsten hab ich es lieber, wenn Datenherkunft und Model getrennt sind.
Wenn du es aber lieber so lassen willst, dann solltest du die Klasse lieber auf lazy-load umstellen, d.h., dass die eigenschaften erst geladen werden, wenn sie abgefragt werden und im konstruktor erstmal nur die ID speicherst.

[Chriz]

Hallo, erstmal zur Quellcodebewertung:

- benutz protected statt private, so ermöglichst du eine Vererbung der Klasse. Das ist nicht immer notwendig, aber du schränkst dich auch nicht unnötig ein. private verwende ich fast nie, es sei denn ich weiß ganz genau, dass diese Eigenschaften/Methoden sehr speziell an die Klasse angepasst sind und von außen (also auch von erbenden Klassen) niemals geändert werden sollen, beispielsweise internes Caching o.ä.

- sobald du anfängst Listen von Eigenschaften abzutippen (list ($this->id, ..)), solltest du dir über einen generischeren Ansatz Gedanken machen. Entweder lässt du dir so einen Code durch einen Quellcodegenerator erzeugen, der bei Änderungen der Datenbank deinen PHP-Code entsprechend anpasst, oder du legst die Eigenschaften als Array in der Klasse ab.

- deine Klasse ist abhängig von der Datenbank und der Folge der Spalten. Wenn du zwischendurch eine Spalte in der Tabelle einfügst, wird dein PHP-Code nicht mehr funktionieren. Benutz statt FETCH_NUM entweder FETCH_ASSOC oder verzichte auf SELECT *-Abfragen.

- im Konstruktor per Singleton eine DB-Instanz abzuholen ist schlecht. Übergib das Datenbank-Objekt per Konstruktor-Parameter

- in fetchList erfolgt keine Validierung der Parameter, eine SQL-Injection ist möglich. Entweder du machst einen INT-Cast oder du benutzt mysql_real_escape_string().

- du überschreibst in der while-Schleife deine Ergebnisse


Grundsätzlich empfehle ich dir die Trennung von Datenobjekt und Datenquelle, wie auch schon vorgeschlagen. ActiveRecord ist für kleinere Projekte sicherlich praktisch und deshalb nicht pauschal zu verurteilen, allerdings wirst du bei vielen Querverbindungen zwischen den Objekten (User hat Profil, hat Nachrichten, hat Freunde, ..) schnell an deine Grenzen damit stossen. ActiveRecord eignet sich daher meiner Meinung nach nur für relativ unabhängige Objekte/Tabelleneinträge (z.B. Gästebücher o.ä.).

[Dormilich]

- in fetchList erfolgt keine Validierung der Parameter, eine SQL-Injection ist möglich. Entweder du machst einen INT-Cast oder du benutzt mysql_real_escape_string().

bzw. (da du bereits PDO benutzt), Prepared Statements.

[SilentSight]

@Dark Guardian:
Richtig erkannt, das sollte ein ActiveRecord-Pattern werden. Ich hab das konzept vor ein paar Tagen im Type3-Framework Flow3 gefunden und habe es eigentlich recht elegant gefunden.
Da ist ja auch das mit dem "Manager", nur eben als "Repository" beschrieben.
Das mit der ewig langen Parameterliste hat mich eben auch gestört. Aber mal anders gefragt: Wie soll ich es sonst lösen, dass ich neue Anfragen hinzufüge? Ich muss immerhin die Werte irgendwie in die Klasse einbringen, ob ich die Parameter jetzt an den Konstruktor oder eine Methode "add" übergebe...

@draco88:
Leider verstehe ich nicht ganz was du mit der Trennung von Datenherkunft und -model meinst.
Aber ich hab grad kapiert, dass die while-Schleife wirklich Schwachsinnig ist, ich speichere ja nicht das Objekt als Wert in's Array sondern bloß den Zeiger auf das Objekt, das hab ich wohl gestern übersehen...
Und was LazyLoad angeht: Das ist doch im Prinzip das gleiche wie Singleton, oder verstehe ich das auch falsch???

@Chriz

- im Konstruktor per Singleton eine DB-Instanz abzuholen ist schlecht. Übergib das Datenbank-Objekt per Konstruktor-Parameter

Wäre es wirklich so besser oder sollte ich die Klasse vielleicht sogar von der DB-Klasse ableiten?

@Dormilich:
Ja, sonst benutze ich auch Prepared Statements. Ich habs bloß gestern weggelassen (;

[Dark Guardian]

@Dark Guardian:
Richtig erkannt, das sollte ein ActiveRecord-Pattern werden. Ich hab das konzept vor ein paar Tagen im Type3-Framework Flow3 gefunden und habe es eigentlich recht elegant gefunden.
Da ist ja auch das mit dem "Manager", nur eben als "Repository" beschrieben.
Das mit der ewig langen Parameterliste hat mich eben auch gestört. Aber mal anders gefragt: Wie soll ich es sonst lösen, dass ich neue Anfragen hinzufüge? Ich muss immerhin die Werte irgendwie in die Klasse einbringen, ob ich die Parameter jetzt an den Konstruktor oder eine Methode "add" übergebe...

Wie Chriz schon geschrieben hat solltest du einen generischen Ansatz verfolgen.

Eine Möglichkeit wäre die Felder über magic getter/setter Methoden zu manipulieren und im Query mit den dort festgelegten Werten zu agieren.

Z.B. so:

class ActiveRecord {
   private $fields = arra;

   public function __get($f) {
      return $this->fields[$f];
   }

   public function __set($f, $v) {
      $this->fields[$f] = $v;
   }

   public function fillFields(Array $fields) {
      $this->fields = $fields;
   }

   public function insert() {
      $dbFields = array_keys($this->fields);
      $dbValues = arra_values($this->fields);

      $sql = "INSERT INTO table ('".implode("','", $dbFields)."') VALUES ('".implode("','", $dbValues)."')";
   }
}

$r = new ActiveRecord();
$r->id = '10';
$r->field1 = 'value1';
$r->field2 = 'value2';
$r->insert(); 

[SilentSight]

Von den magic gettern und settern hab ich auch schon gehört, aber mir wurde in der Berufsschule was von strikter Sicherheit in Klassen beigebracht. Beispielsweise soll die ID ja von außerhalb nicht geändert werden dürfen, was mit dieser Lösung aber möglich wäre... Außerdem habe ich gelesen, dass die magic setter und getter nicht wirklich das wahre im Hinblick auf die Performance sind, ob das wahr und inwiefern das wirklich relevant ist kann ich natürlich nicht sagen...
Aber allein aus ersterem Grund muss ich diese Möglichkeit fast schon ausschließen.

Mit "generischer Ansatz" meint ihr einen möglichst Allgemeinen? Das versuche ich ja hinzubekommen.

[draco88]

@draco88:
Leider verstehe ich nicht ganz was du mit der Trennung von Datenherkunft und -model meinst.
Aber ich hab grad kapiert, dass die while-Schleife wirklich Schwachsinnig ist, ich speichere ja nicht das Objekt als Wert in's Array sondern bloß den Zeiger auf das Objekt, das hab ich wohl gestern übersehen...
Und was LazyLoad angeht: Das ist doch im Prinzip das gleiche wie Singleton, oder verstehe ich das auch falsch???

Du teilst deine Klasse in 2 oder meist noch mehr verschiedene Klassen auf.

Zum einen das Model, was im Grunde nur ein verbessertes Array ist und einfach nur Key/Value Paare speichert.

class Model {
    protected $id;
    /* mehr attribute */
    
    public function setId($id) {
        $this->id = $id;
    }
    public function getId() {
        return $this->id;
    }
    /* mehr methoden */
} 

und eine zum model passende Klasse, die die Daten aus der Datenbank liest und in ein Model schreibt, bzw aus dem Model liest und in die DB schreibt.

[SilentSight]

Sozusagen also ein Model Database Controller (um das mal mit dem MVC-Pattern zu vergleichen)?
Also Das Model, was meine Anfrage ist. Eine Klasse, die von der Haupt-Datenbank-Klasse erbt und die nötigen Daten für mein Model ausliest und eine Controller-Klasse, die die Daten zwischen diesen beiden Klassen austauscht?

Achja, bevor noch jemand was zu meinem Unverständnis der Unterschiede von Lazy-Loading und Singleton schreiben will, ich hab den Unterschied gefunden (;
Bei Singleton kann man ja nur eine Instanz der Klasse haben, aber bei Lazy-Loading wird bloß das Laden der Daten verzögert, aber mehrere Objekte können trotzdem erzeugt werden.

[Dark Guardian]

Von den magic gettern und settern hab ich auch schon gehört, aber mir wurde in der Berufsschule was von strikter Sicherheit in Klassen beigebracht. Beispielsweise soll die ID ja von außerhalb nicht geändert werden dürfen, was mit dieser Lösung aber möglich wäre...

Ich persönlich sehe keinen Grund warum man die Id nicht von außen setzen können sollte.

Die Sicherheiten der Membervairablen von außen sollte auch in sich schlüssig sein.

Nur mal so als Beispiel: Wenn du einen Datensatz dublizieren willst machst du i.d.R. einen Select und danach einen Insert ohne den Primärschlüssel. Ohne ndas setzen/löschen der Id von außen bekommst du das aber nicht hin. Außer du speicherst die Id eben seperat und ignorierst bsie beim Insert. Was passiert aber bei Tabellen mit Primärschlüsseln die sich über mehr als eine Spalte erstrecken?

Nur um den Primärschlüssel nicht setzen zu können halte ich das für etwas viel Aufwand.

[SilentSight]

So, ich habe jetzt mal aus der einen Klasse ein paar mehr gemacht und versucht die Idee mit Daten- und Modelltrennung umzusetzen.
Dabei ist folgendes herausgekommen:

class EnquiryModel
{
    private $id;                // int
    private $mediumType;        // int / MediumType
    private $publicationDate;    // DateTime
    //...
    
    public function getId() { return $this->id; }
    public function getMediumType() { return $this->mediumType; }
    public function getPublicationDate() { return $this->publicationDate; }
    //...
    
    public function fetchData($data)
    {
        list(
            $this->id,
            $this->mediumType,
            $this->publicationDate,
            //...
        ) = $data;
    }
}

class EnquiryData extends Database
{
    public function getData($id)
    {
        return $this->getHandle()->query('SELECT * FROM `pm_new_enquiries` WHERE `id`='.intval($id))->fetch(PDO::FETCH_NUM);
    }
    
    public function getPage($pageLength, $page)
    {
        return $this->getHandle()->query('SELECT * FROM `pm_new_enquiries` LIMIT '.$start.','.$length.' ORDER BY `id` DESC');
    }
}

class EnquiryController
{
    private $data;
    public $model;
    
    
    public function __construct($em, $ed)
    {
        $this->model = $em;
        $this->data = $ed;
    }
    
    public function getEnquiryById($id)
    {
        $this->model->fetchData($this->data->getData($id));
    }
    
    public function getPage($pageLength, $page)
    {
        // Nicht gut...
        $em = new EnquiryManager();
        $em->getPage($this->data->getPage($pageLength, $page));
    }
}

class EnquiryManager
{
    public function getPage($sth)
    {
        while ($row = $sth->fetch(PDO::FETCH_NUM)) {
            $ret[] = new EnquiryModel();
            // Hm... Wie weiter?
        }
        
        return $ret;
    }
}

$model = new EnquiryModel();
$data = new EnquiryData();
$controller = new EnquiryController($model, $data);
$controller->getEnquiryById(1);
$controller->model->getPublicationDate(); 

Ich bin mir nicht ganz sicher, ob das dem entspricht was ihr gemeint habt, deshalb würde ich ganz gern eure Meinung/Verbesserungsvorschläge dazu hören.
Und - falls es der richtige Weg ist - an zwei Stellen weiß ich momentan nicht weiter (sind kommentiert).

Ich freue mich natürlich weiterhin über jede Kritik (:

P.S. Das eine Attribut im Controller hab ich nur public gemacht, um zu schauen ob das alles überhaupt funktioniert.
Unter Umständen werde ich doch zu den Magic Gettern bzw. Settern wechseln (eine Überlegung ist es zumindest wert)...

[draco88]

Ein Beispiel aus der Zend Doku:

// application/models/Guestbook.php
class Application_Model_Guestbook
{
    protected $_comment;
    protected $_created;
    protected $_email;
    protected $_id;
    public function __set($name, $value);
    public function __get($name);
    public function setComment($text);
    public function getComment();
    public function setEmail($email);
    public function getEmail();
    public function setCreated($ts);
    public function getCreated();
    public function setId($id);
    public function getId();
}
class Application_Model_GuestbookMapper
{
    public function save(Application_Model_Guestbook $guestbook);
    public function find($id);
    public function fetchAll();
}

Benutzt wird das ganze dann so:

$guestbook = new Application_Model_GuestbookMapper();
$entries= $guestbook->fetchAll();

[Dormilich]

class EnquiryManager
{
    public function getPage($sth)
    {
        while ($row = $sth->fetch(PDO::FETCH_NUM)) {
            $ret[] = new EnquiryModel();
            // Hm... Wie weiter?
        }
        
        return $ret;
    }
} 

hier kannst du die Iterierbarkeit von PDO ausnutzen …

class EnquiryManager
{
    # sicherstellen, daß du auch wirklich ein PDO resultset hast
    public function getPage(PDOStatement $sth)
    {
        $sth->setFetchMode(PDO::FETCH_NUM);
        foreach ($sth as $row) {
            $ret[] = new EnquiryModel();
            // Hm... Wie weiter?
        }
        return $ret;
    }
} 

falls du die Daten aus dem Query in EnquiryModel ablegen willst (da bin ich mir nich ganz sicher)

class EnquiryManager
{
    # sicherstellen, daß du auch wirklich ein PDO resultset hast
    public function getPage(PDOStatement $sth)
    {
        $sth->setFetchMode(PDO::FETCH_CLASS, "EnquiryModel");
        foreach ($sth as $row) {
            $ret[] = $row;
        }
        return $ret;
        # vielleich funktioniert auch
        return $sth->fetchAll();
    }
}