OOP guidance...

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
User avatar
wtf
Forum Contributor
Posts: 331
Joined: Thu Nov 03, 2005 5:27 pm

OOP guidance...

Post by wtf »

OK... i'm playing bit with OOP/PHP5. I got working what I needed but would like some feedback on what I've done.

Code: Select all

class DBConnectionManager {

	private $m_host;
	private $m_user;
	private $m_password;
	private $m_database;
	private $m_link;

	public function __construct($host, $user, $password, $database) {

		$this->m_host = $host;
		$this->m_user = $user;
		$this->m_password = $password;
		$this->m_database = $database;

		$this->dbConnect();
	}
	
	public function dbConnect() {
		echo '<b>DBConnectionManager->dbConnect()</b> <br />';
		try {
			$this->m_link = mysql_connect($this->m_host, $this->m_user, $this->m_password);
			
			if(!$this->m_link) {
				throw new Exception('Host not available');
			}
			
			if(!mysql_select_db($this->m_database, $this->m_link)) {
				throw new Exception('Database not available');
			}
		}
		catch (Exception $e) {
			echo $e->getMessage();
			exit();			
		}
		return $this->m_link;
	}

}	//	End class DBConnectionManager

Code: Select all

require_once('classes/DBConnectionManager.php');

class SQLHelper extends DBConnectionManager {
	
	private $m_dbm;
	private $m_result;
	private $m_row;

	function __construct() {
		parent::__construct('localhost', 'user', 'pass', 'database');
	}


	function query($query) {
		echo '<b>SQLHelper->query("' . $query . '")</b> <br />';	
		try {
			$this->m_result = mysql_query($query);

			if(!$this->m_result) {
				throw new Exception('your query sucks');
			}
		}
		catch (Exception $e) {
			echo $e->getMessage();
			exit();			
		}
		
		return $this->m_result;
	}
	
	
	function num_rows($result) {
		echo '<b>SQLHelper->num_rows()</b> <br />';
		return mysql_num_rows($result);
	}
	
	
	function queryToArray($result) {
		
		while($myrow = mysql_fetch_array($result, MYSQL_NUM)) {
			$myr[] = $myrow;
		}
		
		return $myr;
	}
	
	
	function arrayTo_TR($array) {
	
		for($i = 0; $i < count($array); $i++) {
			$this->m_row .= '<tr><td>' . $array[$i][0] . '</td><td>' . $array[$i][3] . '</td></tr>';
		}
		
		return $this->m_row;		
	}

}

Code: Select all

require_once('classes/SQLHelper.php');
	
	$sql = new SQLHelper();

	$result = $sql->query('select * from table');

	echo '<table border="1">';
	echo $sql->arrayTo_TR($sql->queryToArray($result));
	echo '</table>';
2. What's the best way to go about constructing the page. Should I just go with HTML and call methods where needed in my page or should I go with Page object and have its methods output HTML. I know that it'll probably come to preferences but would like to hear from your experience.



Regards,

wtf
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

1.

Code: Select all

$this->m_link = mysql_connect($this->m_host, $this->m_user, $this->m_password); 

 if(!$this->m_link) { 
                throw new Exception('Host not available'); 
            }
This error can be caused by invalid username and password as well...generalize error message.

2. SQLHelper class does not have any access specifies, public or private for member functions.

3. Rather extending the DBConnection class, you could declare an instance of it as private variable.

4.

Code: Select all

function num_rows($result) { 
        echo '<b>SQLHelper->num_rows()</b> <br />'; 
        return mysql_num_rows($result); 
    }
You must include this in try block to catch errors if $result is invalid.

5.

Code: Select all

function arrayTo_TR($array) { 
     
        for($i = 0; $i < count($array); $i++) { 
            $this->m_row .= '<tr><td>' . $array[$i][0] . '</td><td>' . $array[$i][3] . '</td></tr>'; 
        } 
         
        return $this->m_row;         
    }
This is a class ... do not design 'view' here ...only design control of program..I mean to say take tr, td out of class...this should be in target PHP file...

6.

Code: Select all

function num_rows($result) 
function queryToArray($result)
Inconsistent naming style.

7. 4.

Code: Select all

function num_rows($result) { 
        echo '<b>SQLHelper->num_rows()</b> <br />'; 
        return mysql_num_rows($result); 
    }
This function is supposed to $m_result member variable, there is no necessity for the user or any function to provide $result object.

8.

Code: Select all

public function __construct($host, $user, $password, $database) { 

        $this->m_host = $host; 
        $this->m_user = $user; 
        $this->m_password = $password; 
        $this->m_database = $database; 

        $this->dbConnect(); 
    }
Do not expect the user to provide connection details, provide default values
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Creating an instance within the class is not always the best idea.. Extend is there for a reason after all.

remove all echo's (especially the one that echo's the SQL statement!!), you don't wany any output to be produced by the class, same for design, you just want data to be returned or an exception thrown on error, and nothing else :)

Use a specific exception, not the generic exception, and exceptions should be caught outside of the class, not inside. Again, the class should not be responsible for the result of an exception - that is the business logics responsibility. e.g:

Code: Select all

<?php

class DBConnectException extends Exception {}

class DBConnectionManager 
{
    //etc..
    public function dbConnect ()
    {
        //etc..
        throw new DBConnectException ('Error connecting to DB Host: ' . mysql_error());
    }
    //etc..
}


//catch should be performed outside of class:
try {
    $db = new DBConnectionManager ($host, $user, $pass, $db);
    $db->dbConnect(); 
} catch (DBConnectException $e) {
    //do something specific for DBConnectException
    logToFile($e->getMessage());
} catch (Exception $e) {
    //Unknown error.. 
}

?>
Last edited by Jenk on Wed Jan 18, 2006 5:42 pm, edited 1 time in total.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

I'd question the practical separation of the DBConnectionManager and SQLHelper classes. I agree that if you want to do this that composition would be the better way. Typically these two are one class, such as:

Code: Select all

class DBConnection {
function connect()
function query()
function commit()
function rollback()
function lastInsertID()
function isError()
}
And query returns some kind of result object that can be a RecordSet, TableRowGateway or ActiveRecord. For example:

Code: Select all

class DBRecordSet {
function fetchAssoc()
function fetchObject()
function numRows()
function isError()
}
(#10850)
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

Jenk wrote: Creating an instance within the class is not always the best idea.. Extend is there for a reason after all.
I think people are now tend to extend every single class where they feel that they require member variables of those classes for processing.

I think that you can extend a class if there exists a strong logical relationship between two classes. Like, you have an abstract class, DB and extend it to make subclasses specific to Mysql, Oracle, Ingres, etc.,
Post Reply