Database wrapper class

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

scottayy wrote:Could've swore that was everah 8O My appologies, aborint!
Yeah, I mean to say arborint, but ole's av is cooler. (I just realized it blinks. Has it ALWAYS blinked???)

Also, this is getting me... In the Result class, theres a little inconsistency with $link_id and $this->link.

It was eating away at me.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

So I got this..

Code: Select all

<?php

/*
* MySQL database wrapper class
*/

class db_connect
{
	/*
	* MySQL link identifier
	* @resource $link
	*/
	var $link_id;
	
	/*
	* MySQL host
	* @string $host
	*/
	var $host;
	
	/*
	* MySQL username
	* @string $username
	*/
	var $username;
	
	/*
	* MySQL password for $this->username
	* @string $password
	*/
	var $password;
	
	function __construct($host, $username, $password)
	{
		$this->host		= $host;
		$this->username	= $username;
		$this->password	= $password;
	}
	
	/*
	* Connects to a mysql database server
	*/
	function connect()
	{
		$this->link_id = mysql_connect($this->host, $this->username, $this->password)
			or die(mysql_error());
	}
	
	/*
	* Selects a database to use on database server
	* @param str $database
	*/
	function use_db($database)
	{
		mysql_select_db($database, $this->link_id);
	}
	
	/*
	* Performs a query on the database and returns the result set
	* @param str $sql
	*/
	function query($sql)
	{
		if(!$this->link_id)
		{
			$this->connect();
		}
		
		$result = new db_result($this->link_id, mysql_query($sql, $this->link_id)); 
		return $result;
	}
	
	/*
	* Escapes a variable for passing to a query
	* @param var $variable
	*/
	function escape($variable)
	{
		return mysql_real_escape_string($variable, $this->link_id);
	}
	
	
}

class db_result
{
	
	var $link_id;
	var $result;
	
	function __construct($link_id, $result) 
	{ 
		$this->link_id = $link_id;
		$this->result = $result;
	} 
	
	/*
	* Gathers and returns the number of rows from a result set
	* @param resource $result
	*/
	function num_rows()
	{
		return mysql_num_rows($this->result);
	}
	
	/*
	* Fetches a single row from a result set
	* @param resource $result
	* @param constant $fetch_type
	*/
	function fetch_row($fetch_type=MYSQL_ASSOC)
	{
		return mysql_fetch_array($this->result, $fetch_type);
	}
	
}

?>
1. Why would I leave escape() in the connection class?
2. Now when I call $db->num_rows(), I get the error Fatal error: Call to undefined method db_connect::num_rows() in C:\Apache2\htdocs\admin_edit-blog-entry.php on line 21. How can I avoid this error without procedurely instantiating a new db_result() class in this code:

Code: Select all

if(empty($_GET['blog_id']) || !is_numeric($_GET['blog_id']))
{
	$error = 'Invalid Blog Entry Specified.';
} else
{
	$blog_id	= $db->escape(htmlentities(stripslashes($_GET['blog_id']), ENT_QUOTES));
	$result		= $db->query("SELECT * FROM `blogs` WHERE `id` = '$blog_id' LIMIT 1");
	
	if($db->num_rows())
	{
		$blog = $db->fetch_row($result);
	} else
	{
		$error = 'Invalid Blog Entry Specified';
	}
}
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

$db->num_rows() doesn't exist.
$result->num_rows() does.


And putting escape in the class stops you from typing mysql_real_escape_string() all the time. I'd never really thought of doing it before.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

Oh, I SEE!! That's awesome!!

by assigning the object of db_result and returning it. when I do $result = $db->query(), $result is now the object of the db_result

And what I meant by leaving escape in there, was.. should it be in the db_connect() class?
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

Sure. The only time you'll ever need it is while running db_connect's query() function.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

ole wrote:Would that be premature optimization you are suggesting their feyd? :P
No, just something that could be added (under a plugin architecture.)
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

So, I've rebuilt this (using my head, I promise). I didn't include comments, cuz right now they're just confusing me.

Code: Select all

<?php

class db_connect
{
	var $host;
	var $username;
	var $password;
	var $link_id;
	
	function __construct($host, $username, $password)
	{
		$this->host = $host;
		$this->username = $username;
		$this->password = $password;
	}
	
	function connect()
	{
		$this->link_id = mysql_connect($this->host, $this->username, $this->password);
	}
	
	function use_db($db)
	{
		mysql_select_db($db, $this->link_id);
	}
	
	function escape($var)
	{
		return mysql_real_escape_string($var, $this->link_id);
	}
	
	function query($sql)
	{
		if(!$this->link_id)
		{
			$this->connect();
		}
		
		return new db_result($this->link_id, mysql_query($sql, $this->link_id));
	}
}

class db_result
{
	var $link_id;
	var $result;
	
	function __construct($link_id, $result)
	{
		$this->link_id = $link_id;
		$this->result = $result;
	}
	
	function num_rows()
	{
		return mysql_num_rows($this->result);
	}
	
	function fetch_rows($fetch_type=MYSQL_ASSOC)
	{
		return mysql_fetch_array($this->result, $fetch_type);
	}
}

?>
Questions:

1: Should this be split into two files? ie: db_connect.class.php & db_result.class.php
2: "You probably also want to add isError() and getError() methods" Could someone give me a pointer on how to get started on these. I know what they're for, but not when to call is_error(), or how to set the errors. Should these be in a new class as well (db_error)?
3: Can someone reference me an article about private, public, protected, etc keywords before the functions and properties? I'm almost pretty sure that "var" shouldn't be used?
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

1) This is PHP. You don't have to split classes into different files if you don't want to.
2) I'm as clueless as you are. However, when there's a problem, there is a mysql_error(), so maybe you should save the errors in case query() fails.
3) We only use var for PHP4. Once you get into PHP5, you'll want to use the public, provate, and protected keywords.

If the function need to be accessible externally, make it public.
If the function should only be used from within the object, make it protected.
Member variables should be private, and accessible through get/set methods.

You don't have any that should be protected, but if you had connect() called through the constructor instead of calling it on your own, you could make it protected.


I don't have any articles, but it's not too hard of a concept to grasp.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

3) We only use var for PHP4. Once you get into PHP5, you'll want to use the public, provate, and protected keywords.
and exceptions and abstracts and interfaces and occasionally statics and overloading.

Code: Select all

return new db_result($this->link_id, mysql_query($sql, $this->link_id));
You'll probably want to check for query success first. If it fails trigger and error and result false. You don't want to return a result object with an invalid state.
1: Should this be split into two files? ie: db_connect.class.php & db_result.class.php
I would say so.
3: Can someone reference me an article about private, public, protected, etc keywords before the functions and properties? I'm almost pretty sure that "var" shouldn't be used?
Public -> accessible to all.
Protected -> accessible to self and subclasses.
Private -> accessible to self only.

These 3 keywords (there's a forth in Java) define the visibility of the class and allow you to achieve encapsulation. When I was learning C++ this was known as data hiding as well. The purpose of this is pretty much the same as function scope. You don't want to allow class internals to be fiddled with from the outside.

You should know by now there are usually no hard rules; everyone has their own opinion on everything; a lot of things have to be assessed on a case by case basis. Public properties are one of the few exceptions to this. Public properties are bad. If you never make a property public you'll be fine. Protected and private are different. I prefer to make everything non-public private and make them protected if it turns out I need them further down the inheritance chain. Others almost never use private at all, you only very occasionally need private - singletons are a good example of this.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

So, now I have this:

db_connect.class.php

Code: Select all

<?php

class db_connect
{
	private $host;
	private $username;
	private $password;
	private $link_id;
	
	public function __construct($host, $username, $password)
	{
		$this->host = $host;
		$this->username = $username;
		$this->password = $password;
	}
	
	public function connect()
	{
		$this->link_id = mysql_connect($this->host, $this->username, $this->password);
	}
	
	public function use_db($db)
	{
		mysql_select_db($db, $this->link_id);
	}
	
	public function escape($var)
	{
		return mysql_real_escape_string($var, $this->link_id);
	}
	
	public function query($sql)
	{
		if(!$this->link_id)
		{
			$this->connect();
		}
		
		if($result = mysql_query($sql, $this->link_id))
		{
			require_once getcwd().DIRECTORY_SEPARATOR.'class'.DIRECTORY_SEPARATOR.'db_result.class.php';
			return new db_result($this->link_id, $result);
		}
	}
}

?>
db_result.class.php

Code: Select all

<?php

class db_result
{
	private $link_id;
	private $result;
	
	public function __construct($link_id, $result)
	{
		$this->link_id = $link_id;
		$this->result = $result;
	}
	
	public function num_rows()
	{
		return mysql_num_rows($this->result);
	}
	
	public function fetch_rows($fetch_type=MYSQL_ASSOC)
	{
		return mysql_fetch_array($this->result, $fetch_type);
	}
}

?>
I don't understand how private $link_id can get passed to the db_result class because it's private. But it does get passed.

I should probably throw out an error if query fails, but I'm not sure how to do that. Any thoughts?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Examine $result. is_resource().

If you're going to abstract the functions, you should also abstract the constants. Also, fetch_rows() would suggest, by its name, that it returns all records, not one. ;)

At this point, I don't see a reason $link_id would be apart of a result handling class.
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

It gets passed because it's accessed from within the db_connect class, and only a reference is made to it (I believe... C++ makes copies, but I think PHP creates references).

And the only thing I can think of for your error is that if the query fails, return NULL or FALSE, and every time that you query, check for errors.

Code: Select all

if(!$db->query($query)){ /* Complain */ }
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

feyd wrote:At this point, I don't see a reason $link_id would be apart of a result handling class.
Agreed. Unless you want to ensure that another db_connect object somehow uses the same link_id as a particular result, but you don't have any functions to that end. And.. I can't see it being that useful unless you're dealing with multiple connections.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

You're right, link_id isn't needed in the result class.
fetch_rows() should be fetch_row()
What constants do you speak of?

Code: Select all

<?php

class db_connect
{
	private $host;
	private $username;
	private $password;
	private $link_id;
	
	public function __construct($host, $username, $password)
	{
		$this->host = $host;
		$this->username = $username;
		$this->password = $password;
	}
	
	public function connect()
	{
		$this->link_id = mysql_connect($this->host, $this->username, $this->password);
	}
	
	public function use_db($db)
	{
		mysql_select_db($db, $this->link_id);
	}
	
	public function escape($var)
	{
		return mysql_real_escape_string($var, $this->link_id);
	}
	
	public function query($sql)
	{
		if(!$this->link_id)
		{
			$this->connect();
		}
		
		if(is_resource($result = mysql_query($sql, $this->link_id)))
		{
			require_once getcwd().DIRECTORY_SEPARATOR.'class'.DIRECTORY_SEPARATOR.'db_result.class.php';
			return new db_result($result);
		}
		
		return false;
	}
}

?>

Code: Select all

<?php

class db_result
{
	private $result;
	
	public function __construct($result)
	{
		$this->result = $result;
	}
	
	public function num_rows()
	{
		return mysql_num_rows($this->result);
	}
	
	public function fetch_row($fetch_type=MYSQL_ASSOC)
	{
		return mysql_fetch_array($this->result, $fetch_type);
	}
}

?>
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

I believe... C++ makes copies, but I think PHP creates references
PHP 4 and C++ both copy and are both ridiculous for it. Most languages reference including PHP 5.

Code: Select all

if(is_resource($result = mysql_query($sql, $this->link_id)))
                {
                        require_once getcwd().DIRECTORY_SEPARATOR.'class'.DIRECTORY_SEPARATOR.'db_result.class.php';
Using DIRECTORY_SEPARATOR like that is overkill for me. PHP doesn't care if its / or \ no matter what OS you are running. The convention in PHP however is to use /. Also you should put any file loads at the top of the file above the class declaration and certainly not inside a function unless that function purpose is to dynamically load things.

Also have you considered following Zend or PEAR coding standards? See the first link in my sig.
Post Reply