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 written some code today to be able to dynamically execute prepared statements/parameterized queries.

I need a fresh pair of eyes to look through my code for eventual performance gains that can be made.

I've tested the code on up to 20 parameters without problems.

For formatting I use this: {0:i}, with position of argument and type of argument

class p_mysqli extends mysqli
{
    public function __construct($host = 'localhost', $user = null, $pass = null, $db = null, $port = null, $socket = null)
    {
        @parent::__construct($host, $user, $pass, $db, $port, $socket);
    }

    public function QueryParam($sql)
    {
        // Find all parameters to be bound
        preg_match_all("/{(\d{1,}:.)}/", $sql, $matches);

        $noStripped = $matches[0];
        $sql = str_replace($noStripped, "?", $sql);
        $params = $matches[1];

        $queryTypes = Array();
        $queryParams = Array();
        $statement = $this->stmt_init();
        $statement = $this->prepare($sql);
        if(!$statement)
            die("Error on prepare: " . $this->error);

        if(count($params) > 0)
        {
            // Get params and pop SQL-query from array
            // This is also the part I'm wondering about..
            $arguments = func_get_args();
            $arguments = array_reverse($arguments);
            array_pop($arguments);
            $arguments = array_reverse($arguments);

            for($i = 0; $i < count($params); $i++)
            {
                $lp = preg_split("/:/", $params[$i]);
                $queryTypes[] = $lp[1];
                $queryParams[] = &$arguments[$lp[0]];
            }

            $query = Array();
            $query[0] = join("", $queryTypes);
            $query = array_merge($query, $queryParams);

            $stateRet = call_user_func_array(array(&$statement, 'bind_param'), $query);

            if(!$stateRet)
                die("Error on parameterbindning: " . $statement->error);
        }
        $statement->execute() or die("Error on execute: " . $statement->errno);
        $ret = $statement->get_result();
        $statement->close();
        return $ret;
    }
}

Example usage:

$indb->queryParam("SELECT COUNT(*) counted_column_test FROM users WHERE userid != {0:i} AND disabled = {2:b} AND email = {1:s}", 1, "this@email.net", false);

Updated code:

class ParamMysqli
{
    public function __construct(mysqli $inDb)
    {
        $this->db = $inDb;
    }

    public $db = null;
    public function queryParam($sql)
    {
        preg_match_all("/{(\d{1,}:.)}/", $sql, $matches);

        $noStripped = $matches[0];
        $sql = str_replace($noStripped, "?", $sql);
        $params = $matches[1];

        $statement = $this->db->stmt_init();
        $statement = $this->db->prepare($sql);
            if(!$statement)
                die("Error at prepare: " . $this->db->error);

        if(count($params) > 0)
        {
            $queryTypes = '';
            $queryParams = Array();
            $arguments = func_get_args();
            array_shift($arguments);

            foreach($params as $param)
            {
                $lp = explode(":", $param);
                $queryTypes .= $lp[1];
                $queryParams[] = &$arguments[$lp[0]];
            }

            $query = Array();
            $query[0] = $queryTypes;
            $query = array_merge($query, $queryParams);

            $stateRet = call_user_func_array(array($statement, 'bind_param'), $query);

            if(!$stateRet)
                die("Error at parameterbindning: " . $statement->error);
        }
        $statement->execute() or die("Error at execute: " . $statement->error);
        $ret = $statement->get_result();
        $statement->close();
        return $ret;
    }
}

//Example usage is as before
$__db = new mysqli(HOST, USER, PASS, DB);
$indb = new ParamMysqli($__db);
$result = $indb->queryParam("SELECT * FROM users WHERE userid = {0:i}", 1);
share|improve this question
    
Post rolled back. Please don't update the original code based on answers; that will invalidate them. You may ask a new follow-up question at some point if you'd like further review. –  Jamal May 9 at 8:44
    
Oh, ok. Thanks, will note that for future posts! –  NoLifeKing May 9 at 8:45

2 Answers 2

up vote 2 down vote accepted

Ok, you've posted quite a lot of code, and I have a few issues with some of it, that haven't been addressed by the accepted answer.
As usual, I'll walk through your code, pointing out omissions, mistakes, things that could be improved... along the way.

class p_mysqli extends mysqli

Stop. Yes, I have some criticism concerning the very first statement already. First off, this is a bit of a hang-up of mine: Please, please stick to coding standards as much as you can.
A class starts with an UpperCase character. Non-static methods (like your QueryParam) start with a lower-case, and are camleCased. Read through the linked PSR files, and try to stick to the recommendations. PHP doesn't have an official standard yet, but the more the PHP-FIG standard is adopted and advocated, the more likely it'll become a standard.

The other issue I have with this class is that it extends a core class. I've given a lengthy review on this site of code that extended PDO. The bottom line is: Avoid extending a class that you don't own.

Onwards, inside your constructor, you have this:

@parent::__construct($host, $user, $pass, $db, $port, $socket);

2 things bug me here: If your constructor merely calls the parent constructor, why bother writing that constructor in the first place... Inheritance means that, if a method is not defined on the child, then the parent's method will be called instead. Creating a method that just forwards to the parent's method is adding noise, and overhead: each method call takes time and resources. Not that you'll notice it that much, but it's just so pointless.

The other thing that really gets my goat here, is the fact that you use the @ suppressor of death. Again, I've been quite vocal in my intense loathing of this operator before, here for example. An error/notice is trying to tell you something isn't quite right, don't ignore it: fix it.
What's worse: you seem to be trying to suppress any problems that might occur in the constructor of an object???? That is just all shades of wrong: if a class can't be instantiated, don't you agree that the user (the one attempting to create that instance) should be notified? If an exception is thrown in a constructor, that means something fundamentally bad has occurred, and you shouldn't even think about carrying on.

If you can't connect to the DB in a MySQL class, then that class, and all code that uses it, can't do its job. trying to suppress errors in that scenario is like a serial killer denying everything, whilst covered in blood, with a pile of bodies behind him, and him holding a sign saying "I did it, and I'm proud of it, too".

But I'm drifting off topic here, badly. Carrying on!
Basically, the whole class is, just like I said in the answer with the PDO class, quite pointless IMO. I do understand the need to clean up mysqli's messy API, but extending a messy API isn't the right way to do it. A far better approach would be to create a wrapper object, which can implement this version of your QueryParam method:

public function queryParam($query, array $params)
{//use assoc array!
    $queryTypes = array_values($params);
    $queryParams = array_keys($params);
    //if needs must, you can use variable variables:
    $$queryParam[0] = 123;//but avoid, at all cost, I'd just bind to:
    $params[$queryTypes[0]];
}

You can then call this method like so:

$instance->queryParam(
    'SELECT * FROM foo WHERE bar = ? AND zar = ? AND x = ?',
    array(
        213  => 'i',
        431  => 'i',
       'abc' => 's'
    )
);

Using the values as keys may seem a bit odd, and it is. It can also cause issues when two parameters have the same value. That's why I think it best to either make the $param array 2 dimensional, pass a third argument ($types), or simply use the types of the arguments as they are being passed:

$typeString = '';
foreach ($params as $value)
{
    switch (true)
    {
        case is_int($value):
            $typeString  .= 'i';
            break;
        case is_string($value):
            $typeString .= 's';
            break;
        default:
            throw new \InvalidArgumentException(
                gettype($value). ' is not a valid query-param type'
            );
    }
}

A shorter, but more dangerous approach would be:

foreach ($params as $p)
    $typeString .= substr(gettype($p),0,1);//gets first char of type-string

Or, the less optimal, but safer approach:

foreach ($params as $k => $v)
    $params[$k] = (string) $v;
//types are all strings, let MySQL cast to the right type
$typeString = str_repeat('s', count($params));

Some other bits and pieces:

Passing by reference is the default for objects, explicit passing by reference is actually deprecated. So this:

$stateRet = call_user_func_array(
    array(
        &$statement,//$statement is an object
        'bind_param'
    ),
    $query
);

works best if you write it like so:

$stateRet = call_user_func_array(
    array(
        $statement,
        'bind_param'
    ),
    $query
);

Get rid of that &.

Another oddity in your code is this:

$arguments = func_get_args();
array_shift($arguments);
foreach ($params as $param)
    $queryParams[] = &$arguments[$lp[0]];

Basically, you're assigning a reference to each and every one of the arguments you passed to the function (except the query string, of course).
What's the point, I wonder? $queryParams is, 99.99% of the time going to end up being exactly the same as $arguments, why not simply write:

$arguments = func_get_args();
$queryParams = array_slice($arguments, 1);

Unless your query "recycles" a particular value like so:

foo = {0:i} AND bar = {0:i}

Just using the $arguments array will work just fine. You also say you want your method to work like .NET's String.format. Well, the niggle I have with that is simply this: you're not working with .NET here. Different languages tend to require different approaches for similar problems: use the tools you have. Forcing a language to work like another language is like trying to use a plane as a boat: Yes, you can sort of make it work, but it's just over-complicating things, and you're almost certainly going to find out it doesn't work as well as it could have.

Update:
After seeing your example, here's how you could apply my suggestions, and still be able to do the same thing:

public function queryParam($query, array $params, array $types = null)
{//I've made $types optional to show 2 possible approaches
    if ($types === null)
    {
        $types = array();
        foreach ($params as $param)
            $types[] = substr(gettype($param)), 0, 1);//boolean -> b, integer -> i, string -> s
            //float or double both yield "double" type -> d, which is what you need
    }
    $typeString = implode('', $types);//make string
    //optional validation of typeString:
    if (preg_match('/[^bsdi]/', $typeString))
        throw new InvalidArgumentException('unknown data types used');
    //add to params:
    array_unshift($typeString, $params);
    $stmt = $this->mysqli->prepare($query);
    call_user_func_array(
        array($stmt, 'bind_param'),
        $param
    );
    //this call_user_func_array equates to:
    $stmt->bind_param($typeString, $param[0], $param[1], $param[2],...);
}
//usage
$indb->queryParam(
    "SELECT COUNT(*) counted_column_test
        FROM users
    WHERE userid != ?
      AND disabled = ?
      AND email = ?",
    array(1, "this@email.net", false)
);
//or
$indb->queryParam(
    "SELECT COUNT(*) counted_column_test
        FROM users
    WHERE userid != ?
      AND disabled = ?
      AND email = ?",
    array(1, "this@email.net", false),
    array('i', 's', 'b')//<--optional
);

Oh, and BTW: userid != ? works, but the <> operator is more common... Some would even use userid NOT IN (?) here. Just thought you might want to know

share|improve this answer
    
I'll look into making a wrapper instead.. But I have removed the constructor (I didn't know that the base constructor was called, thanks for telling :) and the & by the call_user_func_array. I appreciate the sincerity of your answer. –  NoLifeKing May 9 at 11:53
    
@NoLifeKing: Cheers, and you're welcome. I've since edited my answer a bit, to include alternatives to your way of binding the params and types. Using a regex is really not the best way to go here: function arguments are. And 3 function parameters isn't a lot, after all –  Elias Van Ootegem May 9 at 12:00
    
I updated with an usage example to show why I use regex. (I could always build a tokenizer :) –  NoLifeKing May 9 at 12:31
    
@NoLifeKing: Will add "my version" of the same thing to my answer –  Elias Van Ootegem May 9 at 12:59
1  
@NoLifeKing: you don't need that get_class bit in your constructor: the type-hint will cause an error if anything other than an instance of mysqli is passed as an argument. just write $this->db = $inDb;. The code you posted now is quite different from the original code, too. I'd suggest you post it as a separate question/review request. I will edit my answer, though, because I've just spotted a bug: &$arguments[$lp[0]]; is not the best thing to do here... –  Elias Van Ootegem May 9 at 13:54
  1. You use a very complicated way to extract the first element of an array. Just use array_shift instead.

    $query = array_shift($arguments);
    
  2. Use foreach instead of a for loop:

    foreach ($params as $param) {
        $lp = explode(":", $param); //I changed preg_split to explode, because you use a static delimiter
        $queryTypes[] = $lp[1];
        $queryParams[] = &$arguments[$lp[0]];
    }
    
share|improve this answer
    
Used the suggestions in my code. Thanks a bunch! –  NoLifeKing May 9 at 8:45

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.