code inspection...

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
codeblazer
Forum Newbie
Posts: 1
Joined: Fri Apr 27, 2007 4:26 am

code inspection...

Post by codeblazer »

Sorry,

I need opinion on this code I have written; for example I am uncertain about things like:
1) is it scalable?
2) are there better ways of "analyzing" the problem and "designing" the solution for my problem domain?
3) the decision to require a class to depend on an ADBODB object - is this a bad choice?
4) am i doing enough checks for integrity and state? (checking vars, catching possible invalidities, etc)
5) OR am i totally going about this whole programming thing the wrong way?

Problem domain:
I intend on creating websites where at the minimum a website will have members; a member is described by the class below. In some cases the website can also have a business member which always will extend the Member class.

I want code that is strictly written for my set of requirements - not written as a generalisation. This way I know my code when my site goes live - and it is hopefully as efficient as it can be.

These are my goals - robust and efficient code. Please help me out! thanks...



Code: Select all

_Exceptions.inc.php
<?php

	class DB_trans_not_executed_adodb5 extends Exception
	{
		public function __construct()
		{
			print "dberror - if your browser doesnt support javascript please click <a create href=''>here</a><br>";
		}
	}

?>

_Member.php
<?php
include "_Exceptions.php.inc";


//all database transactions are tried first - class user may hold on to class details
//and refresh/get-new data by loading, during idle time database may lose connection
class Member
{
		private $id;
		private $email_address;
		private $username;
		private $password;
		private $date_join;
		private $dbh;//adodb5 handler

		//-1 id indicates a non-registered member instance
		public function __construct($dbh='NULL', $id='-1')
		{
			$this->id = $id;
			$this->dbh = $dbh;

			//type check for adodb object
			if(preg_match("/^ADODB_/", get_class($this->dbh)))
			{
				Member::load_member($this->id);
			}
			else
			{
				echo "Error occured: 0001 - you have loaded a member with a non-adodb object (_Member.php)";
				die();
			}
		}//constructor


		//method can be used to update with different Member attributes
		public function load_member($id='-1')
		{
			if((int)$id > 0 && is_integer((int)$id) )
			{
				$sql = "select * from member where id=? limit 1";
				try
				{
					$this->dbh->Prepare($sql);
					$row = $this->dbh->GetRow($sql, $id);
					if(!is_array($row))
					{
						throw new DB_trans_not_executed_adodb5;
					}
					else
					{
						$this->id = $row['id'];
						$this->email_address = $row['email_address'];
						$this->username = $row['username'];
						$this->password = $row['password'];
						$this->date_join = $row['date_join'];
					}
				}
				catch (DB_trans_not_executed_adodb5 $e)
				{
					echo"Redirecting";//redirect here...
				}
			}
			else
			{
			}
		}//load_member

		public function return_data($attrb)
		{
			return $this->$attrb;
		}//return_data_array

}



class MemberUse extends Member
{
	private $auth;
	private $member_type;//later incorporate member levels - admin etc.

	public function __construct($email_address='', $username='', $password='')
	{
		$this->email_address = $email_address;
		$this->username = md5($username);
		$this->password = $password;
		MemberUse::set_auth();
	}//constructor

	private function set_auth()
	{
		$sql = "select id, email_address, password, date_join from member where username = {?} limit 1";
		$stmt = $this->dbh->Prepare($sql);
		$row = $this->dbh->GetRow($stmt, array($this->username));

		if (is_array($row))
		{
			if ($row['password'] != $this->password)
			{
				$auth = "bad";
			}
			else
			{
				$auth = "good";
			}
		}
		else
		{
			$auth = "new";
		}
	}

	public function insert_member()
	{
		if((int)$this->id == -1)//confirm no id has been given to constructor, or set
		{
			$vld_rslt = Member::validate_self_data(); //validate result
			if (sizeof($vld_rslt) > 0)//validate function returns an array with field=>valid=>reason array
			{
				print_r($vld_rslt);//later you may wish to return this to caller for display etc
			}
			else
			{
				$sql = "insert into member (email_address, username, password, date_join) values (?,?,?,?)";
				$stmt = $this->dbh->Prepare($sql);
				$this->dbh->Execute($stmt,array($this->email_address, $this->username, md5($this->password), '101010101'));
				return true;
			}
		}
		else//for some reason insert member has been called with an id that may exist
		{
			echo "bad"; //throw new exception here
		}
	}//insert_member


	public function update_email_address($email_address='')
	{
		if($this->email_address != $email_address)
		{
			$rslt = array();
			$email_exists_id = Member::does_email_exist($email_address);
			//do some basic validation - format and duplicate (ignoring those of same id)
			if(Member::check_email($email_address) === false)
			{
				array_push($rslt, array ("email"  => array("valid" => "false", "reason" => "bad_format")));
				return $rslt;
			}
			else if (is_integer($email_exists_id) && $email_exists_id != $this->id)
			{
				array_push($rslt, array ("email"  => array("valid" => "false", "reason" => "duplicate")));
				return $rslt;
			}
			else
			{
				$sql = "update member set email_address = ? where id = '{$this->id}'";
				$stmt = $this->dbh->Prepare($sql);
				$this->dbh->Execute($stmt,array($email_address));
				$this->email_address = $email_address;
				return true;
			}
		}
		else
		{
			return false;
		}
	}//update_email_address


	public function update_password($password='')
	{
		$rslt = array();

		if ($this->password != md5($password))
		{
			if(Member::check_password($password) === false)
			{
				array_push($rslt, array ("password"  => array("valid" => "false", "reason" => "bad_format")));
				return $rslt;
			}
			else
			{
				$sql = "update member set password = ? where id = '{$this->id}'";
				$stmt = $this->dbh->Prepare($sql);
				$this->dbh->Execute($stmt,array(md5($password)));
				$this->password = $password;
				return true;
			}
		}
		else
		{
			return false;
		}
	}//update_password

//***************************************PRIVATE METHODS START*************************************



	private function validate_self_data()
	{
		$rslt = array();

		if(Member::check_username($this->username) === false)
		{
			array_push($rslt, array ("username"  => array("valid" => "false", "reason" => "bad_format")));
		}
		else if ((int)Member::does_username_exist($this->username) > 0)
		{
			array_push($rslt, array ("username"  => array("valid" => "false", "reason" => "duplicate")));
		}

		if(Member::check_email($this->email_address) === false)
		{
			array_push($rslt, array ("email"  => array("valid" => "false", "reason" => "bad_format")));
		}
		else if ((int)Member::does_email_exist($this->email_address) > 0)
		{
			array_push($rslt, array ("email"  => array("valid" => "false", "reason" => "duplicate")));
		}

		if(Member::check_password($this->password) === false)
		{
			array_push($rslt, array ("password"  => array("valid" => "false", "reason" => "bad_format")));
		}

		return $rslt;
	}//validate_self_data


	private function does_username_exist($user='')
	{
		$sql = "select id from member where username=? LIMIT 1";
		$this->dbh->Prepare($sql);
		$row = $this->dbh->Execute($sql, $user);

		if ((int)$row->RecordCount() > 0)
		{
			$row = $this->dbh->GetRow($sql, $user);
			return $row['id'];
		}
		else
		{
			return false;
		}
	}//does_username_exist

	private function does_email_exist($email='')
	{
		$sql = "select id from member where email_address=? LIMIT 1";
		$this->dbh->Prepare($sql);
		$row = $this->dbh->Execute($sql, $email);

		if ((int)$row->RecordCount() > 0)
		{
			$row = $this->dbh->GetRow($sql, $email);
			return $row['id'];
		}
		else
		{
			return false;
		}
	}//does_email_exist

	private function check_username($user='')
	{//later can be modified to incorporate mandatory alpha-numeric usernames etc.
		if(strlen($user) < 
		{
			return false;
		}
		else
		{
			return true;
		}
	}//check_username

	private function check_email($email='')
	{
		$email = trim($email);
		if(preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/",$email))
		{
			return true;
		}
		else
		{
			return false;
		}
	}//check_email

	private function check_password($pass='')
	{//later can be modified to incorporate mandatory alpha-numeric passwords etc.
		if(strlen($pass) < 
		{
			return false;
		}
		else
		{
			return true;
		}
	}//check_password

}


?>
Last edited by codeblazer on Mon Apr 30, 2007 1:14 am, edited 4 times 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 »

:arrow: Moved to Coding Critique
Post Reply