Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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.

Source Link

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
lang-php

AltStyle によって変換されたページ (->オリジナル) /