Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So, my question is more of a 'best practices' question rather than a question with a particular aim. This is my current understanding of PHP factories and how to incorporate them into a project using PDOs and dependency injection. I'm pretty new at this and I still don't understand a lot about this subject. As such, I really don't know if I'm doing this quite right. Could someone point me in the right direction?

<?php
class PDOSettings_Factory {
    private function __construct() {}
    public static function build(Array $args = NULL) {
        $settings = array();
        // Set default values.
        $settings["dbType"] = 'mysql';
        $settings["dbName"] = 'test';
        $settings["host"] = 'localhost';
        $settings["user"] = 'guest';
        $settings["pass"] = '1234';
        // If keys are the same, they will be overwritten.
        if(isset($args)) {
            foreach($args as $key => $val) {
                $settings[$key] = $val;
            }
        }
        // Return setings.
        return $settings;
    }
}

$settings = PDOSettings_Factory::build();

class PDOConnection_Factory {
    private function __construct() {}
    public static function build($args) {
        // Set up PDO connection
        if(!isset($args)) {
            return NULL;
        } else {
            $statement = 
                $args['dbType'].':'.
                'host='.$args['host'].';'.
                'dbname='.$args['dbName'];
            try {
                $dbh = new PDO(
                    $statement, 
                    $args['user'], 
                    $args['pass']);
                $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
                // Return DBH
                return $dbh;
            } catch (PDOException $e) {
                echo 'Error: ',  $e->getMessage(), "\n";
            }
        }
        return NULL;
    }
}

And how to use the factories:

class demo {
    private $dbh = NULL;
    public function set($args) {
        if (!isset($args)) {return FALSE;} 
        else {
            $this->dbh = $args;
            return TRUE;
        }
    }
    public function __construct($args = NULL) {
        $this->set($args);
    }

    public function doSomething() {
        // Set SQL command here.
        $sql = 'SELECT * FROM test';

        $results = $this->dbh->prepare($sql);
        // Perform param binds here.
        $results->execute();
        return $results;
    }
}

$db = PDOConnection_Factory::build($settings);

$test = new demo($db);
foreach ($test->doSomething() as $row) {print_r($row[0].PHP_EOL);}
?>
share|improve this question
add comment

2 Answers 2

up vote 1 down vote accepted

demo should be Demo and in new classes you might want to use namespaces instead of underscores in your class names. See PSR-0

As non of your $args is optional and there are only 5 I would recommend the use real method parameters instead of this array. Otherwise some validation and documentation of this array is missing.

Furthermore you could refactor your ifs to real guard conditions and skip the else branch. This makes the code more readable and you get rid of a indentation level. There are also no braces on one-line-ifs.

Edit Sample for guard conditions

class PDOConnection_Factory {
    private function __construct() {}
    public static function build($args) {
        if (!isset($args)) return NULL;
        $statement = ...
        try {
            $dbh = ...
            return $dbh;
        } catch (PDOException $e) {
            //as Cygal stated this is not very nice
            echo 'Error: ',  $e->getMessage(), "\n";
            return NULL;
        }
    }
}

Edit 2 Handle many and optimal method/constructor parameter

First the usage:

new PDOConnectionBuilder().name("test").user("guest").password("1234").create();

And now the details:

class PDOConnectionBuilder()
{
    private type="mysql"; //defaults
    private name;
    private host="localhost";
    private user;
    private password;

    public function create()
    {
        //guards again
        if (empty($name)) throw new PDOConnectionBuilderException("Name is not optional");
        if (empty($user)) throw ...
        if (empty($password)) $passwordToUse=$user; //fallback to default
        else $passwordToUse=$password;

        //do the required stuff
        return $connection;
    }

    public function name ($name)
    {
        $this->name=$name;
        return $this;
    }
    //...
}   

This is a really awesome pattern for building complex object in larger system and in tests. You could even reuse the builder and call create twice. Or change only one property and call create again.

share|improve this answer
    
While I agree and upvoted for the first two paragraphs, I don't think removing braces for one line ifs is such a good idea. @Adam: If it isn't clear, "guard conditions" means that you can return early in PDOConnection_Factory::build() and get rid of the else keyword. –  Quentin Pradet Feb 11 '13 at 7:49
    
My comment only holds for real one-lines as if (!isset($args)) {return FALSE;} , as soon as there is a line break you "have to" add braces. –  mnhg Feb 11 '13 at 8:17
    
@mnhg, Thanks for the responses, I'll update the code to reflect the changes. I have a question about the $args being set as optional. I tried to make the settings factory args as optional as possible, trying to keep the arguments as clean as possible without hard-coding them because I am still not sure how many arguments I will have, what I will do with all of them, and I don't want to force the user to input an argument to get to the next one. Also, for tooltips, I'd prefer to show that you can/should put them in. Could you show me what you mean by refactoring the guard conditions? –  Adam Feb 11 '13 at 8:52
    
@mnhg right, sorry. –  Quentin Pradet Feb 11 '13 at 9:06
    
Actually you don't have optional arguments. So no need to start an optimization here. If you want to be prepared for the future please check my updated post about the builder pattern. –  mnhg Feb 11 '13 at 9:21
add comment

Formatting strings

You should try to avoid those kind of things:

$statement = $args['dbType'].':'.'host='.$args['host'].';'.'dbname='.$args['dbName'];

It's easy to miss something, and it's also this kind of constructions that leads to difficult to read code and (in other contexts) SQL injections. I would argue that using sprintf is a better idea here:

$format = '%s:host=%s:dbname=%s';
echo sprintf($format, $args['dbType'], $args['host'], $args['dbName']);

I don't know if it is considered idiomatic PHP or not, but it seems more readable and less error-prone to me.

PDO and exceptions

mnhg already gave you a way to get rid of the first return NULL in PDOConnection_Factory::build(), but it's not enough, you should also remove the second one. I assume it comes from the possible exceptions in PDO::__construct and PDO::setAttribute. The issue is that you're catching the exception but doing nothing with them.

You should instead embrace exceptions. :) A good first step is the setAttribute call. Since there's nothing you can do to handle them in the factory: you should instead let the application code decide what to do if something fails: for example, display a warning message to users and send an email to the administrator.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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