I think this is going to be a first, but yes. If I had to work on a project, and come across this code, I'd be happy. The only thing I'd change, perhaps, is the name of the PDO instance: $this->done
doesn't say I am a DB connection, IMO. The catching of the exception is required here, but the fact that you're not re-throwing it, or throwing a new, more generic exception is debatable. I'll briefly explain why, but that's entirely up to you to decide
Other than that, I'd say you're pretty much on the money.
A couple of suggestions, though:
Type-hinting
You're assuming the user of your code will be so kind as to pass an array of strings to your function. He/she would have to look at your method to determine what arguments you're expecting. An easier way of doing this would be:
public function foo(array $queries)
{//this method clearly expects an array, pass something else, and it will crash
}
Strings
You're also assuming the array argument will be an array of strings, whereas there is no guarantee that'll be the case. If I were you, I'd do some checking/filtering:
$queries = array_filter($queries, 'is_string');
That will filter out any non-string elements from the argument array.
Notice: undefined variables
The array you're returning uses both the $commitCode
and $errorInfo
variables. Why? If the commitCode variable was set, then there will be no errorInfo. Include a return statement in the try block, or return true
. If there was an error, then return the errorInfo.
return $this->db->commit();
//in catch:
return array('errorInfo' => $e->getMessage(), 'rollback' => $this->db->rollBack());
Or better still, rethrow the exception possibly contained within a more generic/custom exception object. The caller should be allowed to handle exceptions. The caller should know what was passed to your method, therefore any resulting exception is his responsibility:
catch (PDOException $e)
{
throw new RuntimeException(
'Not commited: '. $this->db->rollBack(),
0,
$e
);
}
Prepared statements, no binds
Of course, this isn't your actual code, I take it, but you're creating prepared statements and then execute them with no (room for) binds.
If the query strings come from a trusted source, there's no need to create an instance of PDOStatement
, IMO:
foreach($queries as $query)
{
$this->db->exec($query);
}
will do, and it'll increase performance, too
Allow binds
You'll soon find this method lacking in usability, though. If I wanted to execute a number of SELECT * FROM tbl WHERE ...
or UPDATE
or INSERT
or any query with parameters for that matter, I can't use your method.
Don't worry, though, for that's an easy fix:
public function foo(array $queries)
{
try
{
foreach($queries as $query)
{
if (is_array($query))
{
$stmt = $this->db->prepare($query['prepare']);
$stmt->execute($query['bind']);
}
else
{
if (is_string($query)) $this->db->exec($query);
}
}
return $this->db->commit();
}
catch (PDOException $e)
{
return array(
'errorInfo' => $e->getMessage(),
'rollbackCode' => $this->db->rollBack()
);
}
}
That way, I'm able to call this method like so:
$instance->foo(
array(
'SELECT * FROM someTable',
array(
'prepare' => 'SELECT * FROM anotherTable WHERE someField = :field',
'bind' => array(':field' => $val)
)
);
Of course, this implementation still depends on the user being kind enough to pass the correct arrays to your method. If I were you, I'd create an "Argument class":
class QueryArgument
{
private $query = null;
private $bind = null;
public function __construct($query = null, array $bind = null)
{
if (is_array($query))
{//allow user to pass array('prepare' => '', 'bind')
$bind = isset($query['bind']) && is_array($query['bind']) ? $query['bind'] : null;
$query = isset($query['prepare']) ? $query['prepare'] : null;
}
$this->query = is_string($query) ? $query : null;
$this->bind = null;
}
//this is just to make life easier
public function getTransactionFormat()
{
if ($this->bind)
{
return array(
'prepare' => $this->query,
'bind' => $this->bind
);
}
return $this->query;
}
//implement getters and setters
public function setBind(array $bind)
{
$this->bind = $bind;
return $this;
}
public function getBind()
{
return $this->bind;
}
}
Now you can use this like so:
public function foo(array $queryArgs)
{
$queries = array_filter($queryArgs, array($this, 'filterQueries'));
try
{
foreach($queries as $query)
{//same as before
if (is_array($query)) {/* ... */}
else {/* ... */}
}
}
catch (PDOException $e) {}
}
//filter out only those elements that are instances of QueryArgument
//convert them to array or string
private function filterQueries($instance)
{
if ($instance instanceof QueryArgument) return $instance->getTransactionFormat();
return false;
}