Generalized Sql Query Function

Question

One thing I realized while going through code yesterday at my internship is that I run sql queries quite often. So I decided to write a function to keep all of the calls in one place and reduce the number of lines of code. It doesn't cover all possible sql calls but it does cover the basic calls I believe.

So my question is, are there any coding practices I could improve upon or have violated while writing this?

EDIT: I broke the function up into four separate functions. When it comes to PDO my bosses said they did that behind the scenes with each $db_function(there is a different sequence for each server they access) that is called. I'm simply preparing the string that they manipulate. Also I've always viewed associative arrays as field value pairs so I welcome any suggestions in alternative names.

    <?
     *  INPUT:
     *  (string) Db_function: Which sql function to call
     *  (string) Table: The table to use
     *  (string/array) Fields: The values to be manipulated along with valid sql statements/modifiers(e.g DISTINCT or aliases) , 
     *                         its a string for SELECT/DELETE or an array for INSERT/UPDATE.
     *  (string) Conditional: Statements that go at the end of the query such as WHERE, ORDER BY, JOIN etc.
     *  
     *  OUTPUT:
     *          TRUE for SELECT/DELETE or the query results for INSERT/UPDATE */

    function sql_Select($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "SELECT $Fields FROM $Table";

          if(!is_null($Conditional)){
                $sql .= $Conditional;
                return $Db_Function($sql,1,1,1);
            }
            return $Db_Function($sql,1,1,1);     
    }

    function sql_Update($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "UPDATE $Table SET";

             foreach($Fields as $field => $value){
                $sql .= is_numeric($value) ? " $field = $value , " : " $field = '$value' , " ;
                }
                $sql = preg_replace('/,$/', '', trim($sql)); //Removes the extra ',' 

         if(!is_null($Conditional)){
            $sql .= $Conditional;
            return $Db_Function($sql,0,0,0);
            }   
        return $Db_Function($sql,0,0,0);  
    }

    function sql_Insert($Db_Function,$Table, $Fields, $Conditional = NULL){

        $Field_Name = implode(',',array_keys($Fields));
        $sql = "INSERT INTO $Table ($Field_Name) VALUES ( ";

        foreach($Fields as $field => $value){
                        $sql .= is_numeric($value) ? " $value, " : " '$value', " ; 
                    }
                    $sql = preg_replace('/,$/', '', trim($sql)); //Removes the extra ',' 
                    $sql .= ')';

         if(!is_null($Conditional)){
            $sql .= $Conditional;
            return $Db_Function($sql,0,0,0);
            }   
        return $Db_Function($sql,0,0,0);
    }

    function sql_delete($Db_Function,$Table, $Fields, $Conditional = NULL){

        $sql = "DELETE FROM $Table";

          if(!is_null($Conditional)){
                $sql .= $Conditional;
                return $Db_Function($sql,0,0,0)
            }
            return $Db_Function($sql,0,0,0)     
    }

    ?>

Answer 1

Argggh, my eyes burn!

SQL Injections

You should understand SQL injections. This code is likely to promote SQL injections. Read up about prepared statements. It looks like you are probably passing in a mysql_* function as a string. This stops you from using prepared statements.

Please, don't use mysql_* functions in new code. They are no longer maintained and the deprecation process has begun on it. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

One Size Fits None

Your function is overly complex. It should be broken into methods for each action. This will remove the need for the big switch and if statements and make it more readable. Your code will be easier to test. This also causes you to have a variable function call $Db_Function($sql, 0, 0, 0);. It is easy to have errors by calling non-existent functions with this.

_{Response To Edit: It looks better with separate functions now.}

Method Naming

You really have an awful naming scheme. Is this dictated to you by your bosses? Personally I already think they are silly for wanting a class like this that would promote SQL Injections. You have a mixed pascal case name aaaa_Bbbb_Cccc? Sane choices are camelCase, UpperCamelCase, Pascal_Case, lower_pascal_case.

The methods should not have sql_ preceding them. Their usage should be like this:

$sql = new sql; // Or whatever your class is called.
$sql->insert(/* params go here */);

It should be obvious from the object that you create that you have an sql type object. The sql_ in front of the method names then just gets in the way.

Unused Parameter

The sql_delete method does not use the $Fields parameter and it should therefore be removed.

Magic Numbers

Magic 0's and 1's are a sign of poor design.

Consistency

Please find a consistent way to name variables. $sql, $fields, $value are in the minority. I dislike Pascal_Case for variables in PHP, but inconsistency is even worse.

Answer 2

Even with the changes, this new code still falls underneath all of the categories that Paul posted earlier. On top of this, I would add the extra following problems:

• How is this code testable?

• If a new person comes along, how is this easy to understand? TRUE for SELECT/DELETE or the query results for INSERT/UPDATE? Logic like this would confuse me immensely.

• This is a very PHP 4ish way of going about solving a problem. Have you considered moving towards an OOP approach?