1
\$\begingroup\$

Here's a PHP PDO wrapper class that I wrote, along with example usage. This class features 2 primary methods for interacting with a database: Query(), which returns a result set, and Execute(), which doesn't. The class and its public methods are PascalCase, protected/private properties and methods camelCase. I am interested in any feedback, constructive criticism, and seeing how this class can be improved. Thanks!

class Database
{
    const MYSQL = 1;
    const MSSQL = 2;

    protected $type;
    protected $pdo;
    protected $stmt;
    protected $errMode;

    public function GetPdo()
    {
        return $this->pdo;
    }

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

    public function GetType()
    {
        return $this->type;
    }

    public function __construct($type = self::MYSQL, $errMode = PDO::ERRMODE_EXCEPTION)
    {
        $this->type = $type;
        $this->errMode = $errMode;
    }

    public function Connect($host, $database, $user, $pass)
    {
        switch ($this->type)
        {
            case self::MSSQL:
                $conStr = 'sqlsrv:Server=' . $host . ';Database=' . $database;
                break;
            case self::MYSQL:
            default:
                $conStr = 'mysql:host=' . $host . ';dbname=' . $database;
                break;
        }

        $this->pdo = new PDO($conStr, $user, $pass);

        // Set PDO's error mode
        $this->pdo->setAttribute(PDO::ATTR_ERRMODE, $this->errMode);
    }

    public function Query($sql, $params = [], $singleRow = false, $resultType = PDO::FETCH_ASSOC)
    {
        return $this->dbCall(true, $sql, $params, $singleRow, $resultType);
    }

    public function Execute($sql, $params = [], $resultType = PDO::FETCH_ASSOC)
    {
        return $this->dbCall(false, $sql, $params, false, $resultType);
    }

    protected function dbCall($results, $sql, $params, $singleRow, $resultType)
    {
        $params = (array) $params;

        if ($params)
        {
            $stmt = $this->pdo->prepare($sql);

            // Check if first parameter is an array; if so, these are named parameters
            if (is_array($params[0]))
            {
                // Named parameters are in the format: [[ name => value ]]
                // e.g., [[':name', 'value']]
                foreach ($params as $name => $value)
                {
                    $stmt->bindValue($name, $value, $this->getParamType($value));
                }

                $stmt->execute();
            }
            else
            {
                $stmt->execute($params);
            }

            if (!$results)
            {
                $rows = $stmt->rowCount();
                $stmt = null;
            }
        }
        else
        {
            if ($results)
            {
                $stmt = $this->pdo->query($sql);
            }
            else
            {
                $rows = $this->pdo->exec($sql);
            }
        }

        if ($results)
        {
            $data = ($singleRow) ? $stmt->fetch($resultType) : $stmt->fetchAll($resultType);
            $stmt = null;
            return $data;
        }
        else
        {
            return $rows;
        }
    }

    public function PrepareStatement($sql)
    {
        $this->stmt = $this->db->prepare($sql);
        return $this;
    }

    public function ExecuteStatement($params = [])
    {
        $this->checkStmt();

        if (!is_array($params))
        {
            $params = [$params];
        }

        $this->stmt->execute($params);
        return $this;
    }

    public function BindParam($paramName = null, $paramValue = null, $paramType = null, $paramLength = null)
    {
        $this->checkStmt();
        $this->stmt->bindParam($paramName, $paramValue, $paramType, $paramLength);
        return $this;
    }

    public function Fetch($singleRow = false, $resultType = PDO::FETCH_ASSOC)
    {
        $this->checkStmt();
        return ($singleRow) ? $stmt->fetch($resultType) : $stmt->fetchAll($resultType);
    }

    public function ClearStatement()
    {
        $this->stmt = null;
        return $this;
    }

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

    public function Rollback()
    {
        $this->pdo->rollBack();
        return $this;
    }

    public function Commit()
    {
        $this->pdo->commit();
        return $this;
    }

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

    protected function getParamType($input)
    {
        if (is_bool($input))
        {
            return PDO::PARAM_BOOL;
        }

        if (is_int($input))
        {
            return PDO::PARAM_INT;
        }

        if ($input === null)
        {
            return PDO::PARAM_NULL;
        }

        return PDO::PARAM_STR;
    }

    private function checkStmt()
    {
        if (!$this->stmt)
        {
            throw new BadMethodCallException('No active statement');
        }
    }
}

Example usage:

try
{
    $Database = new Database(Database::MYSQL, PDO::ERRMODE_EXCEPTION);
    $Database->Connect('host', 'db', 'user', 'password');

    // SELECT query
    $data = $Database->Query('SELECT Col1, Col2 FROM Table ORDER BY Col2');
    foreach ($data as $row)
    {
        // $row['Col1'] ...
    }

    // DELETE, UPDATE, etc.
    $Database->Execute('DELETE FROM Table WHERE Col1 = ? AND Col2 = ?', [1, 'foo']);

    // Named parameters
    $sql = 'UPDATE Table SET Col2 = :col2 WHERE Col1 = :col1';
    $params = [
        [':col1', 4],
        [':col2', 'bar']
    ];
    $Database->Execute($sql, $params);

    // If more control is needed with the PDO stmt
    $Database
        ->PrepareStatement('SELECT Col2 FROM Table WHERE Col1 = ?')
        ->ExecuteStatement(1);
    $data = $Database->Fetch(true);
    $Database->ClearStatement();
    // $data['Col2']; ...
}
catch (Exception $e) { ... }
\$\endgroup\$
4
\$\begingroup\$

When I saw the title of this question, I immediately thought of one of my earlier reviews on this site. The first thing I said there, applies to your code, too: IMHO, all PDO wrappers are, essentially pointless

I'd recommend you read through the review I posted there, but for now, let's focus on your code here:

  • PascalCase: Please don't. I strongly recommend you read through the PSR/PHP-FIG coding standards, and follow them. Most projects follow these standards, and for good reason. If ever you want other people to work with your code, the last thing you want is for code style differences to become an obstacle. I personally like Allman style indentation and brackets, but PSR only allows for K&R, so I use K&R.
  • you're not just wrapping PDO, you're actually making it worse: prepared statements (PDOStatement) are re-usable. That's why they're a different class/object. Your wrapper violates the SRP (Single Responsibility Principle): It connects to the DB and prepares, executes, and fetches the results. All the while, you can't access the PDOStatement instance, so if I want to execute the same query in a loop, using your wrapper, I end up calling prepare for the same query over and over again.
  • No type-hints: If a method expects an argument to be of a certain type, use a type-hint: eg: dbCall expects $params to be an array, rather than casting it, just write protected function dbCall($results, $sql, array $params, ...). You can assign a default value (array $params = [] or allow for null values to be passed (array $params = null).
  • The way you're creating an instance of Database, and the way you establish a connection implies that the caller has the DB credentials, and knows what type of DB he's connecting to. What's the point in creating the dependency (PDO) internally, if you can just require the caller to pass the connection to the constructor? Take a look at the SOLID principles: Dependency injection and Inversion of Control is what I'm trying to get at here.
  • You're only allowing the caller to specify the PDO::ATTR_ERRMODE, whereas in reality: PDO has a lot more config parameters to modify the behaviour (default fetch mode, how to handle NULL, emulated prepared statements, ...). Again, if you require the caller to pass the PDO instance, they are able to set all these things your current implementation isn't allowing them to do. In cases like this, you have to ask Why would I want to use a restrictive wrapper like that, if I can do more by simply using PDO?
  • Your code needs more documentation: add docblocks! At the very least: add them to the public methods so IDE's can auto-complete. But really, add them to all methods.

From here on end, I guess I'll just be repeating things I already stated on other reviews (see the link at the top). It all boils down to this: PDO is a well established extension that offers a clean, and easy to use API out of the box. Wrapping it in anything that will not restrict the use of features, takes time and effort. Time and effort that is actually not likely to pay off, because you'll end up with a remarkably similar API to the one you're wrapping, only it'll be slower (because of the extra wrapping layer). The only time putting the effort in really pays off, is if you add more features, and build a fully fledged DBAL/ORM. This is even more work than just creating the wrapper, and it's work that has been done before: Doctrine, Propel, ... Are all well established, well supported, actively maintained and mature tools. Given that, you have to ask: Why spend time reinventing the wheel if you can learn to use the existing tools in a fraction of the time?

\$\endgroup\$
  • \$\begingroup\$ I find myself making very similar comments to this on almost every DB wrapper code review on this site. Thanks. You saved me some typing :) \$\endgroup\$ – Mike Brant Sep 16 '16 at 13:55
  • \$\begingroup\$ @MikeBrant: It does seem to be a common thing (DBAL wrappers)... It's hard to say whether or not wrapper-reviews should be marked as duplicates, though... maybe a subject best discussed on meta \$\endgroup\$ – Elias Van Ootegem Sep 16 '16 at 14:29
1
\$\begingroup\$

I will add a few thoughts beyond those in the excellent answer by @EliasVanOotegem

Typically the use cases one might consider for wrapping a very well-established DB abstraction library like PDO would be to do something such as:

  • Manage database connections. For example, vending out valid PDO objects (database connections) that some code logic would then use to inject the PDO dependency into classes/functions that need it. Also, you might commonly see singleton patterns applied around DB connections.
  • Provide ORM behaviors. As noted by Elias, there are some well-established PHP libraries that do this already.
  • Provide "natural language" query capabilities. I.e abstracting caller from needing to to write raw SQL. Again, there are well-established PHP libraries for this.

Your class doesn't really do any of these.

Other thoughts:

  • Watch line length. You should strive to keep your code under 80 characters per line to aid in readability.
  • camelCase and snake_case are most typically used in PHP, with the notable exception that PascalCase has pretty much become the de facto style for names used for classes, interfaces, traits, and namespaces (not for variable names representing object instances though).
  • In addition to type-hinting where available (a great reason to use OOP, as passed objects are easy to type-hint), you should also include parameter validation on all publicly accessible methods. Throw InvalidArgumentException, BadMethodCallException, or similar as appropriate to the validation failure. You want this validation to be the first thing that happens in this method/function you are writing so that you can fail fast, giving the caller whatever information it needs to decide what to do with the exception.
  • I would reinforce Elias' thoughts on adding comments in your code. Code without appropriate comments should be a non-starter.
\$\endgroup\$
  • \$\begingroup\$ And still, you ended up typing XD... +1 for the effort \$\endgroup\$ – Elias Van Ootegem Sep 16 '16 at 16:19

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.