Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've created a method which is useful for executing multiple queries, and it works perfectly. Is this a good way to handle this?

public function foo($rQuery)
{
    try {
        $this->done->beginTransaction();
        foreach($rQuery as $val) {
            $rSql = $this->done->prepare($val);
            $rSql->execute();
        }

        $commitCode = $this->done->commit();  
    }
    catch (PDOException $ex) {
        $errorInfo = "Ooops: ".$ex->getMessage();

        $roalBackCode = $this->done->rollBack();
    }

    return array('errorInfo' => $errorInfo, 'commitCode' => $commitCode, 'roalBackCode' => $roalBackCode);
}
share|improve this question

1 Answer 1

up vote 1 down vote accepted

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;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.