3
\$\begingroup\$

Feel free to critique this database wrapper which is written as a code example for employers or clients.

<?php 

    class Database {

        static function getInstance()
        {
            if (self::$instance == NULL) {
                self::$instance = new Database();
            }
            return self::$instance;
        }

        public function connect()
        {
            /*
             if dbConnection already exists, do not
             make another one.
             */
            if (is_resource($this->dbConnection))
            {
                return true;
            }

            $conn = $this->createDbConnection();
            if (!$conn){
                trigger_error("Error Connecting to Database", E_USER_NOTICE);
            }

            $this->dbConnection = $conn;
            $result = mysql_select_db($this->database_name, $conn);
            if (!$result){
                trigger_error("Error Selecting Database", E_USER_NOTICE);
            }
        }//endfunction

        public function closeConnection()
        {
            /*
             mysql documentation says closing a connection
             isn't nessesary since the connection is closed
             at the end of the script.
             */
            mysql_close($this->dbConnection);
        }

        public function executeQuery($query)
        {
            $this->query_result = mysql_query($query);
            if (!$this->query_result){
                trigger_error("Error Executing Database Query", E_USER_NOTICE);
            }
        }

        public function getRow(){
            return mysql_fetch_array($this->query_result);
        }


        private function createDbConnection()
        {

            if ($_SERVER["SERVER_NAME"] == "website.com") {
                $this->database_name = 'databasename';
                return mysql_connect('localhost', 'username', 'password');
                //$msdb = mysql_connect("localhost", "root", "");
                //mysql_select_db("test", $msdb) or die(mysql_error());
            }

        }

        private $dbConnection;
        private $query_result;
        private $database_name;
        static private $instance = NULL;
    }

?>
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

I'm going to be harsh, maybe you will disagree with my assessment, or maybe it will help you get a job.

I don't understand why everyone wants to make a singleton database class. Singleton is evil. Now you are limited to a single database object (should we all rejoice?). Interestingly your connection can be managed to connect to different databases using closeConnection and connect (but only 1 at a time). What is the point of this?

If you really must make it a singleton - do it right. Stop people calling constructing, cloning or getting another from unserialize:

private function __construct();
private function __clone();
private function __wakeup();

See the singleton pattern here.

I am not impressed by your choice of mysql when you could have chosen PDO or at least mysqli (mysql improved). PDO in my opinion is the best which allows you to connect to non-mysql databases too.

You have hard-coded values spread throughout your code.

Your object properties appear at the end of your class rather than the start. Personally I like to see them at the start as they tend to be used throughout the methods. Seeing them at the start of the class gives me an idea of the object that is created by the class.

I prefer exceptions rather than errors when I have serious database problems.

Your indentation is not completely consistent. You use the beautiful Allman style once and have extra spacing in createDbConnection.

// endfunction should not be a comment.

Provide a comment for each method. If a method does not do enough to even be commented on does it even deserve to exist?

For pure php I would advise not ending your file with ?>. Extra spacing (such as carriage returns) after this causes output which messes up header function calls.

I think less is more when it comes to example code. I'm not looking through any of the other code on your site. I think a very good database class alone should be enough example code. However, to be very good it must really encapsulate the ideas of a database, be well commented and standalone (loosely coupled). It should also be consistent in its interface and structure.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.