Database in Class for Reusability

Question

Does this code fine for database reusability?

<?php
/**
 * Database
 *
 * Class for handling database connections
 *
 * 
 * @category   Global Class
 * @copyright  ---          
 * @license    ---    
 * @version    1.0
 * @link       ---
 * @since      File available since Release 1.0
 * @created    ---
 * @modified   ---
 * @author     Original Author <--->
 * @author     Future Author <--->
 */

class Database
{
    private $database_connection = null;

    public function __construct( $database_id )
    {
        $database_host     = '';
        $database_name     = '';
        $database_username = '';
        $database_password = '';

        switch ( $database_id ) {
            case 'db1':
                $database_host     = 'host';
                $database_name     = 'name';
                $database_username = 'user';
                $database_password = 'pass';
                break;
            case 'db2':
                $database_host     = 'host';
                $database_name     = 'name';
                $database_username = 'user';
                $database_password = 'pass';
                break;
            default:
                trigger_error( 'Undefined database id', E_USER_ERROR );
        }

        try {
            $this->database_connection = new PDO( "mysql:host={$database_host};
                                                  dbname={$database_name}",
                                                  $database_username,
                                                  $database_password );

            $this->database_connection->setAttribute( PDO::ATTR_ERRMODE,
                                                      PDO::ERRMODE_EXCEPTION );
        } catch ( PDOException $pdo_exception ) {
            trigger_error( $pdo_exception, E_USER_ERROR );
        }

        if ( empty( $this->database_connection ) ) {
            trigger_error( 'Undefined database connection', E_USER_ERROR );
        }       
    }

    public function execute_query( $query, $statement_id, $values )
    {
        $statement = $this->database_connection->prepare( $query );

        if ( $statement_id == 'db1' ) {
            $statement->bindParam( ':id', $values[ 0 ] );
        } else if ( $statement_id == 'db2' ) {
            $statement->bindParam( ':id1', $values[ 0 ] );
            $statement->bindParam( ':id2', $values[ 1 ] );
        } else {
            trigger_error( 'Undefined statement id', E_USER_ERROR );
        }

        $statement->execute();

        return $statement;
    }
}

?>

and call it here like this?

<?php
require once( "database.php" );

$database_1 = new Database( "db1" );
$database_2 = new Database( "db2" );

$database_1->execute_query( "SELECT * FROM user WHERE id = :id", "db1", array( 0 ) );
$database_1->execute_query( "SELECT * FROM user WHERE id = :id", "db1", array( 1 ) );
$database_2->execute_query( "SELECT * FROM user WHERE id = :id1 AND id2 = :id2", "db2", array( 0, 1 ) );
$database_2->execute_query( "SELECT * FROM user WHERE id = :id1 AND id2 = :id2", "db2", array( 2, 3 ) );

?>

Answer 1

No, this code isn't reusable.

Currently, you cannot "execute queries" but execute SELECT queries with exactly one or two parameters. What if you need to add a third id? Or a fourth? Or have a query with no parameters?

The easiest way to solve your problem is to just pass your $values array to execute:

public function execute_query( $query, $statement_id, $values )
{
    $statement = $this->database_connection->prepare( $query );
    $statement->execute($values);
    return $statement;
}

But this still doesn't make very much sense. Your function returns a PDO statement, which still has to be processed, so you didn't really gain anything by introducing this method (well, you save one line for every query you write).

You should either get rid of it and use PDO directly (or some alternative such as an ORM), or you should handle all PDO related code inside of this method (make sure that the method actually improves on using just PDO though).

Misc

• It is pretty much never a good idea to change the behavior of a function based on some control string (like your db1/db2), as it is difficult to use, it must be properly documented, it can easily lead to bugs, etc. In your case, you could simply get rid of the parameter and ask for the size of $values.
• $statement_id is poorly named.
• You use the same db1/db2 string for two very different things (one for different database credentials, one for the number of ids. This is very confusing.

Answer 2

I frankly don't see what value this class adds. Typically, one may implement a database wrapper class to do things like:

• Manage connections to the database. For example, enforcing that all classes interacting with the database use a single connection (Singleton) or to abstract away the management of a connection pool from the consuming code.
• Provide abstraction from the database implementation. Since PDO is in itself an abstraction layer and you are still requiring your calling code to understand that they are working with a PDO object, your class adds no value here.
• Provide a model for object-relational mapping (ORM), whereby you abstract common database record actions (CRUD - create, read, update, delete) by having classes extend the model business logic for whatever object you are creating (i.e. a User class might extend DB_Model class to get basic CRUD functionality, with the user class just defining the tables and columns pertinent to a user object)
• Provide "natural language" querying capabilities that DB classes in many frameworks provide (usually in conjunction with ORM model). They might do things like $users = Users::find('id', $id)->get(); such that you get away from writing SQL altogether unless you choose to do so.

You are doing nothing of the sort here. You are really only storing database configuration here in the class, something that really should be split out into separate application configuration functionality and not hard-coded in a class somewhere.

My recommendations would be:

• Abandon this class as it currently has no value. As your application evolves you might find a reason to reconsider wrapping your PDO access, but until that time comes, why put more effort into a class that brings no value?
• Put database configuration into a separate portion of your application (or perhaps better yet into environmental configuration such that these credentials don't live in your codebase.
• Instantiate your two database connections as needed in your application and simply pass the resulting PDO objects to any code/classes that need to leverage them (i.e. dependency injection).