Custom PHP MySQL database queries function - Suggestions for improvement or additions

Question

For my website needs I made a custom procedural MySQL function in PHP which can INSERT, UPDATE, SELECT or DELETE data from database. I would like a review and any suggestions for improvement or addition of new features.

function mysqli_database_query($sql, $values, $param = NULL, $result_param = NULL) {
    require "dbconn.script.php";
    // String to array conversion
    if (!is_array($values)) {
        $values_array = array($values);
    } else {
        $values_array = $values;
    }
    // Number of values checker and string of parameter types creation
    $number_of_values = count($values_array);
    if ($number_of_values > 1) {
        $param_types = implode("", array_map(function($val) { return gettype($val)[0]; }, $values_array));
    } else {
        $param_types = gettype($values_array[0])[0];
    }
    // SQL query execution
    $stmt = mysqli_stmt_init($conn);
    mysqli_stmt_prepare($stmt, $sql);
    mysqli_stmt_bind_param($stmt, $param_types, ...$values_array);
    mysqli_stmt_execute($stmt);
    // SQL query results management
    if (preg_match("/^SELECT.*/", $sql)) {
        $results = mysqli_stmt_get_result($stmt);
        # RETURN ASSOCIATIVE AND NUMERIC ARRAY COMBINED EVEN IF RESULT PARAMETER IS INVALID
        if ($result_param === NULL || $result_param !== "assoc" || $result_param !== "num" || $result_param !== "row") {
            return mysqli_fetch_array($results, MYSQLI_BOTH);
        # RETURN ASSOCIATIVE ARRAY
        } else if ($result_param === "assoc") {
            return mysqli_fetch_array($results, MYSQLI_ASSOC);
        # RETURN NUMERIC ARRAY
        } else if ($result_param === "num") {
            return mysqli_fetch_array($results, MYSQLI_NUM);
        # RETURN ONE ROW
        } else if ($result_param === "row") {
            if ($param === NULL || $param !== "id") {
                return mysqli_fetch_row($results);
            }
            # RETURN ONE ROW ID
            if ($param === "id") {
                $result = mysqli_fetch_row($results);
                return $result["id"];
            }  
        }
    }
    if (preg_match("/^INSERT.*/", $sql)) {
        # RETURN LAST INSERTED ID
        if ($param === "insert_id") {
            return mysqli_insert_id($conn);
        # RETURN NUMBER OF AFFECTED ROWS
        } else if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        } 
    }
    if (preg_match("/^UPDATE.*/", $sql)) {
        # RETURN NUMBER OF AFFECTED ROWS
        if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        }
    }
    if (preg_match("/^DELETE.*/", $sql)) {
        # RETURN NUMBER OF AFFECTED ROWS
        if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        }
    }
};

In your place I would add some usage examples, to demonstrate how this function is going to be used. — Your Common Sense
– Your Common Sense, Commented Jan 9, 2020 at 11:46

Community · Accepted Answer · 2020-06-10 13:24:26Z

This monster of a function is hard to use and hard to maintain. Besides, it is bloated with duplicated and/or unnecessary code. Last but not least, the way require "dbconn.script.php"; is called will make your code run slower and make some features (such as transactions) unavailable.

Too many connections

Your current code is connecting to the database every time it runs a query. You shouldn't do that. Always connect strictly once and then pass a connection variable as a function parameter.

Useless regexp

You already control the return type buy means of $param variable. No need to check the query type manually. It is useless and error prone. What if your query is CALL? SET? REPLACE?

Passing values as array is not that hard.

With your function you have a choice, whether to pass $values as array or not. It makes the code hard to read. At the same time adding two square brackets to your $values is not a big deal. So just just make $values an array mandatory.

Too much tasks for a function

The first question I must ask, why only a single function to perform so many tasks? All right, I can make it you are not familiar with OOP and don't want to use a class (which would be the best solution of course). But why not to make a set of functions, each serving for its own purpose? Why following that horrible practice of procedural mysqli when you have to write a helluvalot of words every time you calling a simple function (mysqli_stmt_get_result($stmt) vs. $stmt->get_result())?

Here is your intended function call

$rows = mysqli_database_query($sql, $id, "affected_rows");

compare it with with a code which is shorter but more meaningful and readable at the same time:

$rows = mysqli_delete($sql, [$id]);

A function name should be meaningful and tell you what does this function do. It should be a function name, not a parameter.

In your place I would create a set of functions. Say, a basic one that just performs a query

function mysqli_database_query($conn, $sql, $values, $return_result = true)
{
    $types = $types ?: str_repeat("s", count($params));
    $stmt = $conn->prepare($sql);
    $stmt->bind_param($types, ...$params);
    $stmt->execute();
    return ($return_result) ? $stmt->get_result() : $stmt;
}

and then add some helper functions for various tasks

function mysqli_delete($conn, $sql, $values)
{
    $stmt = mysqli_database_query($conn, $sql, $values, false);
    $return $stmt->affected_rows();
}
function mysqli_assoc($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $return $result->fetch_assoc();
}
function mysqli_all($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $return $result->fetch_all(MYSQLI_ASSOC);
}

you can use the following function instead of that awkward $param = "id" of yours, for any column, not only id. Just select the desired column, i.e. SELECT user_id FROM table ...:

function mysqli_cell($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $row = $return $result->fetch_row();
    return ($row) ? $row[0] : false;
}

Thank you for the detailed review of my code. I want to tell you that I am just a beginner in programming, so I still stick to the procedural PHP. I know a little about OOP. I have a few questions more: - I didn't quite understand what did you mean by passing values as array mandatory. If I have an array of values and I want to add them to my function with [] as you suggested, it would convert it to a multidimensional array that will not work in my case (I think). - I cannot grasp this: return ($return_result) $stmt->get_result() : $stmt;. Can you explain it to me? — Boris J.
– Boris J., Commented Jan 9, 2020 at 17:17
And one more thing. How do you call a custom function within another custom function if they are defined within same page? I only succeeded in this by having separate files and the I include file with function1 in file with function2 which uses function1. — Boris J.
– Boris J., Commented Jan 9, 2020 at 17:20
Thank you for the good questions. In your code you have a function that converts $values to array if it is not, if (!is_array($values)). What I suggest is to always send $values as array and thus remove this code. A $return_result code is a ternary operator you can Google it up. Based in this variable's value it tells a function to return either a statement or a result — Your Common Sense
– Your Common Sense, Commented Jan 9, 2020 at 17:28
Ok, I think I understand now. Can you explain me why did you decided to use this code $types = $types ?: str_repeat("s", count($params)); for generating variable types, instead of my version - $param_types = implode("", array_map(function($val) { return gettype($val)[0]; }, $values_array));? — Boris J.
– Boris J., Commented Jan 9, 2020 at 17:36
MySQL will gladly accept all types as strings, no need for anything else. Besides, such a code like yours could lead to problems. I wrote about it several times, let me find it — Your Common Sense
– Your Common Sense, Commented Jan 9, 2020 at 17:37

Stack Exchange Network

Custom PHP MySQL database queries function - Suggestions for improvement or additions

1 Answer 1

Too many connections

Useless regexp

Passing values as array is not that hard.

Too much tasks for a function

You must log in to answer this question.

Hot Network Questions

Custom PHP MySQL database queries function - Suggestions for improvement or additions

1 Answer 1

Too many connections

Useless regexp

Passing values as array is not that hard.

Too much tasks for a function

You must log in to answer this question.

Related

Hot Network Questions