4
\$\begingroup\$

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");
?>
\$\endgroup\$
  • \$\begingroup\$ Not too bad, way too much conditional nesting though. Move your verification code to another method and try and reduce all that nesting. $this->data != NULL is the same as !$this->data. Think of a better structure for the verifyNullFields() method, something simple but more readable could be if (!$this->user || !$this->data || !$this->host) return false; else return true; \$\endgroup\$ – Aaria Carter-Weir May 26 '11 at 8:09
  • \$\begingroup\$ You probably want to use the 4th mysql_connect param. As is, if you try to initiate 2 connections with the only difference being "mysql_select_db", you'll probably get the same connection with the primary database being changed, which most likely isn't what you'd be looking for. \$\endgroup\$ – bitsoflogic Jun 6 '11 at 17:36
  • \$\begingroup\$ REPLACE: if (!$link) TO: if (!$this->link) \$\endgroup\$ – user8724 Nov 30 '11 at 20:15
4
\$\begingroup\$

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');
}
\$\endgroup\$
  • \$\begingroup\$ @php-coder - Thank you for your example. I have been playing with it to better understand it and I have just a couple questions. Question 1, why did you break up Database and Mysql into two different files? I think I understand why DatabaseException is seperate and that's because it needs to be modular. Question 2, why did you make the Database class abstract? I have read up on abstract, but I am really not understanding it's use.... Thanks again for your example, it was very informative and is exatly what I needed :-) \$\endgroup\$ – Mike May 14 '11 at 14:10
  • \$\begingroup\$ @php-grasshoppa 1) Database and Mysql classes in different files because I preffer to place each class to separate file. Easy to maintain and easy to search when you need it. Why it in different classes? In this case, it is looks useless and no needed really. But if you will add support for MSSQL, PostgreSQL etc, then base class will be usefull. It contains common information with database' credentials. 2) Database class marked as abstract because this class should not be created by new Database in program. As I said above it encapsulates common information with related methods. \$\endgroup\$ – php-coder May 14 '11 at 15:28
  • \$\begingroup\$ @php-coder - I fully understand what you are saying about organization with classes. I never thought about putting one class per file, and I love that idea ..... nice :-) As for abstract, I bought an OOP book and it is describing the reason behind it, so thanks again for your time and knowledge. \$\endgroup\$ – Mike May 14 '11 at 19:35
  • \$\begingroup\$ surely use ` === null ` rather than is_null ? Why use sprintf when you could just use normal concatenation? Also, empty ? surely you can just do a falsey check? Otherwise.. an improvement :) \$\endgroup\$ – Aaria Carter-Weir May 26 '11 at 8:06
  • \$\begingroup\$ @skippychalmers IMHO these made code more readable. \$\endgroup\$ – php-coder May 26 '11 at 11:18
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hey Paul, thanks for all your input. This is great information :) I have transitioned to PDO after reading about the simplicity it offers as well as tightened security. \$\endgroup\$ – Mike Apr 24 '12 at 15:37
0
\$\begingroup\$

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);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.