Prepared Statements Function

Question

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 />";
      }
  }
  

Answer 1

• 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.)

Answer 2

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!