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 another link for completeness), I'll focus on your overall code quality and coding style.
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.
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.
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 beconst
,NULL
should benull
). - non-public methods (
protected
orprivate
) do not need the old-school underscore marker (_getConnectionDetails
=>getConnectionDetails
). - see the link for other conventions/standards