Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

One of my friend made a framework.

He made an abstract class, called NullObject (see below).

For his convenience, hid does this, so when he want to build an object from database, just inherit this class, set the table, and pass the ID of the row what he want.

So he does not need to create manually all the property for the inherited class.

In this case all the property will be public. (It can be changed the id to protected, and insert a setter / getter for that, but now i want to ask you about the rest).

Is it a good or a bad practice? I bet, this is bad, i just need your opinion, pros / contras.

<?php

abstract class NullObject {

    protected $table = '';
    public $id = 0;

    public function __construct($id = 0) {
        if (!empty($id)) {
            $this->id = $id;
            $row = $this->getRow($this->id);
            if ($row) {
                $this->setProperties($row);
                return $this;
            }
        }
    }

    protected function setProperties($row) {
        $row = (array) $row;
        if (count($row)) {
            foreach ($row as $key => $val) {
                $this->$key = $val;
            }
        }
    }

    public function getRow($id) {
        $sql = "SELECT * FROM " . $this->table . " 
                WHERE id = " . $this->id;
        return dbGetRow($sql);
    }
share|improve this question
 
I'm not sure if this belongs on SO. To answer your question a little, you might want to check out "ORM", there are other people who already wrote classes with similar functionality - either use or learn from them –  kingkero Sep 13 '13 at 13:06
 
As an FYI so mods don't get flooded - I have already flagged this for mod attention to be moved. –  SmokeyPHP Sep 13 '13 at 13:09
3  
Why the hell there is a return in the __construct() ?! –  tereško Sep 13 '13 at 13:10
2  
doesn't matter, the __constructor will always return the instance, any return statements will be ignored. –  NDM Sep 13 '13 at 13:15
1  
Your friend made bad framework –  djay Sep 13 '13 at 15:09
show 1 more comment

migrated from stackoverflow.com Sep 13 '13 at 15:46

This question came from our site for professional and enthusiast programmers.

3 Answers

To have table objects is, on its own, a good idea. To write your own, especially in the way you describe, is poor execution, to say the least. In previous answers, I've gone into more details on why I find it rather pointless to write your own DB abstraction layers, especially with things like Doctrine being available. Instead of going into all that again, check this answer here, and perhaps consider having a look at this one, too.

Now why do I say the execution is poor? For a number of reasons, but here are a couple of key-reasons:

public function getRow($id)
{//you're not using the $id argument
    $sql = "SELECT * FROM " . $this->table . " 
            WHERE id = " . $this->id;//not checking isset/type of $this->id
    return dbGetRow($sql);//calling global function?
}

Here's just a single method, containing 2 statements. In order for these statements to execute correctly, you're assuming both $this->table and $this->id were set properly, and contain valid information. You also assume the DB connection has been made already, and you can just pass a query to some global function. That's just not how you do things when working with objects.

The other method that I'd bin, if I were you is this:

protected function setProperties($row)
{
    $row = (array) $row;//if you expect an array, type-hint!
    if (count($row))
    {
        foreach ($row as $key => $val)
        {//how do you know $key is  even a string?
            $this->$key = $val;
        }
    }
}

You're also setting properties dynamically (through PHP's overloading of objects). Adding new properties on the fly is to be avoided as much as possible. I've gone into more details on this matter here, and more of the technical information on how overloading affects overall performance can be found here.
Leaving asside the technical aspect for a moment, on why this is not the best way to go about defining a table, anyone working on your code would have to either spend time either looking for which table has what schema and then assume the properties are set accordingly, or will find himself looking for the point in your code where you're actually creating the array that is being used here.
Besides, even if I'd know what the table looked like, this object would still allow me to set the properties to the wrong types. Assume, for a moment that I wrote this class, to insert email addresses into a db:

class EmailTable extends AbstractTable
{
    /**
     * @var id PK int
     */
    protected $id = null;
    /**
     * @var string
     */
    protected $email = null;
    /**
     * @var tinyint
     */
    protected $validated = null;

    /**
     * constructor
     */
    public function __construct(array $data = null)
    {
        if ($data)
        {
            parent::__construct($data);
        }
    }

    /**
     * set email (null to unset, invalid email === null)
     * @param string $email = null
     * @return \EmailTable
     */
    public function setEmail($email = null)
    {
        if ($email === null || !filter_validate($email, FILTER_VALIDATE_EMAIL))
        {//VALIDATE INPUT HERE!
            $email = null;
        }
        $this->email = $email;
        return $this;
    }

    /**
     * get Email
     * @return string|null
     */
    public function getEmail()
    {
        return $this->email;
    }
}

Assume I did the same (ie define getters and setters for all properties), and had an abstract/parent class that looked like this:

abstract class AbstractTable
{
    public function __construct(array $data)
    {
        foreach($data as $property => $value)
        {
            if (method_exists($this, 'set'.ucfirst($property)))
            {
                $this->{'set'.ucfirst($property)}($value);//ensure validation
            }
        }
    }
    final public function __set($name, $val)
    {//disable overloading
        $name = 'set'.ucfirst($name);
        if (!method_exists($this, $name))
        {//object represents table, so you can't overload it!
            throw new RuntimeException('Table objects are tightly coupled to tbl schema');
        }
        return $this->{$name}($value);
    }
    final public function __get($name)
    {
        $name = 'get'.ucfirst($name);
        if (!method_exists($this, $name))
        {
            throw new RuntimeException(substr($name, 3).' is not a valid property...');
        }
        return $this->{$name}();
    }
}

Now, with code like this, you can do pretty much everything you're doing, only: if you add a property, you'll get an exception (the property can't be linked to a field in the table, so fix your code... there's a bug). You're also certain that, even when setting values through an array, the setter methods will sanitize and check the data's validity. I, in this example can't set the $email property to 123, for example. I can only pass a valid email address to that property. That is, after all, the point of the table.

Note, if you will, that my code does, in no way contain any queries, nor does it contain any connection to a db. That's simply because I don't know, nor should I know, who and how this object will be used. Your code doesn't allow for complex queries with multiple joins, for example. Not that I'm aware of, at least. Ah well, look into the linked answers, I've explained it all there, already. I'm not going to keep on repeating myself.

share|improve this answer
add comment

It's not only a bad practice, but also a vulnerability - user can set field table to any value and next query will affect that table.

share|improve this answer
add comment

The idea is good but the execution is lacking. I would suggest implementing the DAO pattern which you can read about in detail in this question or in this tutorial.

In short, the pattern defines an interface, usually BaseDAO that will specify methods to fetch your objects. You are still required to implement one concrete DAO per class you want to instantiate but the pattern has great benefits when it comes to reusability and maintenance.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.