Page 1 of 2

Application design (feeback needed)

Posted: Tue Jun 15, 2004 7:28 am
by Overunner
I'm currently developing an E-commerce solution for a (tiny) company. I'm trying to build the application so it will be easy to maintain, upgrade and so on, and have come up with the following design regarding products and product managament:
I have a (abstract) base class called Product. This class serves as a 'least common denominator'-class for all the other product types. It holds certain variables which all the other products has, e.g ID, name, price, image and so on. I then create a specific product by extending the base class (Product) and then add variables (and functions, which overloads the functions in the baseclass) which is also peculiar for the extended product. To clarify things I'll give you an example: If I wanted a 'Book-product', I extend the Product class and add variables...like author for instance.
To insert, delete and update certain product, I have another baseclass called ProductManager. Again, this class servers as a 'least common denominator'-class. To manipulate 'Book-products', I simply extend the base class and have the functions custom made. Here is an example of how I use the classes:

Code: Select all

require_once("Book.php");
require_once("BookManager.php");

$book = new Book();
$bookManager = new BookManager();

$book->setName("1984");
$book->setAuthor("George Orwell");
$book->setPrice("12.99");
// and so on

$bookManager->insert($book);

$book->setPrice("8.99");

$bookManager->update($book);
That is pretty much it. Now I wonder what you guys think about the design. Do you have better suggestions, should I modify mine, should I follow a pattern instead?

Thanks

Posted: Tue Jun 15, 2004 8:19 am
by PAW Projects
Looks like a solid design to me.

But since you're asking for suggestions, here's one:
If the class variables correspond with the database fields, you might as well add a 'load' function to the base class:

(very, very simplified pseudo-code)

Code: Select all

<?
class Product() {
var $id;
function Product($id=0) {
  if (is_numeric($id)) {
    $this->id = $id;
    $database->get_object($this);
  }

}

?>

Posted: Tue Jun 15, 2004 8:33 am
by Overunner
Thanks, will implement that right away :D

Posted: Tue Jun 15, 2004 8:36 am
by PAW Projects
I love this stuff.
My baseclass has these functions by default:
- get() (fills the class from POST variables)
- load($id) (fills the class from database, using key $id)
- save() (saves the class to database, or updates the row if it exists already)

Posted: Tue Jun 15, 2004 8:37 am
by patrikG
make sure you pass by reference, .e.g.

Code: Select all

$book =& new Book();
$bookManager =& new BookManager();
Since you're going for fully fledged OO design, make sure you use iterators etc. The Eclipse Library (you're probably very familiar with it) has a very powerful implementation of iterators.

I find it a bit hard to comment on your design, since you've only posted a very brief code snippet.

Posted: Tue Jun 15, 2004 9:06 am
by Overunner
Forgot about the references, I will use them of course :P
Here is my product class btw:

Code: Select all

class Product
  {
    // DATA MEMBERS

    /**
     * The ID-number.
     */
    var $id;

    /**
     * Name.
     */
    var $name;

    /**
     * The date when product will become available.
     */
    var $date;

    /**
     * Some quick information.
     */
    var $info;

    /**
     * Stock number.
     */
    var $stock;

    /**
     * Price.
     */
    var $price;

    /**
     * The searchpath to the image.
     */
    var $image;

    /**
     * The category the product belongs to.
     */
    var $category;

    /**
     * Product type.
     */
    var $type;

    // CREATOR

    function Product()
    { 
      die("Constructor of class <b>Product</b> is not implemented.");
    }

    // MANIPULATORS

    function setId($id)
    {
      $this->id = $id;
    }

    function setName($name)
    {
      $this->name = $name;
    }

    function setDate($date = date("Y-m-d"))
    {
      $this->date = $date;
    }

    function setInfo($info)
    {
      $this->info = $info;
    }

    function setStock($stock = 1)
    {
      $this->stock = $stock;
    }

    function setImage($image)
    {
      $this->image = $image;
    }

    function setCategory($category)
    {
      $this->category = $category;
    }

    // ACCESSORS

    function getId()
    {
      return $this->id;
    }

    function getName()
    {
      return $this->name;
    }

    function getDate()
    {
      return $this->date;
    }
    function getInfo()
    {
      return $this->info;
    }

    function getStock()
    {
      return $this->stock;
    }

    function getPrice()
    {
      return $this->price;
    }

    function getImage()
    {
      return $this->image;
    }

    function getCategory()
    {
      return $this->category;
    }

    function getType()
    {
      return $this->type;
    }
  }
And my ProductManager class:

Code: Select all

class ProductManager
  {
    // DATA MEMBERS

    /**
     * Handles the communication with the database.
     */
    var $dbh;

    // CREATOR

    function ProductManager(&$dbh)
    {
      $this->dbh =& $dbh;
    }

    // MANIPULATORS

    function insert(&$product)
    {
      die("Method <i>insert</i> of class <b>ProductManager</b> is abstract.");
    }
    function update(&$product)
    {
      die("Method <i>update</i> of class <b>ProductManager</b> is abstract.");
    }

    function delete($id)
    {
      die("Method <i>update</i> of class <b>ProductManager</b> is abstract.");
    }
  }
The Literature class:

Code: Select all

class Literature extends Product
  {
    // DATA MEMBERS

    /**
     * Description.
     */
    var $description;

    // CREATOR

    function Literature()
    {
      $this->category = 0;
    }

    // MANIPULATORS

    function setDescription($description)
    {
      $this->description = $description;
    }

    // ACCESSORS

    function getDescription()
    {
      return $this->description;
    }
  }
And the LiteratureManager:

Code: Select all

class LiteratureManager extends ProductManager
  {
    // CREATOR

    function LiteratureManager(&$dbh)
    {
      $this->ProductManager(&$dbh);
    }

    // MANIPULATORS

    function insert(&$literature)
    {
      $query = "INSERT INTO products VALUES (null, '" . $literature->getName() . "', '"
                                                      . $literature->getDate() . "', "
                                                      . $literature->getStock() . ", "
                                                      . $literature->getPrice() . ", '"
                                                      . $literature->getInfo() . "', '"
                                                      . $literature->getDescription() . "', '"
                                                      . $literature->getImage() . "', "
                                                      . $literature->getCategory() . ", "
                                                      . $literature->getType() . ")";

      $this->dbh->query($query);
    }

    function update(&$literature)
    {
      $query = "UPDATE products SET " .
        "name           = '" . $literature->getName()  . "', " .
        "date_available = '" . $literature->getDate()  . "', " .
        "info           = '" . $literature->getInfo()  . "', " .
        "stock          = '" . $literature->getStock() . "', " .
        "price          = '" . $literature->getPrice() . "', " .
        "description    = '" . $literature->getDescription() . "', " .
        "image          = '" . $literature->getImage() . "' WHERE id = " . $literature->getId();

      $this->dbh->query($query);
    }

    function delete($id)
    {
      $query = "DELETE FROM products WHERE id = " . $id;
      $this->dbh->query($query);
    }
  }
Any suggestions, tips, improvements about the design is greatly appreciated.

Posted: Tue Jun 15, 2004 11:27 am
by penguinboy
Looks like you're over commenting a bit.

Code: Select all

/** 
     * The ID-number. 
     */ 
    var $id; 

    /** 
     * Name. 
     */ 
    var $name;

Posted: Tue Jun 15, 2004 11:35 am
by Overunner
Actually, my comments looks usually like this:

Code: Select all

/**
     * The ID-number.
     *
     * @type int
     * @access protected
     */
    var $id
And

Code: Select all

/**
     * Sets some quick information.
     *
     * @param string $info Some quick information
     * @access public
     * @return void
     */
    function setInfo($info)
    {
      $this->info = sanitize($info);
    }
But I kinda reduced my comments, otherwise the post would be huge. Any comments on the app design btw :?:

Posted: Tue Jun 15, 2004 8:23 pm
by dull1554
in my opinion yes the comments are a bit much more efficient to do like this

Code: Select all

var $blah = "timmy";//this is a var in a class with a simple comment
easiest and most efficient

heres another thing
instead of

Code: Select all

function setDate($date = date("Y-m-d"))
    {
      $this->date = $date;
    }
id do this

Code: Select all

function setDate($format = "Y-m-d")
    {
      $this->date = date($format);
    }
that way you dont have to call the function like setDate(date("blah"));.........no sense of calling a function inside a function call
there then that
SOLID

Posted: Tue Jun 15, 2004 8:30 pm
by Weirdan
dull1554 wrote:id do this

Code: Select all

function setDate($format = "Y-m-d")
    {
      $this->date = date($format);
    }
that way you dont have to call the function like setDate(date("blah"));.........no sense of calling a function inside a function call
there then that
SOLID
but you can't set dates other then current one this way

Posted: Tue Jun 15, 2004 8:32 pm
by dull1554
though in the context in which this function is beigh used i dont think any date other then the current one need be set...maybe i misunderstood it but good point nevertheless

Posted: Wed Jun 16, 2004 2:59 am
by patrikG
dull1554 wrote:

Code: Select all

function update(&$literature)
    {
      $query = "UPDATE products SET " .
        "name           = '" . $literature->getName()  . "', " .
        "date_available = '" . $literature->getDate()  . "', " .
        "info           = '" . $literature->getInfo()  . "', " .
        "stock          = '" . $literature->getStock() . "', " .
        "price          = '" . $literature->getPrice() . "', " .
        "description    = '" . $literature->getDescription() . "', " .
        "image          = '" . $literature->getImage() . "' WHERE id = " . $literature->getId();

      $this->dbh->query($query);
    }
You can actually refactor that into smaller units - e.g. a query-builder. As is, a db-query doesn't belong in your Literature-Class. You can move it to your database class, where you pass a query-builder method two arrays, one being the "order"-array, i.e. the array containing the db-table column-names in a specific order and the data-array containing the data.

That would make your literature class deal with literature, not with db-queries.

Posted: Wed Jun 16, 2004 8:46 am
by McGruff
Product isn't really an object - it's a hash (in the associative array sense of the word :) ). If an object is an interface which hides the implementation of something or other there is nothing to hide here. There are no methods which actually do anything apart from get/set.

In saying that I do sometimes find it convenient to use objects as hashes. You can save some typing by making a single getter and setter (actually there's no real need for getters or setters at all):

Code: Select all

function setVar($name, $value)
{
    $this->$name = $value;
}

function getVar($name)
{
    return $this->$name;
}

Posted: Wed Jun 16, 2004 11:07 am
by Overunner
Great Idea! :D
This certainly makes things easier

Posted: Sat Jun 19, 2004 12:23 pm
by lazy_yogi
PAW Projects wrote:Looks like a solid design to me.

But since you're asking for suggestions, here's one:
If the class variables correspond with the database fields, you might as well add a 'load' function to the base class:
Database funcyions such as Load (ie Select) along with Insert, Update, and Delete would be handled by the Manager

Code: Select all

$book = $bookManager->get_by_id($id);
patrikG wrote:
dull1554 wrote:

Code: Select all

function update(&$literature)
    {
      $query = "UPDATE products SET " .
        "name           = '" . $literature->getName()  . "', " .
        "date_available = '" . $literature->getDate()  . "', " .
        "info           = '" . $literature->getInfo()  . "', " .
        "stock          = '" . $literature->getStock() . "', " .
        "price          = '" . $literature->getPrice() . "', " .
        "description    = '" . $literature->getDescription() . "', " .
        "image          = '" . $literature->getImage() . "' WHERE id = " . $literature->getId();

      $this->dbh->query($query);
    }
You can actually refactor that into smaller units - e.g. a query-builder. As is, a db-query doesn't belong in your Literature-Class. You can move it to your database class, where you pass a query-builder method two arrays, one being the "order"-array, i.e. the array containing the db-table column-names in a specific order and the data-array containing the data.

That would make your literature class deal with literature, not with db-queries.
Do you have a query builder class handy?
Sounds like an awesome idea. If you have one you wouldn't mind showing, I'd appreciate a peek.
McGruff wrote:Product isn't really an object - it's a hash (in the associative array sense of the word :) ). If an object is an interface which hides the implementation of something or other there is nothing to hide here. There are no methods which actually do anything apart from get/set.

In saying that I do sometimes find it convenient to use objects as hashes. You can save some typing by making a single getter and setter (actually there's no real need for getters or setters at all):

Code: Select all

function setVar($name, $value)
{
    $this->$name = $value;
}

function getVar($name)
{
    return $this->$name;
}
Yeh, I had to toss up whether to just use an associative array or an object. In the end I went with an object because I have ToSqlInsert(), ToSqlUpdate,etc.. to be used by ProductManager methods. Also I have validation and validation-related other funcitons in there. But this could arguably go in the Controller instead of the Model

Regards,
Eli