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.

i created a database class from a good tutorial and wanted to put it up here so it would get in some search results. it took me about 2 days to find it. also i added a few custom functions to it.. here it is :P and if there is something that can be done better or more proficiently please feel free to let me know.

config.php:

// Database Constants
defined('DB_HOST') ? NULL : define('DB_HOST', 'edit:host');
defined('DB_USER') ? NULL : define('DB_USER', 'edit:user');
defined('DB_PASS') ? NULL : define('DB_PASS', 'edit:pass');
defined('DB_NAME') ? NULL : define('DB_NAME', 'edit:databasename');

database.class.php:

class Database {

private $dbhost = DB_HOST;
private $dbuser = DB_USER;
private $dbpass = DB_PASS;
private $dbname = DB_NAME;
private $dbh;
private $error;
private $stmt;

public function __construct() {
    // set DSN
    $dsn = 'mysql:host=' . $this->dbhost . ';dbname=' . $this->dbname;
    // set OPTIONS
    $options = array(
        PDO::ATTR_PERSISTENT => TRUE,
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
    );
    // Create a new PDO instance
    try {
        $this->dbh = new PDO($dsn, $this->dbuser, $this->dbpass, $options);
    } catch (PDOException $e) {
        $this->error = $e->getMessage();
    }
}

public function query($query) {
    $this->stmt = $this->dbh->prepare($query);
}

public function selectQuery($table, $fields, $FieldToQuery, $value) {
    try {
        if ((gettype($fields) != 'array') || (gettype($value) != 'array')) {
            $fields = (array) $fields;
            $FieldToQuery = (array) $FieldToQuery;
            $value = (array) $value;
        }
        $holders = $FieldToQuery;
        for ($i = 0; $i < count($holders); $i++) {
            $holders[$i] = ':' . $holders[$i];
        }
        $array = array_combine($holders, $value);
        $query = 'SELECT ' . implode(',', $fields) . ' FROM ' . $table . ' WHERE ' .
         implode(',',$FieldToQuery) . ' = ' . implode(',', $holders);
        $this->query($query);
        $this->bindArray($array);
        $rows = $this->resultset();
        return $rows;
    } catch (PDOException $e) {
        $this->error = $e->getMessage();
    }
}

public function insertQuery($table, $fields, $values) {
    try {
        if ((gettype($fields) != 'array') || (gettype($values) != 'array')) {
            $fields = (array) $fields;
            $values = (array) $values;
        }
        $holders = $fields;
        for ($i = 0; $i < count($holders); $i++) {
            $holders[$i] = ':' . $holders[$i];
        }
        $array = array_combine($holders, $values);
        $query = 'INSERT INTO ' . $table . '(' . implode(',', $fields)
                . ') VALUES (' . implode(',', $holders) . ')';
        $this->query($query);
        $this->bindArray($array);
        $this->execute();
    } catch (PDOException $e) {
        $this->error = $e->getMessage();
    }
}

public function bindArray($array) {
    foreach ($array as $key => $value) {
        $this->bind($key, $value);
    }
}

public function bind($param, $value, $type = null) {
    if (is_null($type)) {
        switch (true) {
            case is_int($value):
                $type = PDO::PARAM_INT;
                break;
            case is_bool($value):
                $type = PDO::PARAM_BOOL;
                break;
            case is_null($value):
                $type = PDO::PARAM_STR;
        }
    }
    $this->stmt->bindValue($param, $value, $type);
}

public function execute() {
    $this->stmt->execute();
}

public function resultset() {
    $this->execute();
    return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
}

public function single() {
    $this->execute();
    return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
}

public function rowCount() {
    return $this->stmt->rowCount();
}

public function lastInsertId() {
    return $this->dbh->lastInsertId();
}

public function beginTransaction() {
    return $this->dbh->beginTransaction();
}

public function endTransaction() {
    return $this->dbh->commit();
}

public function cancelTransaction() {
    return $this->dbh->rollBack();
}

public function debugDumpParams() {
    return $this->stmt->debugDumpParams();
}

}

here is the link http://culttt.com/2012/10/01/roll-your-own-pdo-php-class/

share|improve this question
    
Code Review is meant for reviewing code that you wrote, not code that you found on the internet. –  svick Feb 14 '13 at 2:39
    
@svick: Looks like it's only partly tutorial code. The bindArray, selectQuery, and insertQuery methods look original. –  cHao Feb 14 '13 at 2:56
1  
@TraeMoore: Please don't change your code too much once you've posted the question; it makes the answers look like people are seeing things. :) –  cHao Feb 14 '13 at 20:51

2 Answers 2

The constructor is basically always creating the same instances, so you are not really using the class as an object. I would rather create a database class that can be used like:

$db = new Database('localhost', 'root', 'root', 'dbname');

and then instantiate it as:

$db = new Database(DB_HOST, DB_USER ...);

which will prevent you from having to refactor your class if in the future when you'll need to connect to multi databases.

Also you are using $this->stmt as the current statement just to save the:

$stmt = $pdo->prepare('...');

which doesn't really make any sense and just reconvert the OOP of PDO back to the functional way of programming.

This:

public function rowCount() {
    return $this->stmt->rowCount();
}

public function lastInsertId() {
    return $this->dbh->lastInsertId();
}

public function beginTransaction() {
    return $this->dbh->beginTransaction();
}

public function endTransaction() {
    return $this->dbh->commit();
}

public function cancelTransaction() {
    return $this->dbh->rollBack();
}

public function debugDumpParams() {
    return $this->stmt->debugDumpParams();
}

is just useless and unmaintainable. Why don't you extend the PDO class instead?

PDO provides a good error handling via Exceptions and you are taking it away by saving it to a private property that is never even called. Why do you wan't to hide errors? Why don't you just let PDOExceptions being thrown? Or why don't you create a:

class DatabaseException extends Exception {}

exception class?

tl;dr: think with objects!

share|improve this answer
    
@cHao no i didn't.. your crazy :P –  Trae Moore Feb 14 '13 at 20:23
    
@TraeMoore: I have no idea what you're talking about. :) –  cHao Feb 14 '13 at 20:25
    
@cHao while this is fun, maybe you could give me some pointers? the first time i tried to make this class i used extends and ended up with nothing... –  Trae Moore Feb 14 '13 at 20:30
    
@jeffrey isn't surrounding things in try and catch error catching? –  Trae Moore Feb 14 '13 at 20:41
    
@TraeMoore: In this case, it's error hiding. :P Callers will be expecting to catch exceptions (cause that's how every other method works), and in a handful of methods you're sticking error messages in some whole other place that (a) you don't let callers know to check, and (b) they wouldn't have access to check even if they knew. If you're not going to fix the problem, and don't have some cleanup to do, don't catch the exception. –  cHao Feb 14 '13 at 20:54

Those constants, DB_HOST etc, since there's no easy way to redefine them, pretty much ensure that you can only ever have one database per run of your app. Maybe this is an issue for you; maybe it's not. But it can be a showstopper if you have to use two databases. I'd at least accept args for host, db, user, and password, and maybe default to the DB_ constants if the args are not set. (Personally, i'd get rid of the constants altogether, but i can see some logic behind them...)

Your select query almost certainly fails for more than one field. The standard syntax is ...WHERE field1 = value1 AND field2 = value2... rather than WHERE field1, field2... = value1, value2....

While you're at it, you might consider using an associative array to replace the two separate ones. Maybe even for the insert as well. array('field1' => 'value1', 'field2' => value2, ...) is a bit less error-prone than having two separate arrays, and it looks like bindArray already expects such an array anyway.

Maybe it's just me, but i don't see any way to retrieve $error. Either provide a getter for it or just get rid of it altogether and let the exceptions propagate like you do with most of the other methods. If you decide to keep it, you need to make sure to let the caller know to look at the error. (The mostly-standard way of doing that is to return false.) But really, you should probably just let the caller catch the exception, for consistency's sake; they'll have to do so for the rest of your methods anyway, and they shouldn't have to wonder how you're handling errors this time (or scratch their heads when they handle errors just like they do everywhere else, and aren't getting an exception back, but stuff's not working).

As far as your config script, it looks like the conditional operator is being abused. It works, as define is a real function rather than a language construct...but it's still abuse. The conditional operator is made for deciding between two values, which is why the ? NULL : is there.

if (!defined('DB_HOST')) define('DB_HOST', 'hostname');

is easier to follow, and is even less to type. :P

share|improve this answer
    
After reading this i see exactly what your saying and am going to figure out how to fix it so that i can still keep what i have. I do see what your saying about error's piling up. i do need to implement some sort of err solution... Forgive me for not catching all corners, ive only been doing php for about 2 weeks. – Trae Moore 5 mins ago.. –  Trae Moore Feb 14 '13 at 20:23
    
You're forgiven... this time... :) Really, though, the point of this site is to bring up those details you hadn't thought about. As long as you consider our suggestions, it's all good. –  cHao Feb 15 '13 at 1:29

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.