Small PHP DAO app

Question

I'm creating a small app and decided to write my own DAO for learning, etc.

At the moment it's in one big file, which I really don't like. Could you give me some tips on how:

• you would separate this into smaller classes and use abstraction, inheritance etc?
• how I can update the CRUD functions (they feel pretty "frumpy")?
• Any other advice/tips?

<?php
namespace App\Core\Framework;

use App;

class DAO
{
    CONST ASSOC = \PDO::FETCH_ASSOC;
    CONST BOTH = \PDO::FETCH_BOTH;
    CONST BOUND = \PDO::FETCH_BOUND;
    CONST LAZY = \PDO::FETCH_LAZY;
    CONST NAMED = \PDO::FETCH_NAMED;
    CONST NUM = \PDO::FETCH_NUM;
    CONST OBJ = \PDO::FETCH_OBJ;
    CONST COLUMN = \PDO::FETCH_COLUMN;

    private $dao = NULL;
    private $connection = false;
    private $host;
    private $username;
    private $password;
    private $database;
    private $type;
    private $connectionString;
    private $query;
    private $fetchType = self::ASSOC;

    public function __construct()
    {
        $this->_getConnectionDetails();
    }

    public function connect()
    {
        if(!$this->connection)
        {
            try {
                $options = array(
                    \PDO::ATTR_PERSISTENT => true,
                    \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION
                );

                $this->dao = new \PDO($this->connectionString,
                                        $this->username,
                                        $this->password,
                                        $options
                                    );
                $this->connection = true;
                return $this->connection;
            }
            catch (\PDOException $e)
            {
                echo $e->getMessage();
            }
        }
        return true;
    }

    protected function _getConnectionDetails()
    {
        $config = App\App::$library->getJson(LOCAL_CONFIG_PATH . DS . 'Local.json', true);
        $config = reset($config);

        foreach($config['db'] as $key => $value)
        {
            $this->$key = $value;
        }

        switch($config['extra']['db_type']){
            case "postgresql":
                $this->connectionString = "pgsql:host=" . $this->host . " dbname=" . $this->database;
                break;

            case "sqlite":
                $this->connectionString = "sqlite:" . $$this->db_path;
                break;

            default:
                $this->connectionString = "mysql:host=" . $this->host . ";dbname=" . $this->database;
                break;
        }

    }

    public function disconnect()
    {
        if($this->connection)
        {
            unset($this->dao);
            $this->connection = false;
            return true;
        }

        return false;
    }

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

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

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

    public function setFetchType($type)
    {
        $this->fetchType = $type;
    }

    public function select($table, $parameters = NULL, $entity = '*')
    {
        $where = $this->_prepareNamedParams($parameters);

        if($where)
        {
            $sql = ('SELECT ' . $entity . ' FROM ' . $table . ' WHERE ' . $where);
            $this->query = $this->dao->prepare($sql);

            foreach($parameters as $param => $value)
            {
                $this->_prepareBind($param,$value);
            }

        }
        else
        {
            $sql = ('SELECT ' . $entity . ' FROM ' . $table );
            $this->query = $this->dao->prepare($sql);
        }

        $this->query->execute();

    }

    public function insert($table, array $columns, array $values)
    {
        $column = '(' . implode(',', $columns) . ')';
        $params = $this->_prepareValues($values);

        $sql = ('INSERT INTO ' . $table . '' . $column . ' VALUES ' . $params);

        $this->query = $this->dao->prepare($sql);

        foreach($values as $param => $value)
        {
            $this->_prepareBind($param,$value);
        }

        $this->query->execute();

    }

    public function update($table, array $values, array $parameters)
    {
        $params = $this->_prepareNamedParams($values);
        $where = $this->_prepareNamedParams($parameters);

        $sql = ("UPDATE " . $table . " SET " . $params . " WHERE " . $where);

        $this->query = $this->dao->prepare($sql);

        foreach($parameters as $param => $value)
        {
            $this->_prepareBind($param,$value);
        }

        foreach($values as $param => $value)
        {
            $this->_prepareBind($param,$value);
        }

        $this->query->execute();
    }

    public function delete($table, array $parameters)
    {
        $where = $this->_prepareNamedParams($parameters);

        $sql = ('DELETE FROM ' . $table . ' WHERE ' . $where);

        $this->query = $this->dao->prepare($sql);

        foreach($parameters as $param => $value)
        {
            $this->_prepareBind($param,$value);
        }

        $this->query->execute();
    }

    public function fetchAll()
    {
        $result = $this->query->fetchAll($this->fetchType);
        return $result;
    }

    public function fetchRow()
    {
        $result = $this->query->fetch($this->fetchType);
        return $result;
    }

    public function fetchAssoc()
    {
        $data = array();
        while ($row = $this->query->fetch(self::ASSOC)) {
            $tmp = array_values(array_slice($row, 0, 1));
            $data[$tmp[0]] = $row;
        }
        return $data;
    }

    public function fetchCol($index)
    {
        $result = $this->query->fetchAll(self::COLUMN, $index);
        return $result;
    }

    public function fetchPairs()
    {
        $data = array();
        while ($row = $this->query->fetch(self::NUM)) {
            $data[$row[0]] = $row[1];
        }
        return $data;
    }

    public function fetchOne()
    {
        $result = $this->query->fetchColumn(0);
        return $result;
    }

    protected function _prepareNamedParams($parameters)
    {
        if(empty($parameters))
        {
            return false;
        }

        $params = array();

        foreach($parameters as $name => $value)
        {
            $params[] = $name . ' = :' . $name;
        }

        return implode(' AND ',$params);
    }

    protected function _prepareBind($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_NULL;
                    break;
                default:
                    $type = \PDO::PARAM_STR;
            }
        }
        $this->query->bindValue($param, $value, $type);
    }

    protected function _prepareValues($values)
    {
        $params = array();
        foreach($values as $key => $value)
        {
            $params[] = ':' . $key;
        }

        return '(' . implode(',',$params) . ')';
    }
}

Answer 1

You've made a nice and small DAO (Data Access Object). Small is good.

I don't have a complete answer to all your questions, but I have some remarks:

A: Extend, don't obscure

You've made the PDO object private in your class. This means you cannot access any of the functionality of the PDO object outside your class. Even running a simple direct query is not possible: PDO::query($statement). Why do this? It's better to extend the PDO class like this:

class MyPDO extends PDO
{
}

Then everything will still be accessible and you simply add to it. I would only add those things that clearly belong to the PDO class, like making a connection and transactions. A class should have only one job to do. (And also read the link, and the comments, at the bottom of this answer, why you should be careful adding things)

B: Lean and reusable classes

There would be an obvious way to split your class, in almost the same way as it was done for the PDO class: Database related and query related. You could write a seperate class to do queries. This allows you to keep a query and run it several times, binding other values to it. You would have to seperate creating the query, and executing it, into at least two methods. In my own DAO I have build a basic query class and extended that to have the CRUD functionality (Create, Read, Update and Delete):

class query
{
}

class selectQuery extends query
{
}

class insertQuery extends query
{
}

class updateQuery extends query
{
}

class deleteQuery extends query
{
}

This way I can put all common functionality in the query class. Doing a query can now looks like this:

$query = new selectQuery($handleDB,'items_table','ProductID, COUNT(*)');
$query->where("Trashed = 'false'")
      ->where('ProductID = :ProductID')
      ->where('Status = :Status')
      ->groupBy('ProductID');
$stock   = $query->execute(array('ProductID' => $productID,'Status' => 'Stock'));
$shipped = $query->execute(array('ProductID' => $productID,'Status' => 'Shipped'));

Note that I can add methods like where() and groupBy() by chaining them. Many arguments can be strings or arrays. A string like 'ProductID, COUNT(*)' is converted to an array, but could also have been: array('ProductID','COUNT(*)') or ['ProductID','COUNT(*)'] when your PHP can handle it.

C: Why do it?

You should only create a DAO class if you can add something to the existing PDO classes, not to simply wrap data access in your own class. That would be pointless. Does your class help you to prevent SQL bugs? Does it make constructing correct SQL statements easier? Does it do profiling? Does it provide shorter and better to read code? Add an extra layer of data protection? In other words: Why is your class better than what's already there? Your class is no good, if you cannot answer that question. Read and shiver: Class for reducing development time

Answer 2

Because KIKO already linked to a previous answer of mine, in which I tempt to explain why your class is really rather pointless (another link for completeness), I'll focus on your overall code quality and coding style.

Code quality

Overall, your code seems to be nicely put together. There aren't any major quality issues that jump out, apart from one. There are a few minor things that's I'd change, too

Big issue:
I'm pretty sure you've come across the SOLID principles before. Your DAO class violates one of these basic, yet important, principles: the Single Responsibility Principle. This states that each class has no more than one reason to change. A reason to change being: what a class represents, or what it manages. Take PDO as an example. A PDO instance represents a DB connection, a query is represented by a PDOStatement instance. Both do different jobs, both have their own reasons to change, and both are separate classes for that reason. Your class handles both the DB connection and the resultset.
Not only is this a violation of the SRP, but it's seriously limiting the use-cases for your code. Let's look at an example, using PDO

$db = new PDO($dsn, $user, $pass, [
    PDO::ATTR_ERRMODE           => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCHMODE => PDO::FETCH_OBJ
]);
$stmt = $db->prepare('SELECT field FROM db.table1 WHERE id > :id');
$stmt->execute([':id' => 12]);
try
{
    $db->beginTransaction();
    $instmt = $db->prepare(
        'INSERT INTO db.table2 (field_value) VALUES (:field)'
    );
    while ($row = $stmt->fetch())
    {
        $instmt->execute([':field' => $row->field]);
        $instmt->closeCursor();
    }
    $db->commit();
}
catch (PDOException $e)
{
    if ($db->inTransaction())
        $db->rollBack();
    throw new \RuntimeException(
        'Failed to insert data',
         0,
         $e
    );
}

As you can see, a single connection is being used, but we're handling 2 distinct prepared statements. Your code simply cannot handle this situation. You're also not calling PDOStatement::closeCursor anywhere (which might cause problems when using other supported DB adapters than mysql).
Another thing you may have seen is that I'm not echo-ing the error message. A class handling back-end logic (like a DB connection is) should not take care of output. That's up to the front-end code to handle. Which brings me to the less crucial errors in your code:

Classes don't echo, they return or throw
As I said above, your class simply shouldn't echo. If a DB class encounters an exceptional situation, it can't fix it. It might be caused by bad arguments being passed by the calling code. That code should be notified, not the end-user. Let the exception fly, or throw a new, custom exception and pass the caught exception as within that exception (As I have).

Names that lead to confusion
Your class is called DAO. That's not really a faux pas, but I'd prefer to see a clearer distinction between the classes and your single-word constants. Changing DAO to Dao would be a step in the right direction.
Your constants (ASSOC, NUM, OBJ, ...) are a bit shorter to write, that's true, but the FETCH_ prefix PDO uses actually means something. It signals that the constant affects the behaviour of PDOStatement::fetch and PDOStatement::fetchAll methods. I'd keep the prefix for that reason. I mean, comming accross code that uses your DAO class to query the DB, passing some constant DAO::OBJ as an argument merely obscures the fact that the argument will affect the way the resultset is returned.

Security, Security, Security:
Looking at your public function select($table, $parameters = NULL, $entity = '*'), I notice you're concatenating the $able and $entity arguments into the query string blindly, no checks whatsoever. You're not even making sure the arguments are strings. That's plain unsafe. If, for example, someone passes * FROM users; -- as $entity value, the resulting query will work just fine, but it's quite unsafe, I think you'd agree.

Customizing is made impossible:
At no point to you allow users to pass arguments to your class (through the constructor or connect) to set attributes that manage the connection. I'm talking about setting the PDO::ATTR_ERRMODE on the fly, or changing the PDO::ATTR_DEFAULT_FETCHMODE.
I know you can customize the connection (somewhat) through App\App::$library (a static property should start with an upper-case BTW). But this approach, having the usage of a json encoded config object in a specific place just doesn't cut it. Suppose you're working on a project that connects to multiple DB's? Your code would be a lot reusable by simply allowing the user to pass the connection parameters as arguments.

Coding style

On the front of coding style, all I have to say is that you should add some doc-blocks, and subscribe to the coding standards. Some violations I saw from the off were:

• capitalized keywords that needn't be capitalized (CONST should be const, NULL should be null).
• non-public methods (protected or private) do not need the old-school underscore marker (_getConnectionDetails => getConnectionDetails).
• see the link for other conventions/standards