Critique an OOP Script Please (DB Layer...)

[corbin]

Yeah, I wrote a DB layer.  This is actually quite old, but I just rewrote part of it, so I was wondering if it's even decent.  (Before, I have 20 people tell me, yes, I realized PDO and others are very, very good, but I like using my own stuff, and besides, it was a learning experience.)

 

 

So basically I want someone more OOP-savy (I'll be the first to say that I suck at OOP) to tell me what is wrong with this.  (Yeah, what is wrong, not if anything is wrong ;p.)

 

 

/*

CDB is essentially just used to get instances of the DB

It's used like:

try {
$db = CDB::GetDB('main');
$q = $db->query("SELECT * FROM somewhere");
while($r = $q->fetch_assoc()) {
	print_r($r);
}
}
catch(CDBException $e) {
echo 'Blah got error: ' . $e->getMessage();
}

*/

class CDB {

protected $config = array(
	'host' => 'localhost',
	'user' => 'root',
	'pass' => null,
	'db' => null,
	'port' => 3306,
);

public static function GetNew($config) {
	$adapter = $config['adapter'];
	$adapter = 'CDB_Adapter_' . $adapter;
	$d = dirname(__FILE__);
	if(!file_exists($d . '/Adapter/' . $adapter . '.php')) {
		echo $d . '/Adapter/' . $adapter . '.php';
		throw new Exception('Adapter does not exist.');
	}

	require_once $d . '/Adapter/' . $adapter . '.php';
	return new $adapter(array_merge($this->config, $config));
}

public function __get($key) {
	return self::GetDB($key);
}

public static function GetDB($w = 'main') {
	static $insts = array();
	if(!isset($insts[$w])) {
		$insts[$w] = CDB::GetNew(
			/*
				Is it bad to make my Config class be used?  Should I allow passing params to GetDB?
			*/
			Config::GetInst()->Get('db_'.$w)
		);
	}
	return $insts[$w];
}

}

/*
I get the feeling people won't like the entire uselessness of CDBException ;p
*/

class CDBException extends Exception { }

/*
Interface to ensure that the adapters have certain methods
*/

interface CDB_Adapter {
public function connect();
public function query($query);
public function execute($query);
public function affected_rows();
public function escape($string);
public function quote($string);
}

/*
This is extended by result classes that go with each adapter.  For example, CDB_Adapter_MySQL_Result.
*/

class CDB_Adapter_Result {

protected $res;

public function __construct(&$res) {
	$this->res = $res;
}

/*
	This is kind of ghetto.  It's also the only reason this is a class that is extended instead of an interface.  I guess I could just move this to every adapter, and make this an interface.  Good or bad?
*/

public function fetch_assoc_all() {
	if(!$this->num_rows()) return array();
	$ret = array();
	while($r = $this->fetch_assoc())
		$ret[] = $r;

	return $ret;
}

/*
	These are of course meant to be overwritten.  This really should be an interface..... Bleh.
	(And yeah, I would add fetch_object and so on things if I ever went full out with this)
*/
public function fetch_assoc() {}
public function fetch_row() {}
public function num_rows() {}

}

 

<?php

/*
A sample adapter, and also 1/2 I've written.  (Also have an unfinished MySQLi)

*/

class CDB_Adapter_MySQL implements CDB_Adapter {

private $conn = false;
private $affected;

/*
	Stole the concept of lazy connections from Zend_DB.
	I can't remember how they did it though.  If they called it in every method, or if they checked if a connection was made and then called it if not.

*/

public function connect() {
	if($this->conn) return true;
	if($this->conn = @mysql_connect($this->config['host'], $this->config['user'], $this->config['pass'])) {
		if(isset($this->config['db'])) {
			if(@mysql_select_db($this->config['db'], $this->conn)) {
				return true;
			}
			/*
				Should I be using numeric codes with class constants or something?
			*/
			throw new CDBException('Error selecting database');
		}
		return true;
	}
	throw new CDBException('Error connecting to MySQL with supplied settings: ' . mysql_error());
}

public function query($q) {
	/*
		The calling connect in every method [that needs a connection] thing that I mentioned earlier.
	*/
	$this->connect();
	if($q1 = mysql_query($q, $this->conn)) {
		return new CDB_Adapter_MySQL_Result($q1);
	}
	throw new CDBException('Query error: ' . mysql_error($this->conn) . ' Query: ' . $q);
}

/*
	Is it considered bad practice to have aliases like this?  I mean it's a waste of space/memory, and if I made CDB_Adapter an interface, this would not be required [in my mind] so some adapter might not have it.
*/

public function exec($q) {
	return $this->execute($q);
}

public function affected_rows() {
	return $this->affected;
}

public function execute($q) {
	$this->connect();
	if($q = @mysql_query($q, $this->conn)) {
		$this->affected = mysql_affected_rows($this->conn);
		return true;
	}
	throw new CDBException('Query error: ' . mysql_error($this->conn));
}

public function escape($s) {
	$this->connect();
	return mysql_real_escape_string($s, $this->conn);
}

public function quote($s) {
	return "'".$this->escape($s)."'";
}

/*
	I wrote this for some reason or other....  Don't remember why.  CDB was meant to be an abstraction layer, not a feature provider (if you get what I mean), so I'll probably remove this.
*/

public function update($table, $values, $where) {

	$v1 = array();

	foreach($values as $k => $v) {
		$v = $this->escape($v);
		$v1[] = "{$k} = '{$v}'";
	}
	$v = implode(', ', $v1);

	$w1 = array();

	foreach($where as $c => $w) {
		$w = $this->escape($w);
		$w1[] = "{$c} = '{$w}'";
	}

	$w = implode(' AND ', $w1);

	$q = "UDPATE {$table} SET {$v} WHERE {$w}";

	return $this->execute($q);

}

}

class CDB_Adapter_MySQL_Result extends CDB_Adapter_Result {

public function fetch_assoc() {
	return mysql_fetch_assoc($this->res);
}

public function fetch_row() {
	return mysql_fetch_row($this->res);
}

public function num_rows() {
	return @mysql_num_rows($this->res);
}

}

 

 

 

And now I'm going to go dissect Zend_DB in a learning attempt again.... lol

[DarkWater]

Doesn't look too bad, and I don't think it's absolutely horrible to use your config class as a static global, but that's just me.  Some people would probably object to it, but I don't care too much.  Looks pretty good.  I'll re-read it in a bit, but I didn't see anything that I'd readily change.

[corbin]

Besides it being a singleton used like that, would it be bad practice to make it stuck to my Config class so much?

 

Would it be better to have __construct accept an array of config, and do CDB::GetDB(Config::GetInst()->Get('main_db'))?

 

 

Also, would there be a better way to do it than CDB::GetDB()?

[DarkWater]

Yeah, I was thinking about it, and you might want to pass in the config like that.  And the factory method seems fine.

[alexweber15]

Yeah, I was thinking about it, and you might want to pass in the config like that.  And the factory method seems fine.

 

Hey DarkWater, time to gain more OOP knowledge from you! 

 

I'm just trying to understand the nuances of this code   (no comments on Corbin's, sorry!)

 

- the DB Layer Factory as a singleton is a good idea?

- the actual DB Layer shouldn't be a singleton (wouldn't this present problems having multiple DB Layers each using different adapters) ?

- Storing the Config in a seperate location and passing the info as a parameter to the constructor is good?

 

(i'm loosely using the terms good/bad here, just trying to learn a thing or two!)

[DarkWater]

1) Essentially, it's not a singleton.  It produces new instances of the database object each time the static factory method is called.  static =/= singleton

 

2) Already addressed, it's not a singleton.  He could keep making new database connections.

 

3) Yes, a Config object is often good to keep settings in one place, and passing it in instead of global using it aids in decoupling the classes, because in reality, a database factory has nothing to do with a Config, it should just do what it's told, so the config data is passed in.

 

 

[corbin]

1/2 were aimed at alexwebe15, right?

 

 

 

3.  Yeah, that's what I figured.

[DarkWater]

1/2 were aimed at alexwebe15, right?

 

 

 

3.  Yeah, that's what I figured.

 

Si.

[alexweber15]

1) Essentially, it's not a singleton.  It produces new instances of the database object each time the static factory method is called.  static =/= singleton

 

2) Already addressed, it's not a singleton.  He could keep making new database connections.

 

3) Yes, a Config object is often good to keep settings in one place, and passing it in instead of global using it aids in decoupling the classes, because in reality, a database factory has nothing to do with a Config, it should just do what it's told, so the config data is passed in.

 

thanks for clearing that up and yeah duh, i missed the fact that the constructor wasnt private and because of the static way of getting instances i assumed singleton 

 

im currently building a DAO class for an application im writing using PDO (which is kinda silly i guess since its creating a DB Layer class for another DB Layer Class in a way) but i just want to have more control over the PDO class.

 

This just occurred to me: keep writing the class or extend PDO and intercept and block functionality i don't want to be able to use?    (im guessing the later but who knows)

 

Also, i now conclude that having a singleton DB Layer is a bad idea!   (time to change private to public on my constructor...  )

[corbin]

Sorry for the useless bump, but uhhh...  Sorry bout the wrong forum thing ;p.