Very basic OOP 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, database and auth classes. Before I end up going too far down the road with dodgy code, i would be interested to get some feedback on what i've done so far.

One thing I'm not sure about - where to start sessions, i.e in the auth class file in the constructor or outside of the class (but in the same file...if that makes sense).

Thanks in advance for any advice!

config.php

class Config {
    
    protected $hostname = 'localhost';
    protected $username = 'root';
    protected $password = '';
    protected $database = 'my_database';
    
  /**
   * Config::get_hostname()
   * 
   * @return
   */
    public function get_hostname()
    {
        return $this->hostname;
    }
    
  /**
   * Config::get_username()
   * 
   * @return
   */
    public function get_username()
    {
        return $this->username;
    }
    
  /**
   * Config::get_password()
   * 
   * @return
   */
    public function get_password()
    {
        return $this->password;
    }
    
  /**
   * Config::get_database()
   * 
   * @return
   */
    public function get_database()
    {
        return $this->database;
    }
} 

database.php

class Database
{
    private $connectlink; //Database Connection Link
    private $username;
    private $password;
    private $database;
    private $hostname;
    private $resultlink; //Database Result Recordset link
    private $rows; //Stores the rows for the resultset

  /**
   * Database::__construct()
   * 
   * @param mixed $conf
   * @return
   */
    public function __construct(Config $conf)
    {
        $this−>hostname = $conf->get_hostname();
        $this−>username = $conf->get_username();
        $this−>password = $conf->get_password();
        $this−>database = $conf->get_database();
        
        $this->connectlink = mysql_connect($this->hostname, $this->username, $this->
            password);
        if ( ! ($this->connectlink))
        {
            throw new Exception("Error Connecting to the Database");
        } else
        {
            mysql_select_db($this->database);
        }
    }

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

  /**
   * Database::query()
   * 
   * @param mixed $sql
   * @return
   */
    public function query($sql)
    {
        $this->resultlink = mysql_query($sql);
        return $this->resultlink;
    }

  /**
   * Database::fetch_rows()
   * 
   * @param mixed $result
   * @return
   */
    public function fetch_rows($result)
    {
        $rows = array();
        if ($result)
        {
            while ($row = mysql_fetch_array($result))
            {
                $rows[] = $row;
            }
        } else
        {
            throw new Exception("Error Retrieving Records");
            $rows = null;
        }
        return $rows;
    }
} 

auth.php

include("database.php");

class Auth {
    
    protected $db;
    
  /**
   * Auth::__construct()
   * 
   * @return
   */
    public function __construct()
    {
        $db = new Database;
    }
    
    
    //function to check login credentials
  /**
   * Auth::login()
   * 
   * @param mixed $username
   * @param mixed $password
   * @return
   */
    public function login($username, $password)
    {
        $query = $db->query("SELECT username, password FROM users WHERE username = '".mysql_real_escape_string($username)."' AND password = sha1('".mysql_real_escape_string($password)."')");
        $result = $db->fetch_rows($query);
        
        if (count($result) == 0)
        {
            return FALSE;
        }
        else
        {
            //set session variable 
            $_SESSION['logged_in'] = TRUE;
            return TRUE;
        }
    }
    
    //logout function
  /**
   * Auth::logout()
   * 
   * @return
   */
    public function logout()
    {
        return $_SESSION['logged_in'] = FALSE;
    }
    
} 

[imaginethis]

Mmm, I would probably not do this

        $this->hostname = $conf->get_hostname();
        $this->username = $conf->get_username();
        $this->password = $conf->get_password();
        $this->database = $conf->get_database(); 

I would prefer to store the config object and use it's methods to access the data. Also, I would invoke __get and __set to allow read/write privileges on member variables rather than creating a getter and setter function for each variable.

In the Auth class, rather than defining the db connection, I would pass it as a none required parameter in construct as well as create a setter and getter function for it. Login would then check to see if db connection is set and if not throw an error.

[franko75]

Thanks for the reply imaginethis. Ok I see how using __get and __set would be quicker. So get rid of my getters (no setters at the moment) and just use something like this:

public function __get($var){

        return $this->$var;
} 

When you say you would prefer to store the config object and use its methods to access the data, can you please explain or show a small example? I'm not sure how to go about this exactly

[imaginethis]

Thanks for the reply imaginethis. Ok I see how using __get and __set would be quicker. So get rid of my getters (no setters at the moment) and just use something like this:

public function __get($var){

        return $this->$var;
} 

When you say you would prefer to store the config object and use its methods to access the data, can you please explain or show a small example? I'm not sure how to go about this exactly

class Database

{
    private $_config = null;
    private $_link = null;
    private $_db = null;

  /**
   * Database::__construct()
   * 
   * @param mixed $conf
   * @return
   */

    public function __construct(Config $conf)
    {
        $this->setConfig($conf)
             ->_connect()
             ->_selectDb();
    }

    public function setConfig($conf)
    {
        $this->_config = $conf;
        return $this;
    }
     
    protected function _connect()
    {
        if(!($conf = $this->_conf))
          throw new Exception('Config Object not found');

        $this->_link = mysql_connect($conf->hostname, $conf->username, $conf->password);
        return $this;
    }

    protected function _selectDb()
    {
        if(!($conf = $this->_conf))
          throw new Exception('Config Object not found');

        if(!($link = $this->_link))
          throw new Exception('Connection to server failed');

        $this->_db = mysql_select_db($config->database, $link);
    }

} 

[Cups]

One thing I'm not sure about - where to start sessions, i.e in the auth class file in the constructor or outside of the class (but in the same file...if that makes sense).

I'd say make a small class that does that session work for you.

Then you can optionally make a class which manages both your auth class and your session class.

You should be able to use your session class in other projects, like a piece of lego.

[franko75]

@cups - thanks for the advice, i will give this a go. Normally I would use a framework or CMS and their in built session libraries, but although what i'm trying to do here is mainly for practice, it's also for small projects where a framework isn't really necessary. It would be really useful to have my own small scale library which i can re-use.

@imaginethis - cheers for showing how to store the config object and use it's methods from within the database class. I'm presuming single underscores are being used for private methods/variables?

In the Auth class, rather than defining the db connection, I would pass it as a none required parameter in construct as well as create a setter and getter function for it. Login would then check to see if db connection is set and if not throw an error.

Had a bash at this, is this what you mean? Presuming i should be using a getter and setting on this occasion and not magic methods.

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

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);
    }
   
    public function get_db()
    {
        return $this->_db;
    }
    
    public function set_db(Database $db)
    {
        return $this->_db = $db;
    }
} 

[galen]

you should try using pdo instead of the mysql_* functions

[bhanson]

I'm presuming single underscores are being used for private methods/variables?

Using underscores for private methods/data is pretty much a standard now.

In the Zend Coding Standard you also use underscores for protected methods and data.

In the PEAR Coding Standards you do not use an underscore for protected methods and data.

I would suggest for your own projects you pick one of these and try to follow it reasonably close. Not only are these designed to make your code easier to read and edit later, but if you ever work in a group project with other developers you'll be able to adapt and write code to a standard.

[Stormrider]

PHP 5 has pretty much removed the need for using _ though, you can just declare a variable as private or protected. I always thought using _ was a kind of indicator when using php 4, when all variables were public.

[imaginethis]

    public function set_db(Database $db)
    {
        return $this->_db = $db;
    } 

This is sort of pointless. You wouldn't want to set the database with an existing instance.

$this->db = $db->get_db(); 

This is also pointless. By passing $db, you already have a reference to the object.

class Auth {

    protected $_db;

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

    public function login($username, $password)
    {
        $query = $this->_db->query(...);
    } 

[imaginethis]

PHP 5 has pretty much removed the need for using _ though, you can just declare a variable as private or protected. I always thought using _ was a kind of indicator when using php 4, when all variables were public.

It's useful to use underscores for private and protected class members. It's a subtle reminder that the the variable or function that you are using is private or protected without having to revisit the declaration to figure out if variables label. It's a good habit NOT to break.

[logic_earth]

This doesn't answer your question, but I thought you would like to know you can write:

class Config
{
    protected $hostname = 'localhost';
    protected $username = 'root';
    protected $password = '';
    protected $database = 'my_database';
} 

As:

class Config
{
    protected
        $hostname = 'localhost',
        $username = 'root',
        $password = '',
        $database = 'my_database';
} 

Less times you have to repeat public/protected/private for class/object vars.

[logic_earth]

It's useful to use underscores for private and protected class members. It's a subtle reminder that the the variable or function that you are using is private or protected without having to revisit the declaration to figure out if variables label. It's a good habit NOT to break.

True it is a helpful reminder, however, for me personally my class variables are always protected never public and never private. So its not much help :P

[bhanson]

True it is a helpful reminder, however, for me personally my class variables are always protected never public and never private. So its not much help :P

You use protected variables when declaring `final` classes?

[logic_earth]

^^^ I don't use final classes...ever

[Stormrider]

Also, any decent editor will tell you what type of variable is being referred to as you type it, so it's not like you keep having to flick back to the declaration. I tend to remember which is which anyway, it's pretty obvious what should be private and what should be public.

[bhanson]

Also, any decent editor will tell you what type of variable is being referred to as you type it, so it's not like you keep having to flick back to the declaration. I tend to remember which is which anyway, it's pretty obvious what should be private and what should be public.

Obvious in your own code, but when reading code you've never seen before--anything to make it easier to work with is very welcome.

What editor do you use?

[Stormrider]

NuSphere PHPEd. I used to use Zend, but it ran very slowly on my PC. PHPEd seems to have everything that I liked about Zend, plus more, and runs a lot faster as well (it isn't built in Java...).

Surely if you are editing someone elses code, you have to know what the variable starts with anyway - if you are typing the variable name out, you already know if it's public or private because you have to know whether to start with a _ or not. If you don't know which it is, and DON'T use underscores, you can start typing the name and autocomplete will do the rest, and tell you what type it is

[logic_earth]

Obvious in your own code, but when reading code you've never seen before--anything to make it easier to work with is very welcome.

What editor do you use?

Sure that is an issue...if the code is nothing but a mess and mixes variable visibilities.

[bhanson]

if you are typing the variable name out, you already know if it's public or private because you have to know whether to start with a _ or not.

You know if it's public or private *because* you're using the underscore naming convention. If you aren't then you have no idea without looking at the definition.

If you don't know which it is, and DON'T use underscores, you can start typing the name and autocomplete will do the rest, and tell you what type it is

My vim doesn't tell me what type it is.

Sure that is an issue...if the code is nothing but a mess and mixes variable visibilities.

There are many "professional" PHP developers who write absolutely terrible code. At some point in your career you're bound to run into at least a few of them. However, your job becomes a lot easier if they were forced to use a coding standard that has an underscore naming convention.

Even though *you* do not use private variables, other developers do and if you know what coding standard they're using then the code suddenly becomes that much more meaningful.

Example:
Programmer is using PEAR coding standard. You see variable $foo and know it is public or protected, you know automatically $_bar is private.

Example 2:
Programmer is using Zend coding standard. You see variable $foo and know it is public. You see $_bar and know it is protected or private.

vs

Example 3:
Programmer is of unknown caliber and is not using a major coding standard. You see variable $foo and either 1. make an assumption or 2. look it up

[logic_earth]

^^^ In all three examples you have to look up all the vars anyways to know exactly what they are for, making the point moot. Why would one blindly use a variable one sees in the code?

[Stormrider]

You know if it's public or private *because* you're using the underscore naming convention. If you aren't then you have no idea without looking at the definition.

This doesn't make sense at all. You want to use a class variable, how do you know whether to use an underscore or not without finding its definition first?

[bhanson]

This doesn't make sense at all.

Yes it does.

Quoting you:

Surely if you are editing someone elses code, you have to know what the variable starts with anyway - if you are typing the variable name out, you already know if it's public or private because you have to know whether to start with a _ or not.

Example: You're editing someone else's code and see $this->config. Their code is consistent with Zend coding standard--therefore you know this variable is public.

Which is exactly what I said:

You know if it's public or private *because* you're using the underscore naming convention. If you aren't then you have no idea without looking at the definition.

If that person was not using a coding standard and you see $this->config then you have no idea what the visibility of that variable is.

^^^ In all three examples you have to look up all the vars anyways to know exactly what they are for, making the point moot. Why would one blindly use a variable one sees in the code?

Since when do you have to read the documentation for every method and every property in a class?

Good code is self documenting and a decent portion of the time you know what the code does without reading the documentation.

If you're in a method called setName($name) do you really have to read the @param phpdoc to figure out what $name does?

If you see the method _htmlToAscii() used, do you really have to go read the entire phpdoc block to figure out what the function does?

No, you don't. And even seeing just once usage of _htmlToAscii() you know it's private/protected and is not available outside of the class.

[logic_earth]

Since when do you have to read the documentation for every method and every property in a class?

I meant look up at the declaration of the class variable in the source code.

[imaginethis]

NuSphere PHPEd. I used to use Zend, but it ran very slowly on my PC. PHPEd seems to have everything that I liked about Zend, plus more, and runs a lot faster as well (it isn't built in Java...).

I don't subscribe to IDEs or WYSISWYG editors. I use, vim/vi or a plain text editor. Java offends me and I try to avoid using anything made in java as much as is permissible...

Surely if you are editing someone elses code, you have to know what the variable starts with anyway - if you are typing the variable name out, you already know if it's public or private because you have to know whether to start with a _ or not. If you don't know which it is, and DON'T use underscores, you can start typing the name and autocomplete will do the rest, and tell you what type it is

So, would you like all developers to abandon good programming habits simple because some tool can "autocomplete" and "and tell you what type it is"? You never program for yourself today. You program for yourself and everyone else tomorrow. Who's to say the day won't come when you or a co-worker, if you have one, won't have an IDE? How will you or that person function then?

I meant look up at the declaration of the class variable in the source code.

We don't all have the benefit of scrolling. My scrolling is Page Up, Page Down, Arrow Up and Arrow Down. Scrolling becomes laborious with a file that contains a 1000+ lines, of code or other wise.