The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Class Interface

I'm building an e-commerce store in PHP, and am trying to do it in good OO style for the first time. I've got a data access class to represent albums, which maps to an SQL table. The class looks something like this:

class Album
{
    public function getTitle() { return $this->title; }
    public function getArtist() { return $this->artist; }
    ...
    public static function findByIdentity($id) { ... }
    public static function findAllAlbums() { ... }
}

I don't have enough experience to know if I should be putting those find methods in this class, or if they belong elsewhere. findByIdentity() will return one album, but findAllAlbums() will return a list (not a record set) populated with album objects. To me, this seems like an inconsistent abstraction, and two solutions come to mind:

1. Return only lists of albums, even if the list only contains one item.

2. Factor out another class that can return only lists of Albums, or maybe lists of other objects too.

What else could I/should I do?
Anon
Sunday, November 14, 2004
 
 
You're interface is suprisingly read-only.  How do you add an album?  What I would do is this:

class Album
{
    public $Id = Null;
    public $Title;
    public $Artist;
    public function Load($id) { ... }
    public function Save();
    public function FillFromArray($array);
}

The Load() method is basically your findByIdentity() but it's a non-static method of the class and loads into that instance.  So you do this:

$album = new Album;
$album->Load($id);

Then you had edit that album by changing the fields and saving:

$album->Artist = "U2"
$album->Save();

You create a new artist easily:

$newAlbum = new Album;
$album->Artist = "Pearl Jam"
$album->Title = "Ten"
$album->Save();

The Save() method does double-duty so if the member $this->Id is null it creates a new record.  If the $id contains a value (because you loaded the record previously) then it updates that record.

All of my classes (in PHP4) inherit from a base class that provides the Load() and Save() methods.  Code in the each class constructor gives information on the table and fields so Load() and Save() know what to do.

As for findAllAlbums() that can be a static method of the Album class.  It could simply return an array of filled Albums.  What I do in my code is return a record-set like object that has the MoveNext(), Eof(), etc methods.  What the MoveNext() method does is retrieve the current record from the DB, populates an Album object, moves to the next record, and returns that Album object.

You can some really neat stuff in PHP5 by using __get/__set methods to control property access and the iteratable interface for making your classes foreach() compatible.  But I'm trying to keep things simple here.
Almost Anonymous Send private email
Sunday, November 14, 2004
 
 
I've written the back-end CRUD interface procedurally, but am writing the (much simpler) front-end interface like this to get used to writing OO code. I like your approach, but I'm still uncomfortable using the static finder method. What I'd like to know is if it would be more correct to factor out the finder methods into another class, something like an AlbumList, to keep the Album abstraction pristine. It seems like a bad idea to let an Album object know about lists of Albums. Here's what I had in mind (I'm leaving out create/update/delete code again):

class Album
{
    public function getTitle() { return $this->title; }
    public function getArtist() { return $this->artist; }
    public function load($id) { /* do the load */ }
}

class AlbumList
{
    public function loadAllAlbums() { /* returns a list of albums */ }
    public function loadAllClassicalAlbums() { /* returns a list of albums */ }
    public function loadAllRockAlbums() { /* returns a list of albums */ }
    public function next() { ... }
    public function prev() { ... }
    public function getCurrent() { ... }
    public function isLast() { ... }
}

And then in the view code:

...
$albumList = new AlbumList();
$albumList->loadAllAlbums();

while (!$albumList->isLast();) {
    $album = $albumList->getCurrent();
    echo $album->getTitle();
    $albumList->next();
}
...

How's this?
Anon
Sunday, November 14, 2004
 
 
Totally.  That's what I would suggest.  I was purposely trying to keep things simple. 

If your using PHP5 you can use the iterator interfaces so that your AlbumList class (which mostly just wraps a database resultset) can use the foreach construct:

$albumList = new AlbumList();
$albumList->loadAllAlbums();

foreach ($albumList as $album) {
  echo $album->getTitle();
}

In my code, I actually abstract most of this out so again there is a base class that handles most of this.  And there is just a specific implementation of that class for each Entity type.

Another suggestion is to create an alternative "Albums" class (not the same as Album) that has static methods for retrieving lists:

$albumList = Albums::AllAlbums();

And the result of this is an EntityList class which is a generic class for returning objects from database results.  Maybe something like this:

class Albums {
static function AllAlbums() {
  $resultset = Database_Run_Some_Query();
  return new EntityList(new Album(), $resultset);
}
Almost Anonymous Send private email
Sunday, November 14, 2004
 
 
Hey, nice effort on what I found to be an almost impossible task - making myself write good OO code for PHP!

Anyways, what you are asking for has a pretty standard solution in the OO world, called the DAO pattern, short for Data Access Objects. For each database entity, you should use a couple of classes, one that simply represents a single album, and one that handles the data access. Writing in pidgin, because it's my current favourite language:

class Album
// this class contains setters and getters and nothing else
{
    public function getTitle() { return $this->title; }
    public function getArtist() { return $this->artist; }
    public function setTitle() { ... }
    public function setArtist() { ... }
}

class AlbumDAO
// or AlbumManager or whatever you want to use
{
    public static function findByIdentity($id) { ... }
    public static function findAllAlbums() { ... }
    public static function findAlbums($filter) { ... }
    public static function insert($album) { ... }
    public static function update($album) { ... }
    public static function delete($id) { ... }
    public static function delete($album) { ... }
}

findByIdentity returns an Album, not a list
findAllAlbums returns a list


You'll find a lot more by googling for "DAO pattern"
Herr Herr Send private email
Monday, November 15, 2004
 
 
[troll]
 What you should really do is to use Perl with
 an  established OR-mapper like Class::DBI,
 and not waste your time on problems that have
 been solved before PHP even came into existance.
[/troll]


In PHP5 things like

public function getTitle() { return $this->title; }

are unnecesary. It has means of hooking up your code to be called on standard ($this->title) accessors. I guess it's special __get and __set instance methods or something.
Egor
Monday, November 15, 2004
 
 
Why do you have

[snip]
public function setTitle() { ... }
public function setArtist() { ... }
[/snip]

I theory when you have created the album object the title or author doesn't change.
Peter Monsson Send private email
Tuesday, November 16, 2004
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz