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.
return
in the__construct()
?! – tereško Sep 13 '13 at 13:10__constructor
will always return the instance, any return statements will be ignored. – NDM Sep 13 '13 at 13:15