[SOLVED] Database class; On the right track?

[hostfreak]

Looking for suggestions, opinions etc on how my database class is looking so far:

 

classes/Database.class.php

<?php

    /************************************************************
    **    Database Wrapper ::
    **        Manages Connections, Queries, Retrivial etc
    ************************************************************/


    //Interface
    interface iDatabase
    {
        public function OpenConn($Host, $User, $PWRD, $DB); //Open Connection
        public function SelectDB($DB); //Select Database

    }

    //MySQL Class; Implements iDatabase
    class MySQL implements iDatabase
    {
        /* Class Members */
        protected $Link; //Hold Connection


        /* Class Methods */
        //Constructor
        function __construct() //Upon new instance of object "MySQL"
        {
            include("include/constants.php");

            $this->OpenConn(Host, User, PWRD, DB);
            $this->SelectDB(DB);
        }

        //Open Connection
        public function OpenConn($Host, $User, $PWRD, $DB) //Required
        {
            $this->Link = new mysqli($Host, $User, $PWRD, $DB);

            //Check connection
            if (mysqli_connect_errno())
            {
                printf("Connect failed: %s\n", mysqli_connect_error());
                exit();
            }
        }

        //Select Database
        public function SelectDB($DB) //Required
        {
            $this->Link->select_db($DB) OR die('Can\'t use '.$DB.' : ' . mysqli_error($this->Link));
        }

        //Destructor
        function __destruct()
        {
        }
    }
    
    //MySQL Actions; Extends MySQL Class
    class MySQL_Actions extends MySQL
    {
        function __construct()
        {
            parent::__construct();
        }
   
        //Query
        public function Query($SQL)
        {
            $Query = $this->Link->query($SQL) OR die('Invalid query: ' . mysqli_error($this->Link));
            return $Query;
        }

        //Fetch associative array
        public function fetch_Assoc($Result)
        {
            $Array = $Result->fetch_assoc();
            return $Array;
        }

        //Fetch number of rows
        public function fetch_Num_Rows($Result)
        {
            $NumRows = $Result->num_rows;
            return $NumRows;
        }
        
        /*Clean Input */
        public function RealEscapeString($String)
        {
            $String = $this->Link->real_escape_string($String);
            return $String;
        }

        //Close connection to database
        public function Close()
        {
            $this->Link->close();
        }
        
        function __destruct()
        {
            parent::__destruct();
        }
    }

?>

 

Example usage:

 

test.php

<?php

    include("classes/Database.class.php");
    
    $DB_Action = new MySQL_Actions;
    
    $SELECT_Articles = $DB_Action->Query("SELECT Name, Body, ID FROM articles");
    $Returned_Rows = $DB_Action->fetch_Num_Rows($SELECT_Articles);
    echo $Returned_Rows;
    $SELECT_Articles->free();
    
    $DB_Action->Close();

?>

 

 

Thanks in advanced.

[448191]

I can't really see the need to subclass...

[matthewhaworth]

I always put my mysql_select_db in my connection method.  Instead of having a separate one.  Also, there's no point in having a subclass, one for connection one for queries etc, thats over organisation.

[hostfreak]

I am a very organized person. Does it hurt or effect it in anyway (in some way or another, defeat a purpose?) to do it like that?

 

I see what you mean on the select_db though, I will group that with the connection method.

[matthewhaworth]

I am a very organized person. Does it hurt or effect it in anyway (in some way or another, defeat a purpose?) to do it like that?

 

I see what you mean on the select_db though, I will group that with the connection method.

 

Performance wise, I don't think it'll be significant, however you should only really have a parent class that's inherited.. (obviously, because it's a parent ) if it's going to be inherited more than once.. otherwise it defeats the idea of inheritance. 

 

Also, when using mysqli object orientedly.. you don't need a function to set db, it should be set by mysqli's constructor.  And thinking about it, since you've now just got one method.. you should really put the connect method into the constructor.  Meaning you've got no methods, just a constructor, therefore meaning lol, that you should just merge the two.

[448191]

You create a class for reason, not just because you can. For the purpose of database abstraction, you might want to expand the interface a little to ensure polymorphism...

 

Again, there is no need whatsoever to subclass in this scenario. YAGNI.

[hostfreak]

I got the idea to have separate methods for the connect and select_db from  "obsidian" post in http://www.phpfreaks.com/forums/index.php/topic,131737.0.html . As far as xpanding the interface; what would you suggest? I will also get rid of the subclass.

[448191]

Ok, there were a number of cases in your code where you were mixing up mysqli's OO and procedural style. I've taken the liberty of rewriting the whole thing. Untested (since I don't have the extension installed) and not complete (you might want to expand a little):

 

<?php
class DbException extends Exception {}
class MySQLException extends DbException {}

interface DbPreparableStmt {
public function prepare($query);
}
abstract class DbCommon  {
abstract public function connect($host, $user, $pass, $dbName = null);
abstract public function selectDb($dbName);
abstract public function query($string);
abstract public function fetchAssoc($resultSet = null);
abstract public function escapeString($string);
abstract public function numRows($resultSet = null);
abstract public function closeConnection();

final public function __construct($host, $user, $pass, $dbName = null){
	$this->connect($host, $user, $pass, $dbName);
}
public static function factory($class, $host, $user, $pass, $dbName = null){
	return new $class($host, $user, $pass, $dbName);
}
}

class MySQL extends DbCommon implements DbPreparableStmt {

private $resultset;
private $mySQLi; 

public function connect($host, $user, $pass, $dbName = null) {
	$this->mySQLi = new MySQLi($host, $user, $pass, $dbName);
	if (mysqli_connect_errno()){
		throw new MySQLException('Connect failed: '. mysqli_connect_error());
	}
}
public function selectDb($dbName) {
	if(!$this->mySQLi->select_db($dbName)){
		throw new MySQLException('Can\'t use '.$dbName.' : ' . $this->mySQLi->error);
	}
}
public function query($string) {
	if(!$this->resultset = $this->mySQLi->query($string)){
		throw new MySQLException('Invalid query: ' . $this->mySQLi->error);
	}
	return $this->resultset;
}
public function fetchAssoc($resultSet = null){
	return $this->checkResultSet($resultSet)->fetch_assoc();
}
public function numRows($resultSet = null) {
	return $this->checkResultSet($resultSet)->num_rows();
}
public function getResultSet(){
	return $this->resultset;
}
public function escapeString($string){
	return $this->mySQLi->real_escape_string($string);
}
public function closeConnection(){
	$this->mySQLi->close();
}
private function checkResultSet($resultSet){
	if($resultSet === null){
		if($this->resultset === null){
			throw new MySQLException('No resultset to fetch from.');
		}
		return $this->resultSet;
	}
		else {
		if(!$resultSet instanceof mysqli_result){
			throw new MySQLException('Invalid argument, $resultSet must be instance of mysqli_result.');
		}
		return $resultSet;

	}
}
public function prepare($query){
	return $this->mySQLi->prepare($query);
}
}
?>

 

Example usage:

 

<?php
try {
$db = DbCommon::factory('MySQL', 'localhost', 'root', 'pass', 'somedb');
$db->query('SELECT count(*) AS count FROM users');
$array = $db->fetchAssoc();
echo $array['count'];
}
catch (DbException $e){
throw new Exception('Somebody screwed up: '.$e->getMessage());
}
?>

[hostfreak]

Wow, looks nice. I'm not even 100% sure about some of the keywords, principles etc used... I am relatively new to OOP. I think I definitely have a lot more research to do; It'll definitely be easier now though, seeing it done the correct way. Thanks a bunch.

[hostfreak]

The only error I get is:

Fatal error: Call to a member function fetch_assoc() on a non-object in C:\wamp\www\Pratice\OOP\Classes\Database.class.php on line 48

 

It is probably a simple fix, though I cannot put my finger on it.

[448191]

I sneaked in a property naming bug.. 

 

In some cases I used $this->resultSet, others $this->resultset. Hence. Should be fixed now.

 

 

<?php
class DbException extends Exception {}
class MySQLException extends DbException {}

interface DbPreparableStmt {
public function prepare($query);
}
abstract class DbCommon  {
abstract public function connect($host, $user, $pass, $dbName = null);
abstract public function selectDb($dbName);
abstract public function query($string);
abstract public function fetchAssoc($resultSet = null);
abstract public function escapeString($string);
abstract public function numRows($resultSet = null);
abstract public function closeConnection();

final public function __construct($host, $user, $pass, $dbName = null){
	$this->connect($host, $user, $pass, $dbName);
}
public static function factory($class, $host, $user, $pass, $dbName = null){
	return new $class($host, $user, $pass, $dbName);
}
}

class MySQL extends DbCommon implements DbPreparableStmt {

private $resultSet;
private $mySQLi; 

public function connect($host, $user, $pass, $dbName = null) {
	$this->mySQLi = new MySQLi($host, $user, $pass, $dbName);
	if (mysqli_connect_errno()){
		throw new MySQLException('Connect failed: '. mysqli_connect_error());
	}
}
public function selectDb($dbName) {
	if(!$this->mySQLi->select_db($dbName)){
		throw new MySQLException('Can\'t use '.$dbName.' : ' . $this->mySQLi->error);
	}
}
public function query($string) {
	if(!$this->resultSet = $this->mySQLi->query($string)){
		throw new MySQLException('Invalid query: ' . $this->mySQLi->error);
	}
	return $this->resultSet;
}
public function fetchAssoc($resultSet = null){
	return $this->checkresultSet($resultSet)->fetch_assoc();
}
public function numRows($resultSet = null) {
	return $this->checkresultSet($resultSet)->num_rows();
}
public function getresultSet(){
	return $this->resultSet;
}
public function escapeString($string){
	return $this->mySQLi->real_escape_string($string);
}
public function closeConnection(){
	$this->mySQLi->close();
}
private function checkresultSet($resultSet){
	if($resultSet === null){
		if(!$this->resultSet === null){
			throw new MySQLException('No resultSet to fetch from.');
		}
		return $this->resultSet;
	}
		else {
		if(!$resultSet instanceof mysqli_result){
			throw new MySQLException('Invalid argument, $resultSet must be instance of mysqli_result.');
		}
		return $resultSet;

	}
}
public function prepare($query){
	return $this->mySQLi->prepare($query);
}
}
?>

[hostfreak]

Yep, that fixed it. Thanks 448191.

[ahowell]

As a side note, you might want to use constants for your database connection details.

When using variables, if the connection fails for whatever reason, the error outputted

to the screen will contain the value of the variables, and not the variable names themselves.

This is a security issue =D

 

Using constants in the same situation, will only output the constant name (not the value).

 

 

[hostfreak]

As a side note, you might want to use constants for your database connection details.

When using variables, if the connection fails for whatever reason, the error outputted

to the screen will contain the value of the variables, and not the variable names themselves.

This is a security issue =D

 

Using constants in the same situation, will only output the constant name (not the value).

 

 

 

Yes, I do use constants.

[448191]

As a side note, you might want to use constants for your database connection details.

When using variables, if the connection fails for whatever reason, the error outputted

to the screen will contain the value of the variables, and not the variable names themselves.

This is a security issue =D

 

Using constants in the same situation, will only output the constant name (not the value).

 

In a production environment, system errors should NEVER NEVER NEVER be outputted. Period. There is a php.ini setting to avoid that you know... This is NOT an argument to use constants. Use (class) constants for efficiency, readability or easier management, but NOT for security.

[hostfreak]

Thanks for clarifying things 448191.

[ahowell]

Security is in many layers. It is not just turning of error reporting in php.ini.

And to tell somebody not to use constants as a method of security is not good advise.

 

In of some weird scenario, I would like the comfort of knowing that because i'm using constants,

the values will not be displayed.

 

 

[448191]

Whatever suits you. You might want to turn display_errors off instead though...