Check my Code

ibnclaudius · December 9, 2011

I am new to PHP.

I developed this class, I wonder if there's anything wrong or that I can improve. I could not test it because I'm in school.

Thanks in advance.

<?



class user {



	var $userID,

		$schoolID,

		$userName,

		$userPass,

		$dbHost,

		$dbUser,

		$dbName,

		$dbPass,

		$dbUserTable;

		$dbSchoolTable;



	function dbInfo() {

		$this->dbHost = 'localhost';

		$this->dbUser = '';

		$this->dbName = '';

		$this->dbPass = '';

		$this->dbUserTable = '';

		$this->dbSchoolTable = '';

	}



	function registerUser($userName, $userPass) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "INSERT INTO $this->dbUserTable VALUES (NULL, \"$userName\", \"$userPass\")";

		$result = mysql_query($query);



		if(!$result) {
			echo "Fail.";
		} else {
			$this->userID = mysql_insert_id();
		}



		mysql_close($dbLink);



		$this->userName = $userName;

		$this->userPass = $userPass;

	}



	function registerSchool($schoolName) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "INSERT INTO $this->dbSchoolTable VALUES (NULL, \"$schoolName\")";

		$result = mysql_query($query);



		if(!$result) {
			echo "Fail.";
		} else {
			$this->schoolID = mysql_insert_id();
		}



		mysql_close($dbLink);



		$this->schoolName = $schoolName;

	}


	function userLogin() {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "SELECT * FROM $this->dbUserTable WHERE userName = \"$this->userName\" AND userPass = \"$this->userPass\" LIMIT 1";

		$result = mysql_query($query);



		if(!$result) {
			echo "Fail.";
		} else {
			while($row = mysql_fetch_array($result)) {
				session_start();
				$_SESSION['userID'] = $row['userID'];
				session_write_close();
			}
		}



		mysql_close($dbLink);

	}


	function changePass($newPass) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);

		$query = "SELECT * FROM $this->dbUserTable WHERE userName = \"$this->userName\" LIMIT 1";

		$result = mysql_query($query);

		if(!$result) {
			echo "Fail.";
		} else {

			$query = "UPDATE $this->dbUserTable SET userPass = \"$newPass\" WHERE userName = \"$this->userName\"";

			$result = mysql_query($query);



			if(!$result) {
				echo "Fail";
			} else {			
				$this->userPass = $newPass;
			}
		}



		mysql_close($dbLink);		

	}



}

?>

trq · December 9, 2011

There are numerous things design wise that are wrong with this class.

Firstly, it is tightly coupled with the mysql extension. Instead, you should be passing a database object (implementing an interface that this class recognises) into this object.

Secondly, there are several places in the class that output error messages within this class. Classes should not output anything (unless that is what they are designed to do) but instead throw exceptions or have methods simply return false.

Other than that, it's a pretty good start. I would however recommend using the more common php5 syntax instead of the php4 syntax.

ibnclaudius · December 9, 2011

I'm newbie in PHP and english is not my native language, this makes it a little harder

Could you give me an example of what you said?

Firstly, it is tightly coupled with the mysql extension. Instead, you should be passing a database object (implementing an interface that this class recognises) into this object.

Secondly, there are several places in the class that output error messages within this class. Classes should not output anything (unless that is what they are designed to do) but instead throw exceptions or have methods simply return false.

Other than that, it's a pretty good start. I would however recommend using the more common php5 syntax instead of the php4 syntax.

I made some changes, check it!

<?php



class network {



	public $userID;

	public $schoolID;

	public $userEnrollment;

	public $userName;

	public $userPass;

	public $dbHost;

	public $dbUser;

	public $dbName;

	public $dbPass;

	public $dbUserTable;

	public $dbSchoolTable;



	function dbInfo() {

		$this->dbHost = 'localhost';

		$this->dbUser = '';

		$this->dbPass = '';

		$this->dbName = '';

		$this->dbUserTable = '';

		$this->dbSchoolTable = '';

	}



	function registerUser($userEnrollment, $userName, $userPass) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "INSERT INTO $this->dbUserTable VALUES (NULL, \"$userEnrollment\", \"$userName\", \"$userPass\")";

		$result = mysql_query($query);



		if(!$result) {

			echo "Fail.";

		} else {

			$this->userID = mysql_insert_id();

		}



		mysql_close($dbLink);



		$this->userName = $userName;

		$this->userPass = $userPass;

	}



	function registerSchool($schoolName) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "INSERT INTO $this->dbSchoolTable VALUES (NULL, \"$schoolName\")";

		$result = mysql_query($query);



		if(!$result) {

			echo "Fail.";

		} else {

			$this->schoolID = mysql_insert_id();

		}



		mysql_close($dbLink);



		$this->schoolName = $schoolName;

	}



	function userLogin() {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";

		$result = mysql_query($query);



		if(!$result) {

			echo "Fail.";

		} else {

			$row = mysql_fetch_array($result);

			session_regenerate_id();

				$_SESSION['userEnrollment'] = $this->userEnrollment;

			session_write_close();

		}



		mysql_close($dbLink);

	}



	function changePass($newPass) {

		$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);

		if(!$dbLink) die("Could not connect to database: " . mysql_error());



		mysql_select_db($this->dbName);



		$query = "SELECT * FROM $this->dbUserTable WHERE userName = \"$this->userName\" LIMIT 1";

		$result = mysql_query($query);



		if(!$result) {

			echo "Fail.";

		} else {

			$query = "UPDATE $this->dbUserTable SET userPass = \"$newPass\" WHERE userName = \"$this->userName\"";

			$result = mysql_query($query);



			if(!$result) {

				echo "Fail";

			} else {			

				$this->userPass = $newPass;

			}

		}



		mysql_close($dbLink);		

	}



}

?>

Thank you for your help!!!

ibnclaudius · December 10, 2011

A few more changes, check it.

<?php



class network {

public $userID;

public $schoolID;

public $userEnrollment;

public $userName;

public $dbUserTable;

public $dbSchoolTable;



protected $sql;



public function __construct($dbHost, $dbUser, $dbPass, $dbName)

{

	$dsn = "mysql:host={$dbHost};dbname={$dbName}";

	try

	{

		$this->sql = new PDO($dsn, $dbUser, $dbPass);

	}

	catch (PDOException $e)

	{

		throw new Exceptopn($e->getMessage());

	}



	$this->dbUserTable   = $dbUserTable;

	$this->dbSchoolTable = $dbSchoolTable;

}



public function registerUser($userEnrollment, $userName, $userPass)

{

	$this->userName = $userName;

	$hashedPass = $this->hashPassword($userPass);



	$query = "INSERT INTO {$this->dbUserTable} VALUES (NULL, :enrollment, :username, :password)";

	$stmt  = $this->sql->prepare($query);



	$result = $stmt->execute(array(':enrollment' => $userEnrollment,

                                       ':username'   => $userName,

                                       ':password'   => $hashedPass));



	return ($result === TRUE) ? $this->sql->lastInsertId() : FALSE;

}



public function registerSchool($schoolName)

{

	$this->schoolName = $schoolName;



	$query = "INSERT INTO {$this->dbSchoolTable} VALUES (NULL, :schoolName)";

	$stmt  = $this->sql->prepare($query);



	$result = $stmt->execute(array(':schoolName' => $schoolName));



	return ($result === TRUE) ? $this->sql->lastInsertId() : FALSE;

}



public function userLogin()

{

	$dbLink = mysql_connect($this->dbHost, $this->dbUser, $this->dbPass);



	if (!$dbLink) die("Could not connect to database: " . mysql_error());



	mysql_select_db($this->dbName);



	$query = "SELECT * FROM $this->dbUserTable WHERE userEnrollment = \"$this->userEnrollment\" AND userPass = \"$this->userPass\" LIMIT 1";



	$result = mysql_query($query);



	if (!$result)

	{

		echo "Fail.";

	}

	else

	{

		$row = mysql_fetch_array($result);



		session_regenerate_id();

			$_SESSION['userEnrollment'] = $this->userEnrollment;

		session_write_close();

	}



	mysql_close($dbLink);

}



public function changePass($newPass)

{

	$query = "SELECT COUNT(*)

		FROM {$this->dbUserTable}

		WHERE userName = :username

		LIMIT 1";


	$stmt  = $this->sql->prepare($query);



	$result = $stmt->execute(array(':username' => $this->userName));



	if (!$result)

	{

		throw new Exception('User does not exist.');

	}

	else

	{

		$hashedPass = $this->hashPassword($newPass);



		$query = "UPDATE {$this->dbUserTable}

			SET userPass = :password

			WHERE userName = :username";


		$stmt  = $this->sql->prepare($query);



		$result = $stmt->execute(array(':password' => $hashedPass,

						':username' => $this->userName));



		return ($result === TRUE) ? TRUE : FALSE;

	}

}



private function hashPassword($password)

{

	$salt   = "This shouldn't really be hard-coded into the function";

	$hashed = crypt($password, '$2a$12$' . substr(md5($salt), 0, 22));



	return $hashed;

}

}



?>

What should I change to the code have a better "functioning"?

I'm not sure about how to call this class and function. How should I make a login form?

requinix · December 10, 2011

Firstly, it is tightly coupled with the mysql extension. Instead, you should be passing a database object (implementing an interface that this class recognises) into this object.

I actually avoid that, if I may guess at what exactly you mean. If I were to go down that road then I'd want an interface that lets me build queries with methods like select() and where() and join() and such. Otherwise I'll be hard-coding SQL queries, and since all the flavors are different I'd stay away from the good pattern just so that it's more likely to be a problem in the future. Yes, I know it's weird, but if the problem is to arise I'd want it to sooner rather than later.

trq · December 10, 2011

Firstly, it is tightly coupled with the mysql extension. Instead, you should be passing a database object (implementing an interface that this class recognises) into this object.

I actually avoid that, if I may guess at what exactly you mean. If I were to go down that road then I'd want an interface that lets me build queries with methods like select() and where() and join() and such. Otherwise I'll be hard-coding SQL queries, and since all the flavors are different I'd stay away from the good pattern just so that it's more likely to be a problem in the future. Yes, I know it's weird, but if the problem is to arise I'd want it to sooner rather than later.

Yeah, I completely agree, but didn't want really to dump all that on someone just starting out as well. I guess I was just trying at least point out that you should try avoid having your objects depend on hard coded extensions and that instead you should try and have them rely on an interface.

If I had more time I'd like to create an example but really, even for a Saturday, I'm flat out.

Sign In

Check my Code

Recommended Posts

ibnclaudius

Link to comment

Share on other sites

trq

Link to comment

Share on other sites

ibnclaudius

Link to comment

Share on other sites

ibnclaudius

Link to comment

Share on other sites

requinix

Link to comment

Share on other sites

trq

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information