OOP Database class advice

[keeps21]

I've created a singleton database class.

 

All it does is make a connection and select the specified database.

 

I don't want to get into why it's a singleton why not this pattern or that pattern just want advice on how it's written, what would you change etc.

 

Have I gone overboard with methods?

 

Declared all variables correctly (public, private, static)?

 

Do I even need to declare them at all?

 

Too many parameters?

 

<?php
class Database 
{
private $db_host;
private $db_user;
private $db_pass;
private $db_name;

    private static $db_connection_instance;

public static function get_instance()
{
	if (!self::$db_connection_instance) {
		self::$db_connection_instance = new Database();
	}

	return self::$db_connection_instance;
}  

private function __contruct()
{
}

private function open_connection($db_host, $db_user, $db_pass)
{
	$this->connection = mysql_connect($db_host, $db_user, $db_pass);

	if (!$this->connection) {
		Throw New Exception(mysql_error());
	}
}

private function select_database($db_name)
{
	$this->select = mysql_select_db($db_name);

	if (!$this->select) {
  	   	Throw New Exception(mysql_error());
	}
}

public function dbconnect($db_host, $db_user, $db_pass, $db_name)
{
	$this->open_connection($db_host, $db_user, $db_pass);
	$this->select_database($db_name);
}

private function close_connection()
{
	mysql_close();
}

private function __clone() // Prevent cloning of the instance
{
}

function __destruct() 
{
	$this->close_connection();
}

}

try
{
$db = Database::get_instance();
$db->dbconnect('localhost', 'root', '', 'wwadmin_ww');
}
catch (Exception $e)
{
	echo 'Database Error: '.$e;
}

 

[keeps21]

Another question I have is have I used '$this->' when it's just not necessary?

[Ken2k7]

private $db_host;
private $db_user;
private $db_pass;
private $db_name;

Those are never used in the class.

[keeps21]

private $db_host;
private $db_user;
private $db_pass;
private $db_name;

Those are never used in the class.

 

Good point.

 

I've rewritten the class taking your advice into consideration.

 

<?php
class Database 
{
    private static $db_connection_instance;
private $select;
private $connection;

public static function get_instance()
{
	if (!self::$db_connection_instance) {
		self::$db_connection_instance = new Database();
	}

	return self::$db_connection_instance;
}  

private function __contruct()
{
}

private function open_connection($db_host, $db_user, $db_pass)
{
	$connection = mysql_connect($db_host, $db_user, $db_pass);

	if (!$connection) {
		Throw New Exception(mysql_error());
	}
}

private function select_database($db_name)
{
	$select = mysql_select_db($db_name);

	if (!$select) {
  	   	Throw New Exception(mysql_error());
	}
}

public function dbconnect($db_host, $db_user, $db_pass, $db_name)
{
	$this->open_connection($db_host, $db_user, $db_pass);
	$this->select_database($db_name);
}

private function close_connection()
{
	mysql_close();
}

private function __clone() // Prevent cloning of the instance
{
}

function __destruct() 
{
	$this->close_connection();
}
}

 

Any more suggestions?

[anupamsaha]

Few improvements w.r.t. method naming, use of $this->connection and removal of $select as class member.

 

<?php
class Database 
{
    private static $instance;
private $connection;

public static function getInstance()
{
	if (!self::$instance) {
		self::$instance = new self();
	}

	return self::$instance;
}  

private function __contruct()
{
}

private function open($db_host, $db_user, $db_pass)
{
	$this->connection = mysql_connect($db_host, $db_user, $db_pass);

	if (!$this->connection) {
		Throw New Exception(mysql_error());
	}
}

private function select($db_name)
{
	$select = mysql_select_db($db_name);

	if (!$select) {
  	   	Throw New Exception(mysql_error());
	}
}

public function connect($db_host, $db_user, $db_pass, $db_name)
{
	$this->open($db_host, $db_user, $db_pass);
	$this->select($db_name);
}

private function close()
{
	if ($this->connection) {
		mysql_close();
	}
}

private function __clone() // Prevent cloning of the instance
{
}

function __destruct() 
{
	$this->close();
}
}
?>

 

Also, I guess, you will surely come up with "query" and other method also in this class.