There are a few things which I consider required. First of all I think your code does a good job. Below I have written some of the things I would also implement.
First of all. Be careful when echoing exception message directly. Especially database exceptions has a tendency to contain connection credentials. Imagine an error happens in your application and a malicious user sees this message. He could then log-in onto your database and wreck havoc! Try to keep the amount of sensitive information out of exception messages users are able to see.
From what I can see your class wraps a PDO instance to make certain tasks easier. When you wrap your PDO instance inside a class your lose some of PDO capabilities. As far as I can see from the code I have no way to perform a transaction. This may not be useful to you now due to an older MySql version, but when you sometimes in the future upgrade (I hope you do :D) this might be desired. I would provide a way to interact with the PDO instance from outside the class. A simple pdo() method would solve that problem.
/**
* Provides access to the application PDO instance.
*
* @return \PDO
*/
public function pdo() {
return $this->dbh;
}
When looking at your execute, resultset and single methods I see three flaws. The first being a potential double execution of your query. Imagine you sometime in the future call the execute method. Then you follow that by calling the resultset or single method. Now you have executed the query twice. This may not cause bugs when you are fetching data, but if the query is an insert statement, which populates an unique column, you will get an SQL error. I would remove the calls to the PDO execute method inside the resultset and single methods. This would effectively eliminate all possible bugs of this kind. Keep in mind you should also perform some error checking to see if the current statement has been executed or you would otherwise get an error. This could be solved by setting a flag indicating if the statement has been executed.
private $executed = false;
public function execute() {
$this->stmt->execute();
$this->executed = true; // Set the flag.
}
The second flaw is that you are unable to close the cursor for the current statement. By closing the cursor you gain the ability to execute the statement once more with different parameters. You would also gain some (small) performance increase on huge queries. Take a look at the documentation for the closeCursor method.
public function closeCursor() {
$this->stmt->closeCursor();
$this->executed = false; // Reset the execution flag.
}
The third flaw which I really think is required regards your resultset and single methods once more. By default they will fetch the result-set into an object. This is a good practice when writing OOP code, but often you will find yourself fetching result sets into specific classes. Consider passing the data directly into an User class or passing an object of StdClass into the user class, so the user class can fetch the data. By passing directly into the class you save some performance and make the code more readable. I would also change the code, so you are able to fetch the result set as an array. Sometimes this may be preferred and you cannot predict the future. You could change the method signature of resultset (and single too) to something like the following:
/**
* Fetch entire result-set of the current query.
*
* @param integer $mode A PDO constant declaring the fetch mode.
* @param string|null $class A qualified class name to fetch the result into.
* @param array $args Constructor arguments for the custom class.
*
* @return mixed Returns the result set according to the specified fetch mode.
* The default mode is to fetch an object of type StdClass.
*/
public function resultset($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {
if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {
return $this->stmt->fetchAll(PDO::FETCH_CLASS; $class, $args);
}
return $this->stmt->fetchAll($mode);
}
/**
* Fetch one row of the current query.
*
* @param integer $mode A PDO constant declaring the fetch mode.
* @param string|null $class A qualified class name to fetch the result into.
* @param array $args Constructor arguments for the custom class.
*
* @return mixed Returns the result set according to the specified fetch mode.
* The default mode is to fetch an object of type StdClass.
*/
public function single($mode = PDO::FETCH_OBJ, $class = null, array $args = []) {
if(!is_null($class) && in_array($mode, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {
return $this->stmt->fetchObject($class, $args);
}
return $this->stmt->fetch($mode);
}
There is also another thing which I think is important. You are creating your PDO connection in the constructor effectively making the parameters for the connection hardcoded (or so it looks from $this->user due to lack of a constructor argument). I would extract the connection functionality into a separate class. This will also allow you to use different connections with the same database class. I would write something like the following. First I would alter the constructor in your current class.
public function __construct(PDOConnection $connection) {
$this->dbh = $connection->getConnection();
}
Then I would write the PDOConnection class.
class PDOConnection {
private $instance;
private $dsn;
private $username;
private $password;
private $options = [];
public function __construct($dsn, $username, $password, array $options = []) {
$this->dsn = $dsn;
$this->username = $username;
$this->password = $password;
$this->options = $options;
}
public function setAttribute($name, $value) {
if(!$this->instance instanceof PDO) {
throw new LogicException('Cannot set PDO attribute. Please make sure you are connected using the connect() method.');
}
if($this->instance->setAttribute($name, $value) === false) {
throw new LogicException('Could not set PDO attribute: ' . $name);
}
}
public function setOption($name, $value) {
$this->options[$name] = $value;
}
public function getConnection() {
if(!$this->instance instanceof PDO) {
throw new LogicException('No database connection established.');
}
return $this->instance;
}
public function connect() {
try {
$this->instance = new PDO($this->dsn, $this->username, $this->password, $this->options);
}catch(PDOException $exception) {
throw new ErrorException('Could not connect to the database!', null, $exception);
}
}
public function disconnect() {
$this->instance = null;
}
}
Keep in mind this code has no error checking and not been tested, so there might be an error.
The usage of these classes would be like the following. I have named your class Database since I couldn't see the original name from your post.
$dsn = 'mysql:host=127.0.0.1;dbname=AWESOMENESS;charset=utf8';
$user = 'username';
$pass = 'password';
$connection = new PDOConnection($dsn, $user, $pass, [
PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
PDO::ATTR_CURSOR => PDO::CURSOR_FWDONLY, // Scrollable cursors may be unavailable or very expensive on MySql
PDO::ATTR_EMULATE_PREPARES => false,
PDO::ATTR_PERSISTENT => false
]);
$connection->connect();
$database = new Database($connection);
I hope this can help guide you in the right direction, happy coding!
