Learning OOP PHP, simple MySQL connection class.

Question

I have posted an earlier version of this and here is the improved version from the feedback I recieved. Some of the feedback I received was;

• Don't chain method (tried my best to limit this)
• Do not use print() or die() and an error response (still a little lost, but I did attempt to use a redirect to a custom error page)

I tried my best to rewrite the code, I am still very new to this so go easy. I did read up on OOP methodology and read about Interfacing and Implementation, so I tried to incorporate that in my class.

I would love to see some different ideas on how to make this the most efficient as possible. I am sure it needs a ton of changing, but that is why I am here to help me learn and grow. I am a visual learner so if possible actual code would be awesome, but any response would be greatly appreciated.

<?php
class Mysql
{
        private $user;
        private $pass;
        private $data;
        private $host;

        public function __construct($user,$pass,$data,$host)
        {
                $this->user = $user;
                $this->pass = $pass;
                $this->data = $data;
                $this->host = $host;
                $this->process();
        }

        /* INTERFACE */
        private function process()
        {
                if($this->verifyNullFields()==true)
                {
                        if($this->verifyDatabaseConnection()==true)
                        {
                                if($this->verifyDatabaseExist()==true)
                                {
                                        print('ALL PASSED'); //for debugging
                                }
                                else
                                {
                                        print('redirect to custom error page will go here');
                                }
                        }

                        else
                        {
                                  print('redirect to custom error page will go here');
                        }

                }
                else
                {
                        print('redirect to custom error page will go here');
                }
        }

        /* IMPLEMENTATIONS */
        private function verifyNullFields()
        {
                if($this->user != NULL)
                {
                        if($this->data != NULL)
                        {
                                if($this->host != NULL)
                                {
                                        return true;
                                }
                                else
                                {
                                        return false;
                                }

                        }
                        else
                        {
                                return false;
                        }
                }
                else
                {
                        return false;
                }
        }
        private function verifyDatabaseConnection()
        {
                $link = @mysql_connect($this->host,$this->user,$this->pass);
                if(!$link)
                {
                        return false ;
                }
                else
                {
                        return true;
                }
        }
        private function verifyDatabaseExist()
        {
                $db = @mysql_select_db($this->data);
                if(!$db)
                {
                        return false;
                }
                else
                {
                        return true;
                }
        }
}
?>
<?php
$m = new Mysql("root","","magic","localhost");
?>

Answer 1

Let me show my version of this code:

DatabaseException.php:

class DatabaseException extends Exception {
}

Database.php:

abstract class Database {
    protected $login;
    protected $password;
    protected $database;
    protected $hostname;

    public function __construct($login, $password, $database, $hostname) {
        // NB: password not checked and may be empty
        $this->throwExceptionIfNotSet('login', $login);
        $this->throwExceptionIfNotSet('database', $database);
        $this->throwExceptionIfNotSet('hostname', $hostname);

        $this->login    = $login;
        $this->password = $password;
        $this->database = $database;
        $this->hostname = $hostname;
    }

    private function throwExceptionIfNotSet($argName, $argValue) {
        if (empty($argValue)) {
            throw new DatabaseException("'${argName}' not set");
        }
    }

}

Mysql.php:

class Mysql extends Database {

    private $link = null;

    public function __construct($login, $password, $database, $hostname) {
        parent::__construct($login, $password, $database, $hostname);
        $this->connect();
        $this->selectDatabase();
    }

    public function connect() {
        if (! is_null($this->link)) {
            return;
        }

        $link = @mysql_connect($this->hostname, $this->login, $this->password);
        if (! $link) {
            throw new DatabaseException(
                sprintf(
                    'Cannot connect to database. mysql_connect() to %s with login %s fails',
                    $this->hostname,
                    $this->login
                )
            );
        }
    }

    public function selectDatabase() {
        $ret = @mysql_select_db($this->database, $this->link);
        if (! $ret) {
            throw new DatabaseException("Cannot select database {$this->database}");
        }
    }

}

application.php:

try {
    $db = new Mysql('root', '', 'magic', 'localhost');
    print('ALL PASSED'); //for debugging

} catch (DatabaseException $ex) {
    print('redirect to custom error page will go here');
}

Answer 2

This now old question needs another answer. No-one has warned of the dangerous nature of the @ operator!

@ suppresses errors (hides them). This is a very bad thing. You can use it and get away with it if you do everything correctly (checking for boolean flags etc.). But when you make a mistake you won't know why. I make mistakes, everyone I know does. So, I suggest against the use of the @ operator.

mysql_* is now softly deprecated (use PDO or mysqli).

Your method names can be improved:

• process gives me no idea what it is doing.
• verifyDatabaseConnection actually connects to the database! verify should only be used to check things, not do things.
• verifyDatabaseExist similarly selects the database (you need to use the right verb).

The problem with your verify methods are that they are doing 2 things. Connecting or Selecting and then Verifying.

Please read Jeff Attwood's Flattening Arrow Code.

Keep work out of the constructor for good OO. Don't call process. While I would never write mysql_* this is what your class should have looked like:

class Mysql
{
    private $db;

    public function __construct($host, $user, $pass)
    {
        $this->db = mysql_connect($host, $user, $pass);

        if (!$this->db)
        {
            throw new RuntimeException(
                __METHOD__ . ' could not connect to DB due to error: ' .
                mysql_error());
        }
    }

    public function selectDatabase($database)
    {
        $db_selected = mysql_select_db($database, $this->db);

        if (!$db_selected)
        {
            throw new RuntimeException(
                __METHOD__ . ' could not select database: ' . $database .
                ' error: ' . mysql_error());
        }
    }
}

No errors will be suppressed and it is simple and easy to read.

Answer 3

Please change this line:

$link = @mysql_connect($this->hostname, $this->login, $this->password);

to this:

$this->link = @mysql_connect($this->hostname, $this->login, $this->password);