Need someone to review my class

[jonsjava]

This is a limited MySQL class that I've been writing.  It's pretty straight forward, but I want to make sure I didn't miss anything obvious.  Please comment on what I can do to improve it.  I only added methods I could see myself using.

<?php
class MyDB{
/* If you are wanting to drop a table/database, this class won't let you. use some other method.
*
*/

/**
 * User that can access the database
 *
 * @var string
 */
public $db_user;
/**
 * Password associated with user
 *
 * @var string
 */
public $db_pass;
/**
 * hostname/IP of the database server. Usually localhost
 *
 * @var string
 */
public $db_host;
/**
 * port number that the database server runs on. Default is 3306. Leave blank for default.
 *
 * @var int
 */
public $db_port;
/**
 * Database. It's the database that you are using this against.
 *
 * @var string
 */
public $db;
/**
 * The query that you are wanting to run.
 *
 * @var string
 */
public $query;

/**
 * 
 *
 * @var bool
 */
protected $cleaned = false;
/**
 * Resource link
 *
 * @var resource
 */
protected $link;


/**
 * Must be called to initialize the link
 * Usage:
 * $db = new MyDB();
 * $db->start("DBUSER","DBPASS","HOST","DB",DB_PORT(optional));
 *
 * @param string $user
 * @param string $pass
 * @param string $host
 * @param string $db
 * @param int $port
 * @return bool
 */
public function start($user, $pass, $host, $db, $port=3306){
	if (!isset($user) || !isset($pass) || !isset($host) || !isset($db)){
		return false;
	}
	else {
		$this->db_user = $user;
		$this->db_pass = $pass;
		$this->db_host = $host;
		$this->db = $db;
		$this->db_port = $port;
		$this->link = mysql_connect($this->db_host.":".$this->db_port,$this->db_user,$this->db_pass);
		mysql_select_db($this->db);
	}
}
/**
 * Must be called at the end of class usage to close database connection
 * @return null
 */
public function stop(){
	mysql_close($this->link);
}

/**
 * Returns total number of results found
 *
 * Usage: 
 * $db->query = "SELECT * FROM `some_table`;";
 * $total_found = $db->countResults();
 * echo "There are $total_found items that match your search";
 * 
 * @return int
 */
public function countResults(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	$count = mysql_num_rows($result);
	return $count;
}

/**
 * Returns multi-dimensional array containing all results
 * 
 * Usage:
 * $db->query = "SELECT * FROM `some_table`;";
 * $results = $db->getResults();
 * print_r($results);
 *
 * @return array
 */
public function getResults(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query) or die("ERROR!");
	$count = 0;
	while($row = mysql_fetch_assoc($result)){
		foreach ($row as $key=>$val){
			$output[$count][$key] = $val;
		}
		$count++;
	}
	return $output;
}

/**
 * Returns true if successful or mysql_error on failure
 * 
 * Usage:
 * $db->query = "DELETE FROM `some_table` WHERE `some_row`='some data';";
 * $outcome = $db->insertDelete();
 * if ($outcome == true){
 * 	  echo "success!";
 * }
 * else{
 *    echo $outcome;
 * }
 *
 * @return bool
 */
public function insertDelete(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	if (!$result){
		return mysql_error();
	}
	else{
		return true;
	}
}

/**
 * Same as getResults, but returns single-domensional array with only the first result
 * useful for finding the first result, or the only result(i.e., a user)
 *
 * @return array
 */
public function findOne(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	$output = mysql_fetch_assoc($result);
	return $output;
}
/**
 * Inserts items into a specified table.
 *
 * @param string $table
 * @param array $item
 * @return bool
 */
public function insert($table, $item){
	if (!array($item)){
		return false;
	}
	else{
		$total = count($item);
		$count = 1;
		foreach ($item as $key=>$val){
			$key = mysql_real_escape_string($key);
			$val = mysql_real_escape_string($val);
			$side1 .= "`$key`";
			if ($count != $total){
				$side1 .=", ";
			}
			$side2 .="'$val'";
			if ($count != $total){
				$side2 .=", ";
			}
			$count++;
		}
		$sql = "INSERT INTO `$table`($side1) VALUES($side2);";
		if (mysql_query($sql)){
			return true;
		}
		else{
			return false;
		}
	}
}

/**
 * Maintenance script. it cleans the SQL against injection attacks, then sets $cleaned to true.
 *
 * @return null
 */
protected function cleanSQL(){
	$this->query = mysql_real_escape_string($this->query, $this->link);
	if (stristr($this->query, "DROP TABLE") || stristr($this->query, "DROP DATABASE")){
		$this->query = null;
	}
	if (stristr($this->query, "DELETE FROM") && !stristr($this->query, " WHERE ")){
		$this->query = null;
	}
	if (stristr($this->query, "ALTER TABLE")){
		$this->query = null;
	}
	$this->cleaned = true;
}
}

[KevinM1]

This is a limited MySQL class that I've been writing.  It's pretty straight forward, but I want to make sure I didn't miss anything obvious.  Please comment on what I can do to improve it.  I only added methods I could see myself using.

<?php
class MyDB{
/* If you are wanting to drop a table/database, this class won't let you. use some other method.
*
*/

/**
 * User that can access the database
 *
 * @var string
 */
public $db_user;
/**
 * Password associated with user
 *
 * @var string
 */
public $db_pass;
/**
 * hostname/IP of the database server. Usually localhost
 *
 * @var string
 */
public $db_host;
/**
 * port number that the database server runs on. Default is 3306. Leave blank for default.
 *
 * @var int
 */
public $db_port;
/**
 * Database. It's the database that you are using this against.
 *
 * @var string
 */
public $db;
/**
 * The query that you are wanting to run.
 *
 * @var string
 */
public $query;

/**
 * 
 *
 * @var bool
 */
protected $cleaned = false;
/**
 * Resource link
 *
 * @var resource
 */
protected $link;


/**
 * Must be called to initialize the link
 * Usage:
 * $db = new MyDB();
 * $db->start("DBUSER","DBPASS","HOST","DB",DB_PORT(optional));
 *
 * @param string $user
 * @param string $pass
 * @param string $host
 * @param string $db
 * @param int $port
 * @return bool
 */
public function start($user, $pass, $host, $db, $port=3306){
	if (!isset($user) || !isset($pass) || !isset($host) || !isset($db)){
		return false;
	}
	else {
		$this->db_user = $user;
		$this->db_pass = $pass;
		$this->db_host = $host;
		$this->db = $db;
		$this->db_port = $port;
		$this->link = mysql_connect($this->db_host.":".$this->db_port,$this->db_user,$this->db_pass);
		mysql_select_db($this->db);
	}
}
/**
 * Must be called at the end of class usage to close database connection
 * @return null
 */
public function stop(){
	mysql_close($this->link);
}

/**
 * Returns total number of results found
 *
 * Usage: 
 * $db->query = "SELECT * FROM `some_table`;";
 * $total_found = $db->countResults();
 * echo "There are $total_found items that match your search";
 * 
 * @return int
 */
public function countResults(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	$count = mysql_num_rows($result);
	return $count;
}

/**
 * Returns multi-dimensional array containing all results
 * 
 * Usage:
 * $db->query = "SELECT * FROM `some_table`;";
 * $results = $db->getResults();
 * print_r($results);
 *
 * @return array
 */
public function getResults(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query) or die("ERROR!");
	$count = 0;
	while($row = mysql_fetch_assoc($result)){
		foreach ($row as $key=>$val){
			$output[$count][$key] = $val;
		}
		$count++;
	}
	return $output;
}

/**
 * Returns true if successful or mysql_error on failure
 * 
 * Usage:
 * $db->query = "DELETE FROM `some_table` WHERE `some_row`='some data';";
 * $outcome = $db->insertDelete();
 * if ($outcome == true){
 * 	  echo "success!";
 * }
 * else{
 *    echo $outcome;
 * }
 *
 * @return bool
 */
public function insertDelete(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	if (!$result){
		return mysql_error();
	}
	else{
		return true;
	}
}

/**
 * Same as getResults, but returns single-domensional array with only the first result
 * useful for finding the first result, or the only result(i.e., a user)
 *
 * @return array
 */
public function findOne(){
	if ($this->cleaned != true){
		$this->cleanSQL();
	}
	$result = mysql_query($this->query);
	$output = mysql_fetch_assoc($result);
	return $output;
}
/**
 * Inserts items into a specified table.
 *
 * @param string $table
 * @param array $item
 * @return bool
 */
public function insert($table, $item){
	if (!array($item)){
		return false;
	}
	else{
		$total = count($item);
		$count = 1;
		foreach ($item as $key=>$val){
			$key = mysql_real_escape_string($key);
			$val = mysql_real_escape_string($val);
			$side1 .= "`$key`";
			if ($count != $total){
				$side1 .=", ";
			}
			$side2 .="'$val'";
			if ($count != $total){
				$side2 .=", ";
			}
			$count++;
		}
		$sql = "INSERT INTO `$table`($side1) VALUES($side2);";
		if (mysql_query($sql)){
			return true;
		}
		else{
			return false;
		}
	}
}

/**
 * Maintenance script. it cleans the SQL against injection attacks, then sets $cleaned to true.
 *
 * @return null
 */
protected function cleanSQL(){
	$this->query = mysql_real_escape_string($this->query, $this->link);
	if (stristr($this->query, "DROP TABLE") || stristr($this->query, "DROP DATABASE")){
		$this->query = null;
	}
	if (stristr($this->query, "DELETE FROM") && !stristr($this->query, " WHERE ")){
		$this->query = null;
	}
	if (stristr($this->query, "ALTER TABLE")){
		$this->query = null;
	}
	$this->cleaned = true;
}
}

 

A few things:

 

1. Your data members (a.k.a., your properties) should be private.  Leaving them public jeopardizes encapsulation.  Also, unless you're planning on creating subclasses, you shouldn't have them protected.

 

2. Outside of a learning exercise, I don't see what the point of this class is.  PHP already comes with an OOP extension for its MySQL stuff (see: MySQLi), and a more generalized data access abstraction layer (see: PDO).

 

3. Returning a multidimensional array is a redundant extra step.  MySQL results are already multidimensional arrays, and the client code will just have to iterate through it all line-by-line anyway in order to use it.

 

4. The idea behind your cleanSQL method is good, but it's better addressed at the db level.  PHP scripts should never use db root access.  Instead, they should use a specific user account that has the bare minimum level of rights as needed to function.

 

Not trying to be the turd in the punchbowl here, but it looks like you're trying to reinvent the wheel.  I think a better exercise would be to create some sort of user management system.  It'd force you to consider how objects interact, which is the most important aspect of OOP.  How would you manage users?  What about user groups?  Or user privileges?  How would you store/retrieve that info from the db?  What about registering new users?  Or logging user activity?  It's a problem with several potential solutions, each more rewarding and useful than bolting on a few methods to preexisting code, IMO.

 

Just some food for thought.

[ignace]

$db = new MyDB();
$db->start("DBUSER","DBPASS","HOST","DB",DB_PORT(optional));

 

would be better if you put that into the constructor and let the constructor then proxy start(), thus:

 

$db = new MyDB("DBUSER","DBPASS","HOST","DB",DB_PORT(optional));
public function __construct($dbuser, $dbpass, $dbhost, $dbname, $dbport) {
     $this->start(..);
}

 

I noticed you are writing for PHP 5 therefor you can use __destruct:

 

public function __destruct() {
    $this->stop();
}

 

if ($this->cleaned != true){
    $this->cleanSQL();
}

 

This code is used multiple times, wrap it!

 

$result = mysql_query($this->query); should not be in the countResults() + mysql_query() has a second parameter to specify the connection you wish to use i suggest using it as you may work with multiple db's

 

$result = mysql_query($this->query) or die("ERROR!"); or die() should not be used within a class, use exceptions instead

 

$result = mysql_query($this->query); is used at multiple locations, you know the drill, wrap it!

 

return mysql_error(); would be better if you used: throw new Exception(mysql_error(), mysql_errno());

 

if (!array($item)) should be if (!is_array($item))

[jonsjava]

great responses! I'm just now moving away from procedural programming to OOP, so any assistance I can get is great!

 

about the "or die()". I had added that in for debugging my class, and forgot to remove it.  Thanks for the heads up.

 

Oh, and about reinventing the wheel: That's why I decided to do this script.  I know the regular way to run these queries, so I know what to expect as outputs.  I can compare my classes results against what I should get to make sure my methods are performing correctly.

[Daniel0]

Please comment on what I can do to improve it.

 

How about error handling?

[jonsjava]

actually, I hadn't thought about that too much.  I'll get on that, and post what I cobble together for your criti....errrm...review. *lol*. Thanks everybody for helping a freak get into new water, and out of his comfort zone.

[ignace]

reinventing the wheel is a very bad proverb IMO. Let's assume I don't and I use the existing wheel, then who do I turn to when it is broke? The garage? And what if the model isn't made anymore? And their current models arent backwards compatible to my model?

 

So I'm left with one option try to figure out how they have put it together in the first place and try to fix whatever is wrong. The better option is to not use the wheel of someone else and always create my own wheels, that way if one is broke I can just by looking at it tell from where the problem originates (well atleast most of the time) and fix it.

[Daniel0]

Also, unless you're planning on creating subclasses, you shouldn't have them protected.

 

I disagree. What if someone else wants to?

[KevinM1]

reinventing the wheel is a very bad proverb IMO. Let's assume I don't and I use the existing wheel, then who do I turn to when it is broke? The garage? And what if the model isn't made anymore? And their current models arent backwards compatible to my model?

 

Unless you're Amish, you upgrade.

 

Is there value in learning how things are done under the hood?  Of course.  That's why the guts of most building block data structures (lists, stacks, maps, queues, etc) are taught.  It brings to light the pros and cons of using such structures, and builds a sort of intimacy with them so you know when and how to use them properly.  This is what the OP is doing, and more power to them. 

 

But in the real world, unless you're the guy writing the library code, or a hardcore computer scientist, you're not going to need to roll your own.  And if it's something that ships with the language?  Unless it's absolute crap, why bother addressing it?

[KevinM1]

Also, unless you're planning on creating subclasses, you shouldn't have them protected.

 

I disagree. What if someone else wants to?

 

Good point, although I don't see much reason to subclass here.  If it was something more generalized, like, say, something more along the lines of PDO?  Then sure.  Have the constructor be a Factory Method that returns the correct underlying db object based on the structure of the DSN info, which then fleshes out the abstract public interface.

[ignace]

Unless you're Amish, you upgrade.

 

It's not always possible to upgrade. Not every company wants to spend money every year to upgrade a system that is already working fine. And when they do upgrade a decade has past.

 

Personally I'm suffering from the not-made-here principle. I will only re-use code that I have written and tested myself, no matter how many classes are already out there on the subject. If it breaks I'll have no-one to return to.

[KevinM1]

Unless you're Amish, you upgrade.

 

It's not always possible to upgrade. Not every company wants to spend money every year to upgrade a system that is already working fine. And when they do upgrade a decade has past.

 

Oh, I understand that.  The best example I can think of is PHP 4 vs 5.  It made sense that a lot of companies wouldn't switch originally.  But now, with version 4 support gone, and PHP 6 inching closer to reality, it makes sense for them to at least switch to 5, which has been around for what, 5+ years now?

 

Personally I'm suffering from the not-made-here principle. I will only re-use code that I have written and tested myself, no matter how many classes are already out there on the subject. If it breaks I'll have no-one to return to.

 

I used to be like that, but found myself working more on the tools I needed rather than the projects themselves.  I'm still generally weary of 3rd party code, but I have no problem using something like Code Igniter, Zend Framework, or jQuery.  They're proven products that work and make my life more efficient.  Given their popularity and seeming ubiquity, by the time I'd have to abandon them (or be abandoned by them) I'd probably have to migrate anyway.

[Daniel0]

Personally I'm suffering from the not-made-here principle. I will only re-use code that I have written and tested myself, no matter how many classes are already out there on the subject. If it breaks I'll have no-one to return to.

 

If you use open source libraries/frameworks, that's not entirely true. What's preventing you from creating a bug report with an attached patch to projects like jQuery, Zend Framework, PHPUnit or Doctrine? All of these four are pretty large projects, so they're being overlooked by a lot of people and tested (security/performance/stability) in real environments by a lot of people. It'll likely be better than what you can churn out, and it'll save you a lot of time.

 

You just have to be careful with what you choose to use.

 

Also, it's no different than if there is a bug in PHP itself. Who do you then turn to? Right, the PHP devs.

[ignace]

Yes ofcourse there is a big difference if the script was developed by some John Doe (snipplr, phpclasses.org, PEAR, ..) or if an entire community or a company has created it.

 

But generally, I tend to use only third-party script's if it has been developed and is being maintained by a company I trust wether open- or closed-source. The only time that I would use other third-party scripts (John Doe, community) is when I'm developing for my own and not when I'm building an application for some client. The same goes for back-ups. I have a back-up for my back-up.

 

Yes I'm a control-freak but I've got that under control.

[ignace]

What's preventing you from creating a bug report with an attached patch to projects like jQuery, Zend Framework, PHPUnit or Doctrine?

 

Time. What do you mean by 'an attached patch'? A solution to the problem?

 

Ok I realize that this even can happen with the script from the company I trust(ed) or that the problem even can be PHP itself. In the first case there is someone who you can bill the amount the customer has billed you for the problem not being resolved in short notice. In the second case however things gets complicated like it does with most open-source solutions.

 

We usually use open-source solutions at area's in the application where it wouldn't be a big deal if it would break.

[Daniel0]

What's preventing you from creating a bug report with an attached patch to projects like jQuery, Zend Framework, PHPUnit or Doctrine?

 

Time. What do you mean by 'an attached patch'? A solution to the problem?

 

Assuming their bug tracker allows you to attach files (like you can on this forum), you could attach a file that is patched (a version of the file where you fixed the bug), or better yet a diff.