My First Class

[SetToLoki]

I have created a class to manage users in a database. It's the first class I have ever made, now it all works or at least seems to but as it is the first time I have tried anything like this would like some gurus to give it a once over and point out if they can see any flaws mistakes or better ways to do things.

 

here's my class

<?php
class users{

	private $mysql;
	private $pass;
	public $id;

	public $user;
	public $userlevel;
	public $error = "";

	function __construct($db_name, $db_user, $db_pass, $db_table){
		$this->mysql = new mysqli($db_name, $db_user, $db_pass, $db_table);	
	}

	function SetUser($user){
		$this->user = $user;
		return $this;
	}

	function SetPass($pass){
		$pass = sha1($pass);
		$this->pass = $pass;
		return $this;
	}

	function SetUserLevel($userlevel){
		$this->userlevel = $userlevel;
		return $this;
	}

	function SetId($id){
		$this->id = $id;
		return $this;
	}

	function AddUser(){		
		if(!$this->CheckUser()){
			$username = $this->mysql->real_escape_string($this->user);
			$password = $this->mysql->real_escape_string($this->pass);
			$userlevel = $this->mysql->real_escape_string($this->userlevel);
			$sql = "INSERT INTO users (username, password, userlevel) VALUES ('$username', '$password', '$userlevel')";
			if(!$this->mysql->query($sql)){
				$this->mysql->free();
				return true;
			} else {
				return false;
				$this->error .= "Failed to Add User to Database. <br />";
				$this->error .= $this->mysql->error."<br />";
			}
		} else {
			return false;
			$this->error .= "User All Ready Exists. <br />";
		}
	}

	function EditUser(){
		$username = $this->mysql->real_escape_string($this->user);
		$password = $this->mysql->real_escape_string($this->pass);
		$userlevel = $this->mysql->real_escape_string($this->userlevel);
		$id = $this->id;
		if(!$this->CheckUser()){
			$sql = "INSERT INTO users (username, password, userlevel) VALUES ('$username', '$password', '$userlevel')";
		} else {
			$sql = "UPDATE users SET username = '$username', password = '$password', userlevel = '$userlevel' WHERE id = '$id'";
		}
		if(!$this->mysql->query($sql)){
			$this->mysql->free();
			return true;
		} else {
			return false;
			$this->error .= "Failed to Edit User. <br />";
			$this->error .= $this->mysql->error."<br />";
		}
	}

	function CheckUser(){
		$username = $this->mysql->real_escape_string($this->user);
		$password = $this->mysql->real_escape_string($this->pass);
		$sql = "SELECT id FROM users WHERE username='$username' AND password='$password' LIMIT 1";
		$result = $this->mysql->query($sql);
		$row_cnt = $result->num_rows;
		$result->free();
		if($row_cnt > 0){
			return true;
		} else {
			return false;
		}	
	}

	function DeleteUser(){
		$id = $this->id;
		$sql = "DELETE FROM users WHERE id = '$id'";
		$result = $this->mysql->query($sql);
		if($result){
			return true;
			$result->free();
		} else {
			$this->error .= "Failed to Delete User. <br />";
			$this->error .= $this->mysql->error."<br />";
			return false;
		}
	}

	function CloseMysql(){
		$this->mysql->close();
	}
}
?>

and usage

<?php
require_once('php/users.php');
$user = new users($db_host, $db_user, $db_pass, $db_name);
$user->SetUser('bobo')->SetPass('test')->AddUser();
echo $user->error;
if($user->CheckUser()){
	echo "User Exists";
}else{
	echo "User Doesn't Exist";
}

?>

 

thanks in advance for any input

[Alex]

I find it's always good to keep things are simple as possible. The less lines the better, so although it doesn't really matter, it's probably a good idea to change something like:

 

function SetPass($pass){
		$pass = sha1($pass);
		$this->pass = $pass;
		return $this;
	}

to:

function SetPass($pass){
		$this->pass = sha1($pass);
		return $this;
	}

 

It's not going to make a performance difference or anything, it's just more organized.

[KevinM1]

A good rule of thumb is to keep all properties private and use accessor methods to manipulate them.  Public properties run the risk of being overwritten.

[roopurt18]

		function __construct($db_name, $db_user, $db_pass, $db_table){
		$this->mysql = new mysqli($db_name, $db_user, $db_pass, $db_table);	
	}

 

What happens when you have more classes that access the database?  Are you going to keep instantiating mysqli objects?  Chances are you only need one mysqli object that can be referenced throughout your code.  Consider adding a new class, DatabaseManager or Registry, to create and maintain a single instance of your database connection that other objects / code grabs as necessary.

 

Something like:

$db = Registry::getInstance()->GetDatabase(); // returns mysqli

$db = DatabaseManager::getInstance()->GetDefaultConnection(); // returns mysqli

 

Your class is making assumptions about the presentation layer by having <br /> tags in it.  You'd be better off having an $errors private member that is an array.  Each time you have an error, instead of appending to an error string, push onto the $errors array stack:

if( /* something fails */ ) {

  $this->_errors[] = 'Username is invalid.';

}

 

Then provide a method GetErrors() that returns this array.  Now your functions can return primarily boolean and if the calling code needs more information it can call GetErrors() to display to the user.

 

Moving on, you allow both conventions:

$user->user = 'Joeboo';

$user->SetUser( 'Joeboo' );

 

You should pick one or the other and stick with it to keep your client code consistent.

[roopurt18]

function CloseMysql(){
		$this->mysql->close();
	}

 

You probably do not need that function.  And the code contained within can be moved to the object's destructor.

 

[SetToLoki]

function CloseMysql(){
		$this->mysql->close();
	}

 

You probably do not need that function.  And the code contained within can be moved to the object's destructor.

 

 

Thanks to you all for your advice

 

could you explain a little more about the :: in "Registry::getInstance()" or at least point me to what to search for not sure of its's name or what it does

also  object's destructors are new to me, sound fun though

[roopurt18]

A destructor is the opposite of the constructor.  When an object no longer has any references pointing at it, it is automatically destroyed and the destructor is called.  So the destructor is a good place to handle any necessary clean up.

 

A Registry is a commonly implemented object.  The purpose of the registry is to store references to globally needed values or objects.  It's not really, but you can almost think of it as a "safer" way to implement globals without using the global keyword.

 

<?php
// example of how I use the registry in my application
$cfg = Registry::getInstance()->GetConfig(); // Returns Config object
$db = Registry::getInstance()->GetPrimaryDatabase(); // Returns database interface object (PDO, mysqli, etc)
?>

 

The Registry::getInstance() syntax is used because the registry is a singleton.

 

A singleton is an object that is never instantiated more than once (Sometimes it's useful to guarantee that an object can ever have only one instance).

 

You implement a singleton through a combination of static methods and a private constructor().

 

<?php
// A sample singleton class
// try and run this code
class TheExample {
  // NOTICE THE PRIVATE  CONSTRUCTOR
  private __construct() {
  }
}

// The constructor is private; that means it can not be called from outside the class.
// Since the constructor can not be called, we can not instantiate the class.
$foo = new TheExample(); // This line crashes
?>

 

<?php
class WorkingExample {
  static private $s_instance = null; // Store a handle to the instance

  // This function returns an instance of the object
  static public function getInstance() {
    if( self::$s_instance === null ) {
      // If the above is null, then the object has not yet been instantiated.
      // Since the constructor is private, only methods of the class can instantiate
      // objects.  
      // So we create one and store the instance to it
      echo 'Instantiating...' . PHP_EOL;
      self::$s_instance = new WorkingExample();
    }
    return self::$s_instance;
  }

  private $_name = null;

  private function __construct() {
    echo __METHOD__ . PHP_EOL;
    $this->_name = 'Sam';
  }

  public function GetName() { return $this->_name; }
}

// We need to use the static method to return an instance
$foo = WorkingExample::getInstance();
echo $foo->GetName() . PHP_EOL;
$bar = WorkingExample::getInstance();
echo $bar->GetName() . PHP_EOL;
// $foo and $bar are actually the same instance!  Notice how many times the construct was called
?>

[play_]

Gonna use an opportunity to ask a question.

 

  function SetPass($pass){
         $pass = sha1($pass);
         $this->pass = $pass;
         return $this;
      }

 

Is that right? Can you have return $this? I thought it'd have to be return $this->pass;

 

cause if so, then this is news to me

[roopurt18]

It returns whatever you tell it to.

 

If you tell it to return $this->pass, then it returns the value in the pass member.

echo $someObj->SetPass( 'asdf' ); // Sets pass to 'asdf' and also echos 'asdf' since SetPass() returns its parameter

 

If you tell it to return $this, then it returns a reference to the object.  This allows you to daisy-chain function calls.

$objA->SetPass( 'asdf' );
$objA->SetUser( 'joeboo' );

// or with daisy chaining
$objB->SetPass( 'asdf' )->SetUser( 'joeboo' );

[play_]

Ahh i get it.

thanks.

[SetToLoki]

Ahh i get it.

thanks.

 

yes if you return $this you can then make an object like so

SetUser($user){
$this->user = $user
return $this;
}

SetPass($pass){
$this->pass = $pass;
return $this;
}

SomeFunc(){
echo "{$this->pass} <br /> {$this->user}";
}
//usage
$user->SetUser("name")->SetPass("pass")->SomeFunc();