Members class-OO: Suggestions solicited[Unsolved]

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

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

Members class-OO: Suggestions solicited[Unsolved]

Post by raghavan20 »

[NOTE: Scroll down to the bottom of this page for recent problems]
Hello all,

I have a Members_tbl which has the following fields

Code: Select all

MemberId, UserName, Password, PrivelegeId, FirstName, LastName, Age,			
	 Gender, Email, Phone, Street, City, State, Country, PostCode, JoinDate
This is the first time I am going in for total OO development.
I have developed three classes for the Members_tbl
1. Members
2. MemberCreation
3. MemberRemoval

1. Do you think that I need three classes for that?
2. What do you think of the implementation...is there a better idea available?
3. Can you suggest better code commenting(classes, functions and small code blocks) practices as well?
4. Is it necessary to store even small classes in individual files with the same names as that of respective classes?


Members Class

Code: Select all

class Members{
	/**
	* @member_array contains the following fields from Members_tbl
	* MemberId, UserName, Password, PrivelegeId, FirstName, LastName, Age,			
	* Gender, Email, Phone, Street, City, State, Country, PostCode, JoinDate			
	* @databaseConnection: Database connection handler
	*/
	var $member_array = array();
	var $db_connection;

	/**
	* Constructor of the Members class
	* 1. The member id is received as input and all the field values are retrieved and stored into class variables
	* @member_id: gets the id of the member
	*/
	function Members($member_id, $db_connection){
		$this->db_connection = $db_connection;
		$this->member_array["member_id"] = $member_id;
		set_member_variables();
	}

	/**
	* Sets the values for all the class member variables by retrieving all the values from the database for the member id given
	* 1. Use the member variable $member_id to retrieve values from database and set them for other member variables
	*/
	function set_member_variables(){
		$query = "select * from `Members_tbl` where `MemberId` = {$this->member_id}";
		$statement = $this->db_connection->executeQuery($query);
		if ($statement->findRows() == 1){
			$row = mysql_fetch_assoc($statement->result);
			$this->member_array["UserName"] = $row["UserName"];
			$this->member_array["Password"] = $row["Password"];
			$this->member_array["PrivelegeId"] = $row["PrivelegeId"];
			$this->member_array["FirstName"] = $row["FirstName"];
			$this->member_array["LastName"] = $row["LastName"];
			$this->member_array["Age"] = $row["Age"];
			$this->member_array["Gender"] = $row["Gender"];
			$this->member_array["Email"] = $row["Email"];
			$this->member_array["Phone"] = $row["Phone"];
			$this->member_array["Street"] = $row["Street"];
			$this->member_array["City"] = $row["City"];
			$this->member_array["State"] = $row["State"];
			$this->member_array["Country"] = $row["Country"];
			$this->member_array["PostCode"] = $row["PostCode"];
			$this->member_array["JoinDate"] = $row["JoinDate"];
		}
	}

	/**
	* Used to set new member id when you want to change the user
	* 1. pass the value of the member id and it would be set as the current member id of the object
	* @member_id: The member id of an user
	* @return value: Returns TRUE or FALSE depends on whether the member id has been changed for the current object
	*/	
	function set_member_id($member_id){
		if (is_integer($member_id)){
			$this->member_array["member_id"] = $member_id;
			$this->set_member_variables();
			return TRUE;
		}
	}

	/**
	* Used to store a set of field values into the Members_tbl
	* 1. pass the values to be stored as associative arrays should be in the format: field_name ==> field_value
	* 2. number of elements in the array is counted and looped so many times to extract each field and its value
	* @store_array: the array which holds associative elements
	* @return value: returns TRUE or FALSE depends on success of updation of values
	*/
	function update_values($store_array){
		$keys_array = "";
		$values_array = "";
		$index = 0;
	
		//store the field names in an array
		foreach($store_array as $field){
			$keys_array[$index++] = "`{$field["name"]}`";
		}
		
		//store the fields values in an array
		foreach($store_array as $field){
			if ($field["type"] == "integer"){			
				$values_array[$index++] = $field["value"];
			}else{
				$values_array[$index++] = "'{$field["value"]}'";
			}
		}
		
		$keys_string = implode(",", $keys_array);
		$values_string  = implode(",", $values_array);
		
		//form the query
		$query = "update `Members_tbl` set ";
		//append the set values to the query
		for ($i = 0; $i < count($store_array); $i++){
			if ($i == count($store_array) - 1){
				$query .= "{$keys_array[$i]} = {$values_array[$i]} ";
			}else{
				$query .= "{$keys_array[$i]} = {$values_array[$i]}, ";
			}
		}
		//append the where condition to the SQL query
		$query .= " where `MemberId` = $this->member_id";
		echo $query;
		
		//execute the query
		if($this->db_connection->updateRows()){
			return TRUE
		}else{
			return FALSE;
		}
	}


	/**
	*  1. Used to selected set of fields from the Members_tbl
	*  2. Pass the member id and set of fields to be retrieved as an array
	*  @fields: pass the set of fields to be retrieved as elements of the $fields array
	*  @return value: an associative array which holds elements having field names and values as keys and values pairs
	*/
	function get_selected_values($fields){
		$return_values = array();
		foreach($fields as $key){
			$return_values[$key] = $this->member_array[$key];
		}
		##DEBUG##echo "<br />"; print_r($return_values);
		return $return_values;
	}

}

MemberRemoval Class

Code: Select all

<?php
class MemberRemoval{
	/**
	* Used to delete a member from the Members_tbl from the given member_id
	* 1. Receive the member id and form the delete query to delete the member who has this member id 
	* @member_id	:	The unique id of the member
	*/
	function remove_member($member_id, $db_connection){
		$query = "delete from `Members_tbl` where `MemberId` = $member_id";
		echo "<br />".$query;
		if ($db_connection->updateRows($query)){
			return TRUE;
		}else{
			return FALSE;
		}
	}
}
	
?>
Last edited by raghavan20 on Tue Nov 15, 2005 4:53 am, edited 1 time in total.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

I'd have a parent class (maybe even abstract) for the member object, then have child (extended) classes for the other functionality. I'd also have a seperate class for the data source object, which you can always inherit with your member classes.

You could, if you wanted to, not have the class on a seperate file at all, but it just makes for easier maintenance to have them in their own file.

It can be a good thing to keep child classes in the same file as their parent classes, but depends really on the relationship between them and their size. (Sounds like family councilling...)

For commenting, I like to try and have similar style to the PEAR documentation/coding standards, which can be seen in this example

A few pointers.. instead of having to pass around the $data_connection object, structure your class to establish a data connection, this can be done via class inheritance (like I touched on above with having separate class for data source) then when you call the remove member function, all you have to do is tell it which member it is removing.
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

Thanks Jenk for your reply.
I have got a separate database class

Code: Select all

<?php
class DbMysql{
	var $hostName;
	var $userName;
	var $password;
	var $dbName;
	var $dbh;//database connection handle
	
	function DbMysql($hostName = "", $userName = "", $password = "", $dbName = ""){
		$this->hostName = $hostName;
		$this->userName = $userName;
		$this->password = $password;
		$this->dbName = $dbName;
	}
	
	function connectDb(){
		$this->dbh = mysql_connect ($this->hostName, $this->userName, $this->password) or die("Unable to connect to the mysql server!!!");
		if (is_resource($this->dbh)){
			if (!mysql_select_db($this->dbName, $this->dbh)){
				die ("Unable to select the database!!!");
			}
		}else{
			die ("Unable to establish connection with the database server!!!");
		}
	}
	
	function updateRows($query){
		//echo "insertRow called!!!";
		if (!is_resource($this->dbh)){
			$this->connectDB();//connect to the db if not connected
		}
		mysql_query($query, $this->dbh) or die("unable to update/insert rows");
		if (mysql_affected_rows()>0){
			return TRUE; 
		}else{
			return FALSE;
		}
	}
	
	function executeQuery($query){
		if (!is_resource($this->dbh)){
			$this->connectDB();//connect to the db if not connected
		}
		$stmt = new DbMysqlStatement($this->dbh, $query);
		return $stmt;//return an object of DbMysqlStatement
	}
}


class DbMysqlStatement{
	var $query;
	var $result;
	var $dbh;
	
	function DbMysqlStatement($dbh, $query, $result = ""){
		//echo "dbmysqlstatement  constructor called!!!";
		$this->dbh = $dbh;
		$this->query = $query;
		if (!is_resource($this->result)){
			$this->computeResult();//compute result if not computed before			
		}
	}
	
	function computeResult(){
		//echo "computeresult called!!!";
		if (is_resource($this->dbh)){
			$this->result = mysql_query($this->query, $this->dbh);
		}
	}
	
	function findRows(){
		if (is_resource($this->dbh) && is_resource($this->result)){
			return @mysql_num_rows($this->result);
		}
	}
	
	function fetchRow(){
		//echo "dbmysqlstatement  fetchrow called!!!";
		if (is_resource($this->result)){
			return mysql_fetch_row($this->result);//return a row
		}
	}
	
	function fetchAssoc(){
		if (is_resource($this->result)){
			return mysql_fetch_row($this->result);//return an associative array
		}
	}
	
}
?>
Usually what I am thinking of doing is,,, for every file not a class file create a new Database Connection object from the above class and pass the same to all other classes used in the same file.
Advantages as I perceive:
1. Not too many connections established
2. Complexity: need to administer individual connection objects if created in every class file; should delete connection objects during the call of destructors
3. Creating one database connection object in each file would make us delete the only one data connection object created at the end of each file
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Really it entirely depends upon your cirumstance, but it might be a good idea to have the databse connection open in the constructor, and then create a destructor to close the connection.

If you are going to be serializing the object, use the 'magic' functions _sleep() and _wake() to close and (re)open the connections.

The class structure I try to aim for, would be like so:

Code: Select all

class DBMySQL
{

    /* db stuff */

}

class Member extends DBMySQL
{

    /* member info and probably 'common' functions, such as add/remove */

}

class Special_Member extends Member
{

  /* this class is for anything ad-hoc, or uncommon functions, perhaps to adjust the members details - basically anything that will not occur everytime you need to access the member info */

}
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

I tend to end with a layered structure like this

Code: Select all

class DB
{
    //Handle all DB queries, new connections etc...
    // Stores the resource an $_SESSION for re-use
}

class Core extends DB
{
    //Repsoitory of all kinds of things that drive the website
    // -- careful not to add too much, refactor if needed
}

class Member extends Core
{
    //Hmmm 
}
I may sometime have additional layers between those, such as a security layer on top of the DB. Generally, anything I add over time will just extend Core... that includes the templating engine. If you're concerned about reoping connections to the database store the connection resource in a session and have you DB class pass it to any queries made during that session.
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

I still don't understand one thing from codes from d11wtq and Jenk.
Why do you have to inherit the DB class instead you can straight away create an object of the DB class in other classes?

Code: Select all

class DB{
}

class something{
var $db_conn;
function something(){
$this->db_conn = new DB();
}
}
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

About using the session to store the database connection object...
This can be done but I am unsure whether this would be safe and I don't want to reveal to someone who can read Session objects if possible.

Code: Select all

class DB(){
}

$_SESSION["database_connection"]= new DB(); //created once and used everywhere

//first class
class one{
var $db_conn = $_SESSION["database_connection"];
}

//second class
class second{
var $db_conn = $_SESSION["database_connection"];
}
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

raghavan20 wrote:I still don't understand one thing from codes from d11wtq and Jenk.
Why do you have to inherit the DB class instead you can straight away create an object of the DB class in other classes?

Code: Select all

class DB{
}

class something{
var $db_conn;
function something(){
$this->db_conn = new DB();
}
}
It's a matter of preference, really.

from having

Code: Select all

class DB
{
	function query($sql) {
		//etc.
	}
}

class myClass extends DB
{
    function doSomethingWithDB ($arg)
    {
        $this->query("SELECT * FROM table WHERE arg='{$arg}'");
	}
}
vs..

Code: Select all

class DB
{
	function query($sql) {
		//etc.
	}
}

class myClass
{
	var $db = new DB();
	
    function doSomethingWithDB ($arg)
    {
        $this->db->query("SELECT * FROM table WHERE arg='{$arg}'");
	}
}
It also means the difference between having an object contain an object, or to just have one object.
Last edited by Jenk on Thu Nov 03, 2005 4:28 am, edited 1 time in total.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

I make an initial connection in the DB constructor, put it in the session and then just use the inherited methods in all of the other classes. It just saves needlessly recreating the object. If you want to make "new" connection there's nothing to stop you instantiating DB from inside another object ;)

Just preference, exactly -- but since it's the bottom layer (not always in my case) it just feels right to me to use inheritance like that.
User avatar
BDKR
DevNet Resident
Posts: 1207
Joined: Sat Jun 08, 2002 1:24 pm
Location: Florida
Contact:

Post by BDKR »

Well, I don't see the need to seperate all of this into three classes. The functionality of all is close enough and related enough that there is no need to specify further. With all of the functionality in question in one class, it's not a big class. Furthermore, it's just more complexity and overhead (from instantiation and message passing).

As for the db connection, I'd pass it in by reference. Check the changes I made below to the function (method) declaration.

Code: Select all

/** 
    * Constructor of the Members class 
    * 1. The member id is received as input and all the field values are retrieved and stored into class variables 
    * @member_id: gets the id of the member 
    */ 
    // function Members($member_id, $db_connection){ 
    function Members($member_id, &$db_connection){ 
        $this->db_connection = $db_connection; 
        // $this->member_array["member_id"] = $member_id; 
        $this->member_array['member_id'] = $member_id; 
        set_member_variables(); 
    }
The reason being that it kinda goes against a premise or idea of OOP. The idea that one can have a collection of objects as free agents all communicating (message passing) with one another based on need. In order for any of these "agents" to do so, it must first have knowledge of the other agent/agents. Passing an object in by copy has the feeling of destroying that concept. Instead, each member object ends up with it's own copy of a connection object that only it is communicating with. That's not good OOD.

Now this may not be a problem as you are most likely dealing with only one member at a time. However, if you had a situation where there may be multiple instances of member objects, do they really all need there own connection to the data source?

Ideally DB classes as most see them should, in my opinion, be seperated into seperate connection and query classes. Then a factory can distribute multiple query objects to whatever class may need them all the while maintaining only one connection object. This also opens up the possibility of the query class being data source mobile. In other words, if you are working with multiple data sources, a lone query class can use any one of those data sources by simply speaking to a different connection object (I'm presently working on the mechanism to do just this in my DB library and it's just about there).

Another little change in the code above is the use of single instead of double quotes. A small thing really, but many small things together can become one big thing right? Seriously, unless the string needs to be evaluated (the engine goes through it looking for variables to echo or other special characters), single quotes are best. Just another bit of overhead saved. :wink:

Anyway, looks good to me for the most part other then what I mentioned. Sorry for the diatribe. :roll:
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Take a look at the PoEAA catalogue. RowDataGateway or ActiveRecord might be what you're looking for.

I'd strongly recommend getting hold of the book which contains more thorough descriptions of the patterns, when to use them and much other good advice.
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 agree with McGruff (always ;)) that checking out something like the PoEAA or like book would be a good thing to do.

I think maybe TableDataGateway might be better suited, based on your example. I would probably just have a single Member class. Try to limit inheritance if you can. Just pass the DB Connector object into the Member constructor.
Jeroen Oosterlaar
Forum Commoner
Posts: 37
Joined: Sun Nov 06, 2005 4:12 pm

Post by Jeroen Oosterlaar »

Personally, with respect to a full OO design, I would use two classes instead of three. The member creation and removal functionality basically belong to the same set of instruction. Namely, processing datasource transactions. Therefore, I would include all such functionality within a single class, that you could call MemberFactory for example, or MemberDAO (Member Data Access Object) etc. The second class would be the representation of the member entity. In other words: the class Member would simply and only describe a member by some properties (variables).
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

Thanks for all your suggestions.
I am now passing the reference of the database connection object to all classes' objects created in the same file and destroying the only database connection object at the end of the file. That works well.

Now, I have another design problem,,,
I have an entirely different Members class now which I did not post here since it's so long...

1. Usually what I am doing is, getting the MemberId from the user in the Members class and retrieving all the other attributes from the database table and assigning them to an array.
Here is what i am doing now

Code: Select all

function Members($db_connection){
		##DEBUG##echo "<br />"."Received member id:".$member_id;
		$this->db_connection = $db_connection;
		##DEBUG##echo "<br />Member profile:";  print_r($this->member_array);
	}

	/**
	* Sets the values for all the class member variables by retrieving all the values from the database for the member id given
	* 1. Use the member variable $member_id to retrieve values from database and set them for other member variables
	*/
	function set_member_variables(){
		$query = "select * from `Members_tbl` where `MemberId` = {$this->member_array["MemberId"]}";
		##DEBUG##echo "<br />"."Member Profile retrieval query:".$query;
		$statement = $this->db_connection->executeQuery($query);
		if ($statement->findRows() == 1){
			$row = mysql_fetch_assoc($statement->result);
			$this->member_array["UserName"] = $row["UserName"];
			$this->member_array["Password"] = $row["Password"];
			$this->member_array["PrivelegeId"] = $row["PrivelegeId"];
			$this->member_array["FirstName"] = $row["FirstName"];
			$this->member_array["LastName"] = $row["LastName"];
			$this->member_array["BirthDate"] = $row["BirthDate"];
			$this->member_array["Gender"] = $row["Gender"];
			$this->member_array["Email"] = $row["Email"];
			$this->member_array["Phone"] = $row["Phone"];
			$this->member_array["Street"] = $row["Street"];
			$this->member_array["City"] = $row["City"];
			$this->member_array["State"] = $row["State"];
			$this->member_array["Country"] = $row["Country"];
			$this->member_array["PostCode"] = $row["PostCode"];
			$this->member_array["JoinDate"] = $row["JoinDate"];
		}
	}

	/**
	* Used to set new member id when you want to change the user
	* 1. pass the value of the member id and it would be set as the current member id of the object
	* @member_id: The member id of an user
	* @return value: Returns TRUE or FALSE depends on whether the member id has been changed for the current object
	*/	
	function set_member_id($member_id){
		if ((int)$member_id > 0){
			$this->member_array["MemberId"] = $member_id;
			$this->set_member_variables();
			return TRUE;
		}
	}
But now, I have a search page in the application, where I have to find the details of the member from his username/last name/etc...not from member id. This has created a problem for me. so then I have to create dynamic sql statements where field names would be different. If field names are different, then the data types would also differ causing problems because we have to wrap field values in '' incase of string and with nothing in case of integer data types. So, I have to do the following...do you think this is worth doing it or a work around is available.

1. Get the search field from the user; The user would pass the field name and the field value
(set_member_id() would now become as set_search_field(field_name, field_value))
2. Find the datatype of the field name from the database table
3. Form a dynamic query
a)for string type, wrap in quotes
b)for integer type, do not wrap
4. Retrieve all values from the Members table
5. Assign the retrieved values to all elements


Or

Do you guys run SQL queries outside Members class to retrieve necessary information from Members_tbl table?



2. In my earlier cases, I used to set all values of the Members_tbl in a member array of the Members class and make it publicly accessible to objects of it defined in other files. For more clarity of what i am talking,,,here i explain with an example.

Code: Select all

class Members{
var $members_array = array
}
function set_members_attributes(){
                              $query = "select * from `Members_tbl` where `MemberId` = {$this->member_array["MemberId"]}";
		##DEBUG##echo "<br />"."Member Profile retrieval query:".$query;
		$statement = $this->db_connection->executeQuery($query);
		if ($statement->findRows() == 1){
			$row = mysql_fetch_assoc($statement->result);
			$this->member_array["UserName"] = $row["UserName"];
			$this->member_array["Password"] = $row["Password"];
			$this->member_array["PrivelegeId"] = $row["PrivelegeId"];
			$this->member_array["FirstName"] = $row["FirstName"];
			$this->member_array["LastName"] = $row["LastName"];
			$this->member_array["BirthDate"] = $row["BirthDate"];
			$this->member_array["Gender"] = $row["Gender"];
			$this->member_array["Email"] = $row["Email"];
			$this->member_array["Phone"] = $row["Phone"];
			$this->member_array["Street"] = $row["Street"];
			$this->member_array["City"] = $row["City"];
			$this->member_array["State"] = $row["State"];
			$this->member_array["Country"] = $row["Country"];
			$this->member_array["PostCode"] = $row["PostCode"];
			$this->member_array["JoinDate"] = $row["JoinDate"];
		}
}
Now the member_array is accessible as

Code: Select all

$member_instance = new Members(&$db_connection);
$member_instance->member_array("FirstName")// and so on....
I know make it publicly accessible is not good in terms of Java but how many of you implement private modifiers in PHP?

Thanks for answering to any of the above questions
Jeroen Oosterlaar
Forum Commoner
Posts: 37
Joined: Sun Nov 06, 2005 4:12 pm

Post by Jeroen Oosterlaar »

Hi raghavan20,

Why are you passing a database connection object to your objects data management objects? As far as I know, that is not necessary. Once you have created a connection, everything that is executed during the request, can execute transactions freely. So when you have a page script that first initializes the database connection and then invokes some method that executes some database transaction, that object doesn't need to have an expliciet reference to the database connection (object). The connection is 'just there'.
Post Reply