Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I need some advice\suggestions on how to create\handle database connections for a project I'm working on. I'm creating a simple work order system for my company using PHP 5.4.3, right now there isn't any type of authentication as I'm not ready for that complexity.

So far I'm planning on the following structure:

class Db {
    private static $dbh = null;

    static function open_connection() {
        self::$dbh = new PDO(... }

    static function query($sql) {
        $result = self::$dbh->... }

    static function fetch_array($result){
        ...}
...

}

class Workorder extends Db {

    protected static $table_name = ...
    ...

    public $wonum;  
    ...

    function instantiate {
    //return workorder objects
    ...}

    fucntion findallwos {
    //find all work orders...
    ...}

...}

I think this will work fine for pages that I need to display all of the work orders or put in new work orders. But I have a few pages that require very simple queries, for example one of the reporting pages will just have a drop down list of the 3 technicians we have working for us, if I was using a global $dbh variable I could just do:

function create_dropdown () {
    GLOBAL $dbh;

    $sql = "select...";
    $sth = $dbh->prepare($sql);
    $sth->execute();
    $arry = $sth->fetchAll(PDO::FETCH_ASSOC);
    .....

But I'd rather stay away from global variables as eventually I would like to add future complexity to this site which might not work with globals. (Including multiple users with authentication etc).

My questions are:

  1. Should I not use the Db class as I have it designed, and instead use some sort of connection_factory class instead that passes out connections? (and not have my workorder class be an extension of anything)
  2. Should I just use static calls to the Db class to pull in simple queries? db::fetch_array...
  3. Do you have any other suggestions? I've come across a lot of different ways to go about this, but ideally I'd like to design this so that I don't have to completely recode everything when I get to the point where I'm adding multiple users with different permissions etc.
share|improve this question
    
You can use opensource classes code.google.com/p/digg/wiki/PDB – Imdad Jun 6 '12 at 5:45

First of all it seems that there isn't really much reason for your database class. If you are writing a wrapper over PDO that doesn't add anything to PDO, why not just use PDO directly? And if you do have some functions that extend PDO's functionality, then why not do just that:

class Db extends PDO {
   public function HeyPDOCantDoThisSoImWritingItMyself() {

   }        
}

Moving on to Workorder, that seems to be something of a database model. I'd urge against extending your database class directly, it would make much more sense to interject a model parent class in between:

class Model {
    protected $db;
    protected $tablename;

    public function __construct(Db $db, $tablename) {
        $this->setDatabase($db)
             ->setTablename($tablename);
    }

    public function setDatabase(DB $db) {
        $this->db = $db;

        return $this;
    }

    public function getDatabase() {
        return $this->db;
    }

    public function setTablename($tablename) {
        if(!is_string($tablename) || empty($tablename)) {
            throw new Exception("Table name must be a non empty string!");
        }

        $this->tablename = $tablename;

        return $this;
    }

    public function getTablename() {
        return $this->tablename;
    }        
}

class Workorder extends Model {    
    public function ThisIsAWorkorderSpecificFunction() {
    }    
}

This way you have a solid foundation for all your future models, and you don't have to re-write their common code next time you need a model. There are some things in there that you might be seeing for the first time, let's dissect them:

    $this->setDatabase($db)
         ->setTablename($tablename);

This is called method chaining, and it's just a fancy and perhaps more readable way of structuring your code. It works because both setDatabase() and setTablename() return the object at the end, via return $this;.

    public function __construct(Db $db, $tablename) {

What I've done here is completely forget about static and global, and instead went for dependency injection. There is nothing inherently wrong with using static and global, especially given PHP's procedural tradition, however they do tend to make errors harder to find, it's not always easy to now where stuff broke. Since you are going for an object oriented solution, dependency injection is the simplest solution.

    public function setDatabase(DB $db) {

Here, I'm type hinting that $db should be a Db object, if you pass anything else to it, it will produce an error. Unfortunately PHP doesn't currently support type hinting for scalar types, so for $tablename I have to check, just to make sure:

    public function setTablename($tablename) {
        if(!is_string($tablename) || empty($tablename)) {
            throw new Exception("Table name must be a non empty string!");
        }

If $tablename is not a string or it's an empty string, I throw an exception, as, well there isn't much we can do to solve the problem. Again, this is a failsafe check, that you'll probably never see failing, but better to be safe than sorry.

And since you mention multiple connections, your Db class could easily handle them, however you'd have to give up extending PDO directly:

class Db {
    protected $connections = array();

    public function setConnection(PDO $connection, $name = "default") {
        if(!is_string($name) || empty($name)) {
            throw new Exception("Table name must be a non empty string!");
        }    

        $this->connections[$name] = $connection;

        return $this;
    }

    public function getConnection($name = "default") {
        if(!is_string($name) || empty($name)) {
            throw new Exception("Table name must be a non empty string!");
        }    

        return
            isset($this->connections[$name])
                ? $this->connections[$name]
                : false;
    }    
}
share|improve this answer
    
good answer, IMHO Model should be an abstract class and i would rather use an abstract DB class and write a MySQLi adapter then using PDO when "changing the database within the project" could be an issue. – braunbaer Jun 6 '12 at 14:45
    
@braunbaer I went with PDO because the OP is using it, but yes, if you are only working with MySQL, MySQLi makes more sense. As for the abstract model, I thought about it but decided against it as the answer was getting complicated enough, not to mention tl;dr. But it's a good call. – Yannis Jun 6 '12 at 14:51

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.