New to OOP, looking for feedback

[franko75]

Hi, I am in the process of making the switch from procedural to OO PHP. I'm just practising by setting up really simple config, session, database and auth classes. Before I end up going too far down the road with dodgy code and develop bad habits, i would be interested to get some feedback on what i've done so far. This code works at the moment, but if there is any way to remove redundancies, improve security etc, then i would love to hear it. I'm also not 100% sure how best to use exceptions and if i am using them in the right way.

 

Thanks in advance for any advice and for anyone who takes the time to read through my code!

 

config.php

class Config {

protected $hostname = 'localhost';
protected $username = 'root';
protected $password = '';
protected $database = 'my_db';

  
public function __get($var)
{
        return $this->$var;
    }

}

 

session.php

class Session {


  /**
   * Session::__construct()
   * 
   * @return
   */
public function __construct()
{
	//start the session
	session_start();
}

  /**
   * Session::set()
   * 
   * @param mixed $key
   * @param bool $val
   * @return
   */
public function set($key, $val = FALSE)
{
	// Set the key
	$_SESSION[$key] = $val;
}

  /**
   * Session::get()
   * 
   * @param mixed $name
   * @return
   */
public function get($name)
{
	//if this session variable exists, return the value
	if(isset($_SESSION[$name]))
	{
		return $_SESSION[$name];
	}

	else
	{
		return FALSE;
	}	
}

  /**
   * Session::delete()
   * 
   * @param mixed $key
   * @return
   */
public function delete($key)
{
	// Unset the key
	unset($_SESSION[$key]);
}
}

 

database.php

class Database
{
    private $config = null;
    private $link = null;
    private $db = null;
    
     
    public function __construct(Config $conf)
    {
        $this->set_config($conf)
		->connect()
		->select_db();
    }
    
    public function set_config($conf)
    {
        $this->config = $conf;
        
        return $this;
    }
    
    protected function connect()
    {
        if ( ! ($conf = $this->config))
            throw new Exception('Config Object not found');
            
        $this->link = mysql_connect($conf->hostname, $conf->username, $conf->password);
        
        return $this;
    }
    
    protected function select_db()
    {
        if ( ! ($conf = $this->config))
            throw new Exception('Config Object not found');
            
        if ( ! ($link = $this->link))
            throw new Exception('Connection to server failed');
            
        $this->db = mysql_select_db($conf->database, $link);
    }

    /**
     * Database::__destruct()
     * 
     * @return
     */
    public function __destruct()
    {
        @mysql_close($this->connectlink);
    }

    /**
     * Database::query()
     * 
     * @param mixed $sql
     * @return
     */
    public function query($sql)
    {
        
        if ($sql)
	{
		return mysql_query($sql);
	}
	else 
	{
		 throw new Exception("Error - invalid query supplied ".mysql_error());
	}
    }

    /**
     * Database::fetch_rows()
     * 
     * @param mixed $result
     * @return
     */
    public function fetch_rows($result)
    {
        $rows = array();
        if ($result)
        {
            while ($row = mysql_fetch_array($result))
            {
                $rows[] = $row;
            }
            return $rows;
        } 
	else
        {
             throw new Exception("Error retrieving records - invalid query supplied".mysql_error());
        }
    }
    
    /**
     * Database::fetch_rows()
     * 
     * @param mixed $result
     * @return
     */
    public function fetch_array($result)
    {
        
        if ($result)
        {
           $row = mysql_fetch_array($result);
            return $row;
           
        } 
	else
        {
            throw new Exception("Error retrieving records - invalid query supplied".mysql_error());
        }
       
    }
    
    public function count_rows($result)
    {
	if ($result)
	{
		return mysql_num_rows($result);
	}
	else
	{
		throw new Exception("Error retrieving number of rows - invalid query supplied");
	}
}
    
    public function get_db()
    {
	return $this->_db;
}

}

auth.php

class Auth {

protected $db;
protected $session;

  /**
   * Auth::__construct()
   * 
   * @return
   */
public function __construct($db = null)
{
	$this->db = $db;
}


//function to check login credentials
  /**
   * Auth::login()
   * 
   * @param mixed $username
   * @param mixed $password
   * @return
   */
public function login($username, $password)
{
	$query = $this->db->query("SELECT username, password FROM users WHERE username = '".mysql_real_escape_string($username)."' AND password = sha1('".mysql_real_escape_string($password)."')");
	$result = $this->db->fetch_rows($query);

	if (count($result) == 0)
	{
		return FALSE;
	}
	else
	{
		//set session variable 
		$this->session = new Session;
		$this->session->set('logged_in', TRUE);
		return TRUE;
	}
}

//logout function
  /**
   * Auth::logout()
   * 
   * @return
   */
public function logout()
{
	return $_SESSION['logged_in'] = FALSE;
}

}

[cooldude832]

I wouldn't make the vars protected for the config as you should be able to load it via some other data (text file user input mysql etc.)

 

You need to think about a var's type in the sense of how to use it not its security

[franko75]

Thanks for the reply cooldude832. Think I understand what you mean, instead of hard coding the values in the config class, maybe set them in a config file which i include and then i can assign the config data to the class variables?

[premiso]

Thanks for the reply cooldude832. Think I understand what you mean, instead of hard coding the values in the config class, maybe set them in a config file which i include and then i can assign the config data to the class variables?

 

In the constructor of the class, add a parameter for either each value needed or for an array of all values. Then after you are done using those values set the parameters to null, just in case.

[franko75]

Thanks premiso, i'll update my constructor as you suggest.

 

Any feedback on how i'm using exceptions as this is one of the main areas i'm not sure about?