PHP Critiuqe

Soldato
Joined
18 Apr 2004
Posts
2,613
Location
London
Could you let me know if there are any improvments that can be made on this code that ive written...

PHP:
<?php

/*

		+-------------------------------------------------+
		| USER MANAGER CLASS                              |
		|  by Tom Lynch                                   |
		+-------------------------------------------------+
		| This class will allow you to easily manage a    |
		| user base.                                      |
		+-------------------------------------------------+
		| MYSQL                                           |
		| =====                                           |
		| CREATE TABLE users (                            |
		| user_id INT NOT NULL AUTO_INCREMENT PRIMARY KEY,|
		| username VARCHAR(255) NOT NULL,                 |
		| password VARCHAR(255) NOT NULL,                 |
		| email VARCHAR(255) NOT NULL                     |
		| ) ENGINE = MYISAM;                              |
		|                                                 |
		| FUNCTIONS                                       |
		| =========                                       |
		| set_db_details - sets database details          |
		| db_connect - used internally to connect to db   |
		| login_user - logs in the user                   |
		| check_user - verifies username                  |
		| logout_user - logs out the user                 |
		| user_id - sets the user id to be used           |
		| user_name - sets the username to be used        |
		| user_details - sets the user details to be used |
		| add_user - adds user                            |
		| update_user - updates user details              |
		| remove_user - removes user                      |
		| get_user - gets user details                    |
		| list_users - gets a list of users               |
		| last_user_if - gets last user id                |
		| get_user_id_from_username - gets user id        |
		| get_user_id_from_email - gets user id           |
		|                                                 |
		| HOW TO: INITIALIZE CLASS                        |
		| =======================                         |
		| include_once("user.php");                       |
		| $manager = new usermanager;                     |
		| $manager->set_db_details($host, $username,      |
		| $password, $database, $table);                  |
		|                                                 |
		| HOW TO: CALL A FUNCTION                         |
		| =======================                         |
		| $output = $manager->function(var1, var2);       |
		|                                                 |
		| HOW TO: CHECK FOR ERRORS                        |
		| ========================                        |
		| if ($manager->error) {                          |
		|	echo "Error: " . $manager->error;             |
		| };                                              |
		+-------------------------------------------------+

*/

class usermanager {

	// Specify variables
	var $db_host;				// Holds database hostname
	var $db_username;		// Holds database username
	var $db_password;		// Holds database password
	var $db_database;		// Holds database name
	var $db_user_table; // Holds user table name
	var $user_id;				// Holds user id
	var $user_name;			// Holds username
	var $user_password;	// Holds password
	var $user_email;		// Holds email address
	var $error;					// Holds error messages

	// Set database details
	function set_db_details($host, $username, $password, $database, $table) {

		$this->db_host = $host;
		$this->db_username = $username;
		$this->db_password = $password;
		$this->db_database = $database;
		$this->db_user_table = $table;

	}

	// Connect to database
	function db_connect() {

		mysql_connect($this->db_host, $this->db_username, $this->db_password)
		or $this->error = "The connection to the host failed, please try again";

		mysql_select_db($this->db_database)
		or $this->error = "Unable to select database, please try again";

		return $output;

	}

	// Login User
	function login_user() {

		$this->db_connect();
		$_SESSION['username'] = mysql_real_escape_string($this->user_name);
		$_SESSION['password'] = mysql_real_escape_string($this->user_password);

		session_regenerate_id()
		or $this->error = "There was a script error at login, you may no longer be logged in, please try again";

	}

	// Check to ensure user is valid
	function check_user() {

		$this->db_connect();

		$output = 0;

		$query = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE username = '" . $_SESSION['username'] . "' && password = '" . $_SESSION['password'] . "';")
		or $this->error = "There was an error searching the database, please try again";

		if (mysql_num_rows($query) > 0) $output = 1;

		return $output;

	}

	// Logout User
	function logout_user() {

		session_unset();

	}

	// Get user details
	function user_id($user_id) {
		$this->db_connect();
		$this->user_id = mysql_real_escape_string($user_id);
	}

	function user_name($user_name) {
		$this->db_connect();
		$this->user_name = mysql_real_escape_string($user_name);
	}

	function user_details($user_name, $user_password, $user_email=0) {
		$this->db_connect();
		$this->user_name = mysql_real_escape_string($user_name);
		$this->user_password = md5(mysql_real_escape_string($user_password));
		$this->user_email = mysql_real_escape_string($user_email);
	}

	// Perform administrative functions
	function add_user() {

		$this->db_connect();

		$query_user = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE username = '" . $this->user_name . "';");
		$query_email = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE email = '" . $this->user_email . "';");

		if (mysql_num_rows($query_user) > 0 and mysql_num_rows($query_email) > 0) {
			$this->error = "You must enter a unique username and email address";
		} elseif(mysql_num_rows($query_user) > 0) {
			$this->error = "You must enter a unique username";
		} elseif(mysql_num_rows($query_email) > 0) {
			$this->error = "You must enter a unique email address";
		} else {
			mysql_query("INSERT INTO " . $this->db_user_table . " (username, password, email) VALUES ('" . $this->user_name . "', '" . $this->user_password ."', '" . $this->user_email . "');")
			or $this->error = "There was an error inserting the user into the table, please try again";
		};

		return $output;

	}

	function update_user() {

		$this->db_connect();

		$query_user = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE username = '" . $this->user_name . "';");
		$query_email = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE email = '" . $this->user_email . "';");

		if (mysql_num_rows($query_user) > 0 and mysql_num_rows($query_email) > 0) {
			$this->error = "You must enter a unique username and email address";
		} elseif(mysql_num_rows($query_user) > 0) {
			$this->error = "You must enter a unique username";
		} elseif(mysql_num_rows($query_email) > 0) {
			$this->error = "You must enter a unique email address";
		} else {
			mysql_query("UPDATE " . $this->db_user_table . " SET username = '" . $this->user_name . "', password = '" . $this->user_password ."', email = '" . $this->user_email . "' WHERE user_id = '" . $this->user_id . "';")
			or $this->error = "There was an error updating the user, please try again";
		};

		return $output;

	}

	function remove_user() {

		$this->db_connect();

		mysql_query("DELETE FROM " . $this->db_user_table . " WHERE user_id = '" . $this->user_id . "';")
		or $this->error = "There was an error removing the user from the database, please try again";

		return $output;

	}

	function get_user() {

		$this->db_connect();

		$query = mysql_query("SELECT * FROM " . $this->db_user_table . " WHERE user_id = '" . $this->user_id . "';")
		or $this->error = "There was an error searching the database, please try again";

		$data = mysql_fetch_array($query) or
		$output = $this->error = "There was an error fetching the data from the database, please try again";

		$output['id'] = $data['user_id'];
		$output['username'] = $data['username'];
		$output['email'] = $data['email'];

		return $output;

	}

	function list_users() {

		$this->db_connect();

		$query = mysql_query("SELECT user_id, username, email FROM " . $this->db_user_table . ";")
		or $this->error = "There was an error searching the database, please try again";

		$count = 0;

		while ($count != mysql_num_rows($query)) {
			$output[$count] = mysql_fetch_array($query);
			$count += 1;
		}

		return $output;

	}

	function last_user_id() {

		$this->db_connect();

		$output = mysql_insert_id()
		or $this->error = "There was an error retriving the previous user id, please try again";

		return $output;

	}

	// Database searching
	function get_user_id_from_username() {

		$this->db_connect();

		$query = mysql_query("SELECT user_id FROM " . $this->db_user_table . " WHERE username = '" . $this->user_name . "';")
		or $this->error = "There was an error searching the database, please try again";


		$data = mysql_fetch_row($query);

		$output = $data[0];

		return $output;

	}

	function get_user_id_from_email() {

		$this->db_connect();

		$query = mysql_query("SELECT user_id FROM " . $this->db_user_table . " WHERE email = '" . $this->user_email . "';")
		or $this->error = "There was an error searching the database, please try again";

		$data = mysql_fetch_row($query);

		$output = $data[0];

		return $output;

	}

}

?>
 
Associate
Joined
24 Jun 2005
Posts
249
Break the code out into different Classes. I dont understand why you have all the database code in with the functionality?

Perhaps:

- Functionality
- Security
- Database
 
Caporegime
Joined
18 Oct 2002
Posts
29,490
Location
Back in East London
As above, seperate the logic from the data layer.. though I'm not quite sure where 'security' comes into it as I don't see any there..

Code:
<?php

class UserManager
{
    //vars'
    var $db;

    function UserManager ()
    {
        $this->db = new DataBaseWrapper();
    }
}

?>

The use the db object internally to the UserManager object.
 
Last edited:
Associate
Joined
24 Jun 2005
Posts
249
This is an interesting post really and something which even though I provided suggestions that I dont fully understand.

I havent yet read something about object orientation to this effect but what is the best way of breaking a system down into classes in regards to a full database system. For example if you take an address book; do you just have 3 classes?

- User Interface
- Address
- Database

Does anyone know any good articles on this?
 
Associate
Joined
18 Oct 2002
Posts
1,044
Just a quick note, I can't see any mention of hashing user passwords (my php sucks though); you should never store passwords a plain text in any data system.

akakjs
 
Associate
Joined
24 Jun 2005
Posts
249
PHP:
function user_details($user_name, $user_password, $user_email=0) { 
        $this->db_connect(); 
        $this->user_name = mysql_real_escape_string($user_name); 
        $this->user_password = md5(mysql_real_escape_string($user_password)); 
        $this->user_email = mysql_real_escape_string($user_email); 
    }

Hashing like md5?
 
Caporegime
Joined
18 Oct 2002
Posts
29,490
Location
Back in East London
Need to look up on Patterns, don't expect to get it instantly, it's something that is forever evolving and has been since the 1950's (The "Patterns" concept was originally designed by Architects, then later adopted for use in prgramming)

Wiki
 
Soldato
Joined
26 Dec 2003
Posts
16,522
Location
London
robc123 said:
This is an interesting post really and something which even though I provided suggestions that I dont fully understand.

I havent yet read something about object orientation to this effect but what is the best way of breaking a system down into classes in regards to a full database system. For example if you take an address book; do you just have 3 classes?

- User Interface
- Address
- Database

Does anyone know any good articles on this?


Most languages have frameworks that help you break things down. MVC is the most popular, which breaks things into the Model, View and Controller layers for easy separation of business, presentation and event logic. See here for more details.

A good example of this separation would be using something like Smarty and then writing an API for your business logic. So you'd have a page that ended up looking like this in its most simplest form:

Code:
$id = intval($_GET['id']);

$post = $post::from_id($id);

$smarty->assign('post', $post);
$smarty->display('post.tpl');

The business logic is separate, in the $post class, and the view logic is separate still, in the Smarty template which we're parsing.
 
Associate
Joined
24 Jun 2005
Posts
249
I know what you mean though about the hashing, I expected there to be more as well. I imagine most of it must be done before passing it to the class
 
Soldato
OP
Joined
18 Apr 2004
Posts
2,613
Location
London
robc123 said:
I know what you mean though about the hashing, I expected there to be more as well. I imagine most of it must be done before passing it to the class

How do you mean? if used correctly the only time u would pass a password into the flass is through this function so it will always be md5'd how much more do you need?
 
Back
Top Bottom