4
\$\begingroup\$

I learned of the Database Access Object design pattern from a friend and decided to implement it recently to my MySQL database. The code uses MySQLi to connect to the Database, and uses prepared statements to execute all queries. I have tried to apply OOP principles in creating these series of classes (encapsulation, code reuse).

Below is the CustomerValueObject class I simply use as a container for a Customer that I retrieve from a query. It contains simply a constructor and various setters and getters.

<?php

class CustomerValueObject {
    private $customerUserName;
    private $firstName;
    private $middleName;
    private $lastName;

    public function __construct( $customerUserName, $firstName, $middleName, $lastName ) {
        $this -> customerUserName = $customerUserName;
        $this -> firstName = $firstName;
        $this -> middleName = $middleName;
        $this -> lastName = $lastName;
    }

    public function getCustomerUserName( ) {
        return $this -> customerUserName;
    }

    public function setCustomerUserName( $customerUserName ) {
        $this -> customerUserName = $customerUserName;
    }

    public function getFirstName( ) {
        return $this -> firstName;
    }

    public function setFirstName( $firstName ) {
        $this -> firstName = $firstName;
    }

    public function getMiddleName( ) { 
        return $this -> middleName;
    }   

    public function setMiddleName( $middleName ) {
        $this -> middleName = $middleName;
    }

    public function getLastName( ) {
        return $this -> lastName;
    }

    public function setLastName( $lastName ) {
        $this -> lastName = $lastName;
    }
}

?>

Below is my parent DatabaseAccessObject class. Each class that inherits from it is suppose to act on one table of my Database alone.

<?php
//Not the actual values for my DB below
abstract class DatabaseAccessObject{
    private $DATABASE_HOST = '*';
    private $DATABASE_USERNAME = '*';
    private $DATABASE_PASSWORD = '*';
    private $DATABASE_NAME = '*';

    protected $connection;

    public function __construct( ) {
        $this -> connection = new mysqli( $this -> DATABASE_HOST, $this -> DATABASE_USERNAME, 
            $this -> DATABASE_PASSWORD, $this -> DATABASE_NAME );
    }

    /*
    @$sqlStatement - takes in a SQL prepared statement (or partial statement) 
    @$sqlBindParams - array of parameters to bind SQL statement - if element in null - not concatenated 
    @$sqlClauses - array of clauses appended to $sqlStatement 
    @$paramTypes - array of types for each variable in $sqlBindParams
    @$sqlStatementBinder - string that is appended to each element in $sqlClauses after the first

    Concatenates together a sqlStatement on non-null values of $sqlBindParams together
    and returns an array of $sqlParameters - with first element as $types followed by the bind parameters 
    */
    protected function setUpQuery( &$sqlStatement, &$sqlBindParams, &$sqlClauses, $paramTypes, $sqlStatementBinder = ' and' ) {
        $types = '';
        $sqlParameters = array( );
        $sqlStatementClauses = array( );

        for( $i = 0; $i < count( $sqlBindParams ); ++$i ) {
            if( isset( $sqlBindParams[ $i ] ) ) {
                $sqlStatementClauses[ ] = &$sqlClauses[ $i ];
                $types .= $paramTypes[ $i ];
                $sqlParameters[ ] = &$sqlBindParams[ $i ];
            }
        }

        array_unshift( $sqlParameters, '' );
        $sqlParameters[ 0 ] = &$types;

        $sqlStatement = $this -> concatSQLStatement( $sqlStatement, $sqlStatementClauses, $sqlStatementBinder );
        return $sqlParameters;
    }


    /*
    Concatenates a sqlStatement together
    $sqlStatement is a complete or partial sql statement.
    $sqlSegments is an array of parts of a sql statement to be appended to the $sqlStatment.
    $sqlStatementBinder is a string that is appended to a $sqlSegment after the first $sqlSegement is added
    returns a complete $sqlStatement
    */
    protected function concatSQLStatement( $sqlStatement, $sqlSegments, $sqlStatementBinder ) {
        $added = 0;
        foreach( $sqlSegments as $segments ) {
            if( $added > 0 ) {
                $sqlStatement .= $sqlStatementBinder;
            }
            $sqlStatement = $sqlStatement . ' ' . $segments;
            ++$added;
        }
        $sqlStatement .= ';';
        return $sqlStatement;
    }

    abstract protected function executeStatement( $sqlQuery, $parameters );

    public function __destruct( ) {
        $this -> connection -> close();
    }

}

?>

Finally - below is a CustomerDatabaseAccessObject class. This inherits from the DAO above.

<?php
require_once( dirname( __FILE__ ) . '\DatabaseAccessObject.class.php' );
require_once( dirname( __FILE__ ) . '\..\ValueObjects\CustomerValueObject.class.php' );

class CustomerDatabaseAccessObject extends DatabaseAccessObject {   

    public function __construct( ) {
        parent::__construct( );
    }

    public function getAllCustomers( ) {
        $sqlQuery = 'select * from Customer;';
        return $this -> executeStatement( $sqlQuery ) ;
    }

    public function getCustomerByUserName( $userName ) {
        $sqlQuery = 'select * from Customer where customer_username = ?;';
        $sqlParameters = array( );
        $paramType = 's';

        $sqlParameters[ 0 ] = &$paramType;
        $sqlParameters[ 1 ] = &$userName;
        return $this -> executeStatement( $sqlQuery, $sqlParameters );
    }

    public function getCustomerByName( $firstName = null, $middleName = null, $lastName = null ) {
        $sqlQuery = 'select * from Customer where';
        $names = [ $firstName, $middleName, $lastName ];
        $sqlClauses = [ 'first_name = ?', 'middle_name = ?', 'last_name = ?' ];
        $paramTypes = [ 's', 's', 's' ];

        $sqlParameters = $this -> setUpQuery( $sqlQuery, $names, $sqlClauses, $paramTypes );

        return $this -> executeStatement( $sqlQuery, $sqlParameters );
    }

    public function addCustomer( $userName, $firstName, $middleName, $lastName ) {
        $sqlStatement = 'insert into Customer ( customer_username, first_name, middle_name, last_name ) values ( ?, ?, ?, ? );';
        $paramTypes = 'ssss';

        $sqlParameters = array( );
        $sqlParameters[ 0 ] = &$paramTypes;
        $sqlParameters[ 1 ] = &$userName;
        $sqlParameters[ 2 ] = &$firstName;
        $sqlParameters[ 3 ] = &$middleName;
        $sqlParameters[ 4 ] = &$lastName;

        return $this -> executeStatement( $sqlStatement, $sqlParameters );
    }

    public function updateCustomer( $userName, $firstName = null , $middleName = null, $lastName = null ) {
        $sqlStatement = 'update Customer set';
        $customerData = [ $firstName, $middleName, $lastName, $userName ];
        $sqlClauses = [ 'first_name = ?', 'middle_name = ?', 'last_name = ?', 'where customer_username = ?' ];
        $paramTypes = [ 's', 's', 's', 's' ];

        $sqlParameters = $this -> setUpQuery( $sqlStatement, $customerData, $sqlClauses, $paramTypes, '' );

        return $this -> executeStatement( $sqlStatement, $sqlParameters ); 
    }

    public function deleteCustomer( $userName ) {
        $sqlStatement = 'delete from Customer where customer_username = ?;';
        $sqlParameters = array( );
        $paramType = 's';

        $sqlParameters[ 0 ] = &$paramType;
        $sqlParameters[ 1 ] = &$userName;

        return $this -> executeStatement( $sqlStatement, $sqlParameters );
    }

    protected function executeStatement( $sqlQuery, $parameters = array( ) ) {
        $sqlStatement = $this -> connection -> prepare( $sqlQuery );
        if( $sqlStatement ) {
            if( count( $parameters ) > 0 ) {
                call_user_func_array( array( $sqlStatement, 'bind_param' ), $parameters );
            }
            $resultSet = array();
            if( $sqlStatement -> execute() ) {
                $sqlStatement -> store_result( );
                if( $sqlStatement -> num_rows > 0 ) {
                    $sqlStatement -> bind_result( $customerUserName, $firstName, $middleName, $lastName );
                    while( $sqlStatement -> fetch( ) ) {
                        array_push( $resultSet, new CustomerValueObject( $customerUserName, $firstName, $middleName, $lastName ) );
                    }
                }
            }
            else {
                $error = 'Failed Execute Query: ' . $this -> connection -> error . "in function: executeQuery( \$sqlQuery, \$parameters )\n";
                throw Exception ( $error );
            }
            $sqlStatement -> close();       
            return $resultSet;
        }
        else {
            $error = 'Failed Prepared Query: ' . $this -> connection -> error . "in function: executeQuery( \$sqlQuery, \$parameters )\n";
            throw new Exception( $error );
        }
    }       
}

?>

In terms of my CustomerDAO if throwing an exception for executeStatement was the right way to fail it.

Test code to show that it is working:

<?php
        print( "getAllCustomers( ): <br> " );
        $customerDAO = new CustomerDatabaseAccessObject( );
        $customers = $customerDAO -> getAllCustomers( );
        print_r( $customers );

        print( "<br><br> getCustomerByUserName( 'user0' ): <br> " );
        $customers = $customerDAO -> getCustomerByUserName( 'user0' );
        print_r( $customers );

        print( "<br><br> getCustomerByName( 'Frank' ): <br> " );
        $customers = $customerDAO -> getCustomerByName( 'Frank' );
        print_r( $customers );

        print( "<br><br> getCustomerByName( null, null, 'Sinatra' ): <br> " );
        $customers = $customerDAO -> getCustomerByName( null, null, 'Sinatra' );
        print_r( $customers );

        print( "<br><br> getCustomerByName( null, null, null ): <br> " );
        try {
            $customers = $customerDAO -> getCustomerByName( null, null, null );
            print_r( $customers );
        }
        catch(  Exception $exception ) {
            print( $exception -> getTraceAsString() );
        }

        print( "<br><br> addCustomer( 'user1', 'Robert', 'E', 'Lee'): <br> " );
        try {
            $customers = $customerDAO -> addCustomer( 'user1', 'Robert', 'E', 'Lee');
            print_r( $customers );
        }
        catch( Exception $exception ) {
            print( $exception -> getTraceAsString( ) );
        }

        print( "<br><br> addCustomer( 'user1', 'Robert', 'E', 'Lee'): <br> " );
        try {
            $customerDAO -> updateCustomer( 'user1', 'Hell' ); 
        }
        catch( Exception $exception ) {
            print( $exception -> getTraceAsString( ) );
        }
        $customers = $customerDAO -> getAllCustomers( );
        print_r( $customers );


        print( "<br><br> deleteCustomer( 'user1' ): <br>" );
        try { 
            $customerDAO -> deleteCustomer( 'user1' );
        }
        catch( Exception $exception ) {
            print( $exception -> getTraceAsString( ) );     
        }
        $customers = $customerDAO -> getAllCustomers( );
        print_r( $customers );

    ?>

I want to know if:

  • I'm applying the design pattern correctly
  • If my classes or functions need any refactoring
  • Any concerns that these classes raise (performance, security, etc.)
\$\endgroup\$
4
\$\begingroup\$

Ah, well, after two days... I will give it a try.

CustomerValueObject

As for your first class: 'CustomerValueObject'; it doesn't do much, so there's not much I can say about it.

Perhaps it should contain the code for the retrieval of the values from the database? In that case the constructor would only need a customer id as an argument. It would make the class more useful.

And then the name: It could simply be called 'customer', the other two words don't add anything, moreso it is not an object, it's a class, so 'customerClass' would be better.

Inside the class you're a bit inconsistent with your names: You use 'getCustomerUserName()' and 'getFirstName()', I would change the first to 'getUserName()' since it's obviously belonging to a customer.

So you see, even such a simple class can be improved.

DatabaseAccessObject

Then the 'DatabaseAccessObject', ehm 'class'. You define the database access parameters within this class, that's a bad idea. If you want to use this class to access another database you will actually have to change the class itself. Not good! Better do it like this:

$DATABASE_CANDYBAR = array('hostaddr' => '*',
                           'username' => '*',
                           'password' => '*',
                           'database' => 'Candybar');

and then inside the class:

public function __construct($accessArray) {
  extract($accessArray);
  $this->connection = new mysqli($hostaddr,$username,$password,$database); 
}

Only now you have a reusable class, and that's the whole point of classes.

The quality of 'setUpQuery()' and 'concatSQLStatement()' is not very good. To start: 'concatSQLStatement()' is used once, by the class itself, then why is it a function at all, especially since it hardly does anything. It's function could be summarized in one line of code:

$sqlStatement .= implode(" $sqlStatementBinder ",$sqlSegments);

So this could replace this line inside 'setUpQuery()':

$sqlStatement = $this -> concatSQLStatement( $sqlStatement, $sqlStatementClauses, $sqlStatementBinder );

Also note that you should be consistent about your naming. In one place you call something '$sqlStatementClauses' and the same thing elsewhere is called '$sqlSegments'. How should anybody understand they're exactly the same thing? Oh, and surprise, a third name: '$sqlClauses'.

To be honest, I've got no idea what the purpose of 'setUpQuery()' exactly is, but I trust it does what the description says it does, albeit in a very complicated way.

'executeStatement()' seems to be a failed function? It certainly doesn't do what anybody would think.

CustomerDatabaseAccessObject

And here's the piece the resistance: 'CustomerDatabaseAccessObject'. Obviously the name of this class is not correct. This class is about all customers. So it should be called 'customersClass'.

I would have preferred to see a cleaner distinction between what I call your 'customerClass' and this 'customersClass', because a lot of functionality in 'customersClass' should be in 'customerClass'. For instance: 'addCustomer()' belongs in 'customersClass' whereas 'updateCustomer()' could be part of 'customerClass'.

Better still, wouldn't it be logical if getCustomerByName() would return a 'customerClass' object?

I can see you've tried hard to fit the MySQLi library in this class, but that shouldn't be the objective: You're trying to make it easier to access your customers. The brunt of the stuff that deals with database access should not be here. There's hardly any abstraction in your 'DatabaseAccessObject' so now you have to deal with types and various other SQL things in this class. That's not how it should be. No!

Conclusion

In short: There's a lot to improve.

Try to start anew but then from the top. You have customers, and a single customer. Define those classes, and their methods first, without exactly specifiying how these methods should be implemented.

Apart from this you have the database access. You need to be able to SELECT, INSERT, UPDATE and DELETE. Try to abstract these database functions so you end up with methods like 'retrieveRow($rowID)', 'updateRow($rowID,$data)', 'deleteRow($rowID)', etc.

Finally you need to implement your customer(s) classes, but with abstracted database access that should be a breeze.

I haven't looked for any security holes, but this code is not ready for production.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Cannot upvote yet - not 15 reputation but would. Fixed the names - I only used those names to denote the actual design pattern I constructed them after (Value Object pattern and Database Access Object pattern). The 'customer' is simply suppose to hold the customer data so I can pass the data. When I make a query, I want to be able to return a customer data. I made the decision to make the class an immutable but publically accessible object, reducing the class size. I made the change to have the DAO constructor take the those as inputs as you suggested instead of storing them. \$\endgroup\$ – Rockyx123 Mar 4 '15 at 3:36
  • \$\begingroup\$ executeStatement() - sets up the prepared statement - and binds the parameters to prepared statement where the $sqlQuery = "select * from Customer where u_name = ?;" where the $parameters = array( "username" ); It executes and returns whatever is in the result set. Could you clarify what confused you in this function? I based the design pattern on [link] sitecrafting.com/blog/php-patterns-part-ii [/link]. \$\endgroup\$ – Rockyx123 Mar 4 '15 at 3:38
  • \$\begingroup\$ The setUpQuery and concatSQLStatement were the basis for this review in the first place. They existed because I didn't want to write multiple functions for querying each part of a customer name (by first name, by middle, by last) - but ultimately, that seems to remove the need for those two functions. I'll post a code update when I finish updating it. \$\endgroup\$ – Rockyx123 Mar 4 '15 at 3:39
  • \$\begingroup\$ Note that there are two 'executeStatement()' methods, I was talking about the one in the 'DatabaseAccessObject' class. Please do not change the code in this question, instead post a new question with your new code. This way the answers here will stay relevant, and people can see how your code progressed. \$\endgroup\$ – KIKO Software Mar 4 '15 at 8:13
  • \$\begingroup\$ My apologizes for tardiness in reply. That first "executeStatement()" was simply an abstract declaration - since the DAO class in abstract (similar to C++ virtual) - you couldn't instantiate an instance of that class. I wanted any class that inherited that class to implement "executeStatement()". \$\endgroup\$ – Rockyx123 Mar 9 '15 at 20:22

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.