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.

Hello, CR :)

I've finally finished my universal query preparation function. Sorry about my previous post, I wasn't aware of the rules before.

Anyway, here is a working function. Any suggestions to make it better? I feel it could use some tweaking, but am not sure how I would go about it...

Variable examples/explanation

  • $db | the database connection
  • $query | ex) $query = "SELECT * FROM users WHERE username=?"
  • $operation | The operation you wish to perform (number of rows, insert data)
  • $type | the variable types in mysqli_stmt_bind_param (example: $type = "ssi")
  • $variables | the variables to replace the question marks with

    function manipulate($db, $query, $operation, $type, $variables)
    {
        //Check Number of Variables inputted
        $numVars = (int)strlen($type);
        //Seperate variables into an array
        $var = explode(",",$variables);
    
        //Handle Statement
        $stmt = mysqli_stmt_init($db);
        if(mysqli_stmt_prepare($stmt, $query)) {
            echo "<br /> QUERY PREPARED <br />";
            $x = 0;
            if( $numVars == 1 ) {
                mysqli_stmt_bind_param($stmt, $type, $var[0]);
            } else if ( $numVars == 2 ) {
                mysqli_stmt_bind_param($stmt, $type, $var[0], $var[1]);
            } else if ( $numVars == 3 ) {
                mysqli_stmt_bind_param($stmt, $type, $var[0], $var[1], $var[2]);
            } else if ( $numVars == 4 ) {
                mysqli_stmt_bind_param($stmt, $type, $var[0], $var[1], $var[2], $var[3]);
            } else if ( $numVars == 5 ) {
                mysqli_stmt_bind_param($stmt, $type, $var[0], $var[1], $var[2], $var[3], $var[4]);
            }
    
            mysqli_stmt_execute($stmt);
            mysqli_stmt_store_result($stmt);
    
            if($operation == "numRows" || $operation == "num_rows") {
                $output = mysqli_stmt_num_rows($stmt);
            } else {
                echo "Operation not supported";
            }
    
            mysqli_stmt_close($stmt);
            return $output;
    
        } else {
            echo mysqli_stmt_error($stmt);
            echo "<br /> QUERY DENIED <br />";
        }
    }
    
share|improve this question

2 Answers 2

up vote 2 down vote accepted
  • This is going to sound a bit harsh, but I don't see a purpose to this function. It offers a tiny bit of a shortcut, but it cripples a lot of functionality to do so.

    • I'm not very familiar with MySQLi (PDO seems to have a lot more advantages than MySQLi and only a few, insignificant disadvantages -- though PDO does not have a procedural API like MySQLi does), but it seems like the only thing this function ever returns is the number of rows? So this is a glorified num_rows() function?
  • debugging output should be removed, and errors should either be returned or thrown as exception. Directly printing errors is bad since the calling code then doesn't know that an error occurred.

  • $variables is a blatant anti pattern. If you want an array, use an array. What if one of your bindings needs to have a comma in it?

  • What if numVars is >= 6? Doesn't seem very flexible.

    • If you decide to keep this function, use call_user_func_array to handle a dynamic amount of parameters.
  • Supporting numRows and numRows would probably come back to be a pain. I'd stick with one or the other.

    • Or, better yet, use constants like MANIPULATE_OP_NUMROWS (like built in functions do).
  • manipulate is a vague name. Manipulate what? And manipulate how?

  • (int)strlen($type); strlen already returns an int. (And even if it didn't, you're using loose comparisons anyway.)

share|improve this answer
    
Thanks very much for all the feedback.I understand your confusion in the 'point of this function' as it is not complete yet (I haven't added the other functionalities such as INSERT, DELETE, etc. only num_rows so far).Thus, the point of this function was so I didn't have to keep preparing statements - it gets a tad bit annoying,I'm lazy.I don't know if OOP makes it any easier; however, procedural is easy for me and gets the job done. Also, can you explain your 4th point about $variables and array. Oh and <3 <3 <3 <3 <3 for call_user_func_array. I am finding it troublesome on how to use it :( –  Wulf Dec 29 '12 at 4:20
    
@Wulf I mean instead of passing in something like "a,b,c" just pass in something like array('a', 'b', 'c'). Otherwise how can you represent a,b as an atomic piece and not ['a', 'b']. As for call_user_func_array, it's just used to call a function with the array as unpacked args. Like: function f($x, $y) { return $x + $y; } $z = call_user_func_array('f', 3, 8);` That would have $z as 11 since $x = 3 and $y = 8 in the function call. –  Corbin Dec 29 '12 at 23:25
    
Thanks Corbin, i would've upvoted, but dont have the permissions currently. –  Wulf Dec 30 '12 at 3:08

I agree with everything Corbin said, so +1 to him, I just had a few things I wanted to add. Corbin had a pretty good explanation of why this function is unnecessary, as well as some good general advice, but he did not mention anything about the code itself. So I will limit myself to that aspect alone.

Type hinting will allow you to ensure that only the proper arguments are passed to your functions. In this case you could type hint the $db parameter as a MySQLi object and, if you take Corbin's advice, the $variables parameter as an array. This will then ensure that no mistakes, such as passing a PDO or MySQL object instead of a MySQLi object, can occur. If the wrong type of variable is injected, then it will fail.

function manipulate( MySQLi $db, $query, $operation, $type, Array $variables ) {

The internal comments should be unnecessary. If your code is self-documenting and following the core principles (DRY, Single Responsibility, etc...), then it should be obvious what is going on. Comments, such as these, merely add clutter and make your code less legible. If you need to add a comment, limit yourself to doccomments.

Your $x variable seems to be unused. This can cause confusion, so you should avoid adding or keeping variables you aren't using. If it is for a feature you haven't added yet, wait until you add the feature before adding the variable. That being said, be careful of adding too many features to a function. Following the Single Responsibility Principle means that a function should do just one thing. So be careful that you don't violate it.

First off, if you have a value that you want to apply multiple comparisons to, in this case $numVars, then you should use a switch statement. They are slightly faster, and a little cleaner.

switch( $numVars ) {
    case 1 :
        mysqli_stmt_bind_param($stmt, $type, $var[0]);
    break;

    //etc...
}

Second, these if/else statements, and even the switch I mentioned above, is unnecessary and redundant. These solutions violate the "Don't Repeat Yourself" (DRY) Princple. As the name implies, your code should not repeat itself. There are many ways to avoid violating this principle, but in this instance you could either use a loop, I don't think it possible in this case, or you can use the call_user_func_array() as Corbin mentioned. If you look at the documentation for bind_param() one of the top comments shows an example of this.

Your code is also violating the Arrow Anti-Pattern. This pattern usually illustrates code coming to points to create an arrow, but essentially it applies to any code that is heavily or unnecessarily indented. In this case you could reverse your first if statement and return early to avoid the most amount of indentation.

if( ! mysqli_stmt_prepare( $stmt, $query ) ) {
    return FALSE;//or throw error
}

//success start binding params

Hope this helps!

share|improve this answer
    
Thank you very much! –  Wulf Jan 9 '13 at 4:05

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.