7
\$\begingroup\$

As part of a fun project to help build my knowledge of PHP. I've written a Data Access Class here to bridge the site to the database. I know there are a lot of posts and articles out there explaining this type of thing, but I needed some individual feedback from people.

It does work, but I'm open to all sorts of responses! I'm torn between which format to work with, but I've been following Zend's code formatting documents. If you're familiar with that, I'd love to hear where my code is wrong there.

Any speed issues I'd gladly hear. I'm also open to code recycling tips.

class DataBase
{
    /** 
     * @var null|\PDO
     */
    private $_connection = NULL;
    /** 
     * @var string
     */
    private $_databaseHost = 'localhost';
    /** 
     * @var string
     */
    private $_databaseUser = 'root';
    /** 
     * @var string
     */
    private $_databasePassword = '';
    /** 
     * @var string
     */
    private $_returnMethod = 0;

    const DECIDE_RETURN_METHOD = 0;
    const ASSOC_ARRAY_RETURN_METHOD = 1;
    const LAST_ID_RETURN_METHOD = 2;
    const BOOLEAN_RETURN_METHOD = 3;
    /**
     * Construct a Database object. Use this to query the database(s).
     *
     * @param  string    $database The database to start a MySQL connection with.
     * @param  string    $host     The host the connection will be with.
     * @param  string    $username The username the connection will be with.
     * @param  string    $password The password the connection will be with.
     *
     * @throws Exception           If the database could not be connected to.
     */
    public function __construct($database, $host = 'localhost', $username = 'root', $password = '')
    {
        $this->_databaseHost = $host;
        $this->_databaseUser = $username;
        $this->_databasePassword = $password;
        try {
            $dsn = 'mysql:dbname=' .$database. ';host=' .$this->_databaseHost;
            $this->_connection = new PDO($dsn, $this->_databaseUser, $this->_databasePassword,array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
        } catch (PDOException $error) {
            throw new Exception('Could not connect to ' .$database. '.<br>' .$error->getMessage());
        }
    }
    /**
     * Execute a string of SQL to the database.
     *
     * @param  string     $sql    The SQL to be executed.
     * @param  array|null $data   Optional data to be prepared for the SQL staement.
     * @param  int        $return Use DECIDE_RETURN_METHOD if you're using a common sql statement, and it will choose what it thinks is the 
     *  best return method. Or choose ASSOC_ARRAY_RETURN_METHOD for an associative array to be returned, BOOLEAN_RETURN_METHOD for a boolean,
     *  or LAST_ID_RETURN_METHOD for the last inserted ID. If the last method is used, you may specify $data['lastInsertId'] to give
     *  a value for lastInsertId().
     *
     * @return mixed              Corresponds to the parameter: $return.
     */
    public function PerformDBQuery($sql, array $data = NULL, $return=self::DECIDE_RETURN_METHOD) 
    {
        if ($return == 0) {
            if (stripos($sql, 'select') > -1) {
                $this->_returnMethod = self::ASSOC_ARRAY_RETURN_METHOD;
            } elseif (stripos($sql, 'insert') > -1 || stripos($sql, 'update') > -1 || stripos($sql, 'delete') > -1) {
                $this->_returnMethod = self::BOOLEAN_RETURN_METHOD;
            }
        } elseif ($return > 0 && $return < 4) {
            $this->_returnMethod = $return;    
        }
        return $this->query_db($sql, $data);
    }
    /**
     * Execute a string of SQL to the database.
     *
     * @param  string     $sql    The SQL to be executed.
     * @param  array|null $data   Optional data to be prepared for the SQL staement.
     *
     * @throws Exception          If the sql statement is not a minimum 10 character string.
     * @throws Exception          If the database is not connected.
     *
     * @return mixed              Corresponds to $this->_returnMethod.
     */
    private function query_db($sql, array $data) 
    {
        if (is_string($sql) && isset($sql[9])) {
            if (!is_null($this->_connection)) {
                try {
                    if (is_null($data)) {
                        $statement = $this->_connection->query($sql);
                    } else {
                        $statement = $this->_connection->prepare($sql);
                        $statement->execute($data);
                    }
                    if ($this->_returnMethod == 1) {
                        return $statement->fetchAll(PDO::FETCH_ASSOC);
                    } elseif ($this->_returnMethod == 2) {
                        if (isset($data['lastInsertId'])) {
                            $column = $data['lastInsertId'];
                        } else {
                            $column = NULL;    
                        }
                        return $this->_connection->lastInsertId($column);
                    } elseif ($this->_returnMethod == 3) {
                        return TRUE;    
                    }
                } catch (PDOException $error) {
                    if ($this->_returnMethod == 1 || $this->_returnMethod == 2) {
                        return NULL;
                    } elseif ($this->_returnMethod == 3) {
                        return FALSE;    
                    }
                }
            } else {
                throw new Exception('Database connection not available.');
                return FALSE;
            }
        } else {
            throw new Exception('Invalid SQL string.');
            return FALSE;
        }
    }
}

If it's possible, I'd like an opinion on how to organize errors and exceptions!

  • Do I throw errors in this class?
  • Should I use Exceptions, or should I return the error message from the class?

I have yet to find an article explaining standards for error handling in this situation. I've been researching when to use self:: and $this-> and I'd like to know if I'm using those right.

\$\endgroup\$
2
\$\begingroup\$

I've been researching when to use self:: and $this-> and I'd like to know if I'm using those right.

self:: is for referencing static variables, etc $this-> is for dynamic variables

Eg your constants are static so

self::ASSOC_ARRAY_RETURN_METHOD is correct

Whereas calling a method from within the object itself (once it has been instantiated)

$this->PerformDBQuery(...

I have made some changes to your code, see inline comments

<?php

class DataBase
{
    /**
     * @var null|\PDO
     */
//    private $_connection = NULL;
//    /**
//     * @var string
//     */
//    private $_databaseHost = 'localhost';
//    /**
//     * @var string
//     */
//    private $_databaseUser = 'root';
//    /**
//     * @var string
//     */
//    private $_databasePassword = '';


    // why is this declared as a string in the php docs ???
    /**
     * @var string
     */
    private $_returnMethod = 0;


    // maybe a better name would be AUTO_DETECT_RETURN_METHOD
    const DECIDE_RETURN_METHOD = 0;

    const ASSOC_ARRAY_RETURN_METHOD = 1;
    const LAST_ID_RETURN_METHOD = 2;
    const BOOLEAN_RETURN_METHOD = 3;

    /**
     * Construct a Database object. Use this to query the database(s).
     *
     * @param  string $database The database to start a MySQL connection with.
     * @param  string $host The host the connection will be with.
     * @param  string $username The username the connection will be with.
     * @param  string $password The password the connection will be with.
     *
     * @throws Exception           If the database could not be connected to.
     */

    // Use of default username of root and password of blank should be discouraged
//    public function __construct($database, $host = 'localhost', $username = 'root', $password = '')
    public function __construct($database, $host = 'localhost', $username, $password)
    {

        // you have no re-connect function, so why store host/user/pass, storing the connection is sufficient
        // unless you have more advanced plans in mind for this class
        // $this->_databaseHost = $host;
        // $this->_databaseUser = $username;
        // $this->_databasePassword = $password;

        try {
            // use local use
            // $dsn = 'mysql:dbname=' . $database . ';host=' . $this->_databaseHost;
            $dsn = 'mysql:dbname=' . $database . ';host=' . $host;

            // $this->_connection = new PDO($dsn, $this->_databaseUser, $this->_databasePassword, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));
            $this->_connection = new PDO($dsn, $username, $password, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));

        } catch (PDOException $error) {
            throw new Exception('Could not connect to ' . $database . '.<br>' . $error->getMessage());
        }
    }

    /**
     * Execute a string of SQL to the database.
     *
     * @param  string $sql The SQL to be executed.
     * @param  array|null $data Optional data to be prepared for the SQL staement.
     * @param  int $return Use DECIDE_RETURN_METHOD if you're using a common sql statement, and it will choose what it thinks is the
     *  best return method. Or choose ASSOC_ARRAY_RETURN_METHOD for an associative array to be returned, BOOLEAN_RETURN_METHOD for a boolean,
     *  or LAST_ID_RETURN_METHOD for the last inserted ID. If the last method is used, you may specify $data['lastInsertId'] to give
     *  a value for lastInsertId().
     *
     * @return mixed              Corresponds to the parameter: $return.
     */
    public function PerformDBQuery($sql, array $data = NULL, $return = self::DECIDE_RETURN_METHOD)
    {

        // you have declared constants, might as well use them it makes your code a lot more readable
        //if ($return == 0) {
        if ($return == self::DECIDE_RETURN_METHOD) {

            // according to php docs, recommended way of testing stripos matches is !== FALSE
            // if (stripos($sql, 'select') > -1) {
            if (stripos($sql, 'select') !== FALSE) {
                $this->_returnMethod = self::ASSOC_ARRAY_RETURN_METHOD;
            } elseif (stripos($sql, 'insert') > -1 || stripos($sql, 'update') > -1 || stripos($sql, 'delete') > -1) {
                $this->_returnMethod = self::BOOLEAN_RETURN_METHOD;
            }

            // here is another way of checking the statement type, that is a little easier to read (in my opinion)
            $stmt_type = strtolower(substr($sql, 0, 6));

            if ($stmt_type == 'select') {
                $this->_returnMethod = self::ASSOC_ARRAY_RETURN_METHOD;
            } elseif ($stmt_type == 'insert' || $stmt_type == 'update' || $stmt_type == 'delete') {
                $this->_returnMethod = self::BOOLEAN_RETURN_METHOD;
            } else {
                // hang on, what if the statement isn't select/update/delete/insert
                // what returnMethod will you use
                // eg COMMIT and ROLLBACK are valid sql
            }

        // use your constants here too
        //} elseif ($return > 0 && $return < 4) {
        } elseif (in_array($return, array(self::ASSOC_ARRAY_RETURN_METHOD, self::LAST_ID_RETURN_METHOD, self::BOOLEAN_RETURN_METHOD ))) {
            $this->_returnMethod = $return;
        } else {
            // else??? what is the default
            // or should we throw an exception here?
        }

        // not sure why we are storing the return method as a property, it is a single use field (unless you have other plans in mind for this class)
        // why not just pass it as a parameter to the query_db method?

        return $this->query_db($sql, $data);
    }

    /**
     * Execute a string of SQL to the database.
     *
     * @param  string $sql The SQL to be executed.
     * @param  array|null $data Optional data to be prepared for the SQL staement.
     *
     * @throws Exception          If the sql statement is not a minimum 10 character string.
     * @throws Exception          If the database is not connected.
     *
     * @return mixed              Corresponds to $this->_returnMethod.
     */
    private function query_db($sql, array $data)
    {

        // I am just implementing @palacsint recommendations here, as I am in agreement with them

        if (is_string($sql) && isset($sql[9])) {
            throw new Exception('Invalid SQL string.');
        }

        if (!is_null($this->_connection)) {
            throw new Exception('Database connection not available.');
        }

        // i prefer to have 1 return statement per function if possible, for easier reading
        $ret = NULL;

        try {
            if (is_null($data)) {
                $statement = $this->_connection->query($sql);
            } else {
                $statement = $this->_connection->prepare($sql);
                $statement->execute($data);
            }

            // use the constants to help us understand your code
            // if ($this->_returnMethod == 1) {
            if ($this->_returnMethod == self::ASSOC_ARRAY_RETURN_METHOD) {
                $ret = $statement->fetchAll(PDO::FETCH_ASSOC);

            // use of constants again
            // } elseif ($this->_returnMethod == 2) {
            } elseif ($this->_returnMethod == self::LAST_ID_RETURN_METHOD) {
                if (isset($data['lastInsertId'])) {
                    $column = $data['lastInsertId'];
                } else {
                    $column = NULL;
                }
                $ret = $this->_connection->lastInsertId($column);
//            } elseif ($this->_returnMethod == 3) {
            } elseif ($this->_returnMethod == self::BOOLEAN_RETURN_METHOD) {
                $ret = TRUE;
            }



        } catch (PDOException $error) {

            // Is this the best way to handle pdo exceptions?
            // We are effectively masking the error and returning null
            // how will that be useful to the end user.
            // Perhaps we could store the error in a property call last_error_message or something like that
            // then the user could do
            //
            // if (!$dal->db_query($sql, $data)) {
            //    echo $dal->last_error_message();
            // }

            // if ($this->_returnMethod == 1 || $this->_returnMethod == 2) {
            if ($this->_returnMethod == self::ASSOC_ARRAY_RETURN_METHOD || $this->_returnMethod == self::LAST_ID_RETURN_METHOD) {

                $ret = NULL;
            // } elseif ($this->_returnMethod == 3) {
            } elseif ($this->_returnMethod == self::BOOLEAN_RETURN_METHOD) {

                $ret = FALSE;
            }
        }

        return $ret;
    }
}
\$\endgroup\$
5
\$\begingroup\$

Just a few random notes:

  1. Correct me if I'm wrong but if you throw an exception the statements after the throw statements won't be run. So here return FALSE never runs, therefore it's unnecessary:

    throw new Exception('Database connection not available.');
    return FALSE;
    
  2. The query_db function could use guard clauses to make the code flatten:

    private function query_db($sql, array $data) {
        if (!is_string($sql) || !isset($sql[9])) {
            throw new Exception('Invalid SQL string.');
        }
        if (is_null($this->_connection)) {
            throw new Exception('Database connection not available.');
        }
        try {
        ...
    }
    

    References:

    • Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code;
    • Flattening Arrow Code
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for both of those. I didn't realize throws would do that! I originally had this class with guard clauses, but when I re-coded to update it, I took them out for some reason. Thanks for putting me back on track! \$\endgroup\$ – Alex L Jan 26 '14 at 0:14

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.