Function to get rows from database

Question

I have a function that returns rows from my database, and it works fine, but I was told that it was poorly written, and there is a lot of unnecessary code in it. I'm wondering if there is a simpler, easier way to rewrite this function.

function gd($query) {

    global $connect;
    $fetch = mysqli_query($connect, $query);
    $count = mysqli_num_rows($fetch);

    while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM)) {

        $count --;
        $arrayCount = count($myArray);

        $tempArrayCount = 0;
        while($tempArrayCount < $arrayCount){
            $array[$count][$tempArrayCount]= $row[$tempArrayCount];
            $tempArrayCount++;
        } //end of while($tempArrayCount < $arrayCount)

    } //end of while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM)) 

    return $array;

} //end of function gd($query)

Answer 1

The answer by Darwin von Corax would be better, but doesnt help you understand why your current code is wrong:

• You use fetch_array() with the MYSQLI_NUM flag. Just use fetch_row() instead.
• You use $tempArrayCount++, I assume to set a new number for the key. If you want only want to save the values, use [] instead. ( $arr = array(); $arr[]='newValue')
• It seems you want an array, with an array with values. You now loop through all the fields to get them in an array, but you allready have them in an array, being $row. This makes $tempArrayCount and the while obsolete. Also makes the count() for the length unneeded.

After all that, this'll do the same as your code:

while( $row = mysqli_fetch_row($fetch) ){
    $array[]= $row;
} 
return $array;

Tip: Don't use fetch_row(), use fetch_assoc(). We're humans, this'll give you an array with keys that make sense, instead of numbers.

Answer 2

Your code will work, of course, but you go to a lot of trouble to grind the result set into an array when that capability is already available. Mysqli would provide the fetch_all() function if you had the mysqlnd drivers available; fortunately, PDO has provided it since PHP 5.1.

You will need to create a PDO connection with

$dsn = "mysql:host=" . $hostname . ";dbname=" . $dbname . ";";
$dbh = new PDO($dsn, $dbuser, $dbpass);

then your function would be

function gd($query)
{
    global $dbh;
    $result = $dbh->query($query);
    $array = $result->fetchAll(PDO::FETCH_NUM);  // fetchAll() does the same as most of your original code
    return $array;
}

Answer 3

This function copies the rows in reverse order. The resulting array would be weird if the query contained an ORDER BY clause.

The design of the function does not allow the query to be parameterized. Callers can either…

• execute canned queries
• perform SQL escaping manually (yuck)
• live with an SQL injection vulnerability (even worse)

As @Darwin points out, the function should probably be eliminated altogether.

Answer 4

Formatting

With zero functional code changes, your code is already made more legible with some simple formatting, which is important to the maintainer of your code:

/* TODO: Rename parameter $query to something more meanginful. */
function gd($query) { 
    global $connect;
    /* TODO: rename $fetch...? Fetch what? */
    $fetch = mysqli_query($connect, $query);
    /* TODO: Rename $count to something more meaningful */
    $count = mysqli_num_rows($fetch);
    while ($row = mysqli_fetch_array($fetch, MYSQLI_NUM)) {
        $count --;
        /* TODO: find better names for $arrayCount, $tempArrayCount */
        $arrayCount = count($myArray);
        $tempArrayCount = 0;
        while($tempArrayCount < $arrayCount){
            $array[$count][$tempArrayCount]= $row[$tempArrayCount];
            $tempArrayCount++;
        } 
        //end of while($tempArrayCount < $arrayCount)
    } //end of while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM)) 
    return $array;
} //end of function gd($query)

Better points have already been addressed in other answers, as for the code itself.

Answer 5

Ok, I basically agree with most (if not all) of the comments others have made so far. Although none of the current reviewers have mentioned what I'd consider to be the biggest problem with your code.

I'll go through your code sort of line-by-line, commenting on any issues I encounter:

function gd($query) {

Yup, I have a comment about this already. 2 even. For starters: gd is a very, very bad name. It probably stands for get data or something, but you can't expect people reading your code to know that if they see some code like $data = gd($query);. Queries tend to contain a WHERE clause, and that clause may look different depending on logic that resides elsewhere, so you could end up with messy code like this $array = gd(getQuery($data));. That just looks awful.

The other problem I have with the function name is that gd is actually the name of an existing PHP extension. If I see gd in PHP code, I immediatly think of images being generated or transformed. I don't think about MySQL queries.

The last thing I'll say about this is that you really should consider following the PSR coding standards. That means putting the opening { bracket on a line of its own

---

Moving on...

    global $connect;

This line is the biggest problem with your code. A function is an encapsulated piece of logic that do a single, relatively simple job that you're very likely to repeat. It should receive the tools and data required from the caller. A function should not be relying on a global variable being there, simply because a function cannot guarantee that this variable exists, let alone that that variable will hold a DB connection. In other words: your function should be expecting the caller to pass the DB connection as an argument. Whether you're using PDO or mysqli: the best thing about this is that you can actually type-hint for this in your function signature. I urge you to change the definition to something more like this:

function getData(mysqli $connect, $query)
{
    //function body here
}

Now this function is (somewhat) clearer about what it does, and what it needs to work. If I see this function, I know it requires a DB connection and a query. Looking at that, I'd be about 99% sure that this function will return an array.

But like I said, this, to me is the worst part of your code: global is a keyword that you can, and should, avoid 100% of the time. I've been writing PHP for over a decade, and I've never, ever come across a situation where I actually needed to use global. If you do, treat it as a sign of a design fault.

---

    $fetch = mysqli_query($connect, $query);
    $count = mysqli_num_rows($fetch);

Just a small comment: mysqli_query returns either false or an instance of the mysqli_result class (which already holds the rowcount). You're not checking for a false return value, and you're assigning a result resource to a variable called $fetch. That just doesn't look quite right to me.
You can also use a mysqli connection as an object (which it actually is). It saves you the bother of having to pass that $connect variable to the function. To me, it looks cleaner to write this:

if (($result = $connect->query($query)) === false) {
    //handle failed query, eg:
    throw new Exception($connect->error);
}
$count = $result->num_rows;//result holds all relevant info!

---

Next...

    while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM)) {

        $count --;
        $arrayCount = count($myArray);

        $tempArrayCount = 0;
        while($tempArrayCount < $arrayCount){
            $array[$count][$tempArrayCount]= $row[$tempArrayCount];
            $tempArrayCount++;
        }

    }

Ok, whenever you're nesting loops like this, you really have to ask yourself if there isn't a different way to do things. What you're actually doing AFAIK, is fetching the results, row by row, and adding them to an array. For some reason, you're decrementing the row count, and you're using these numeric values as keys. So the first result you fetch will have the highest index. Not sure what the advantage of this is (you could just as well use an ORDER BY x DESC or ORDER BY x ASC clause in your query, or even use array_reverse. Personally though, I wouldn't bother. A function, like I said earlier, does one thing. This function returns the results of a query as an array. It shouldn't be expected to "format" those results.

Be that as it may, there is a far shorter way to do get an array of rows. If you have the native driver enabled, you can just write this:

$array = $result->fetch_all(MYSQLI_NUM);//I'd recommend MYSQLI_ASSOC though...
//or even:
return $result->fetch_all(MYSQLI_ASSOC);//numeric array with assoc sub-arrays

To find out if you have mysqlnd enabled, just run php -i | grep mysqlnd and look for mysqlnd => enabled in the output.

If you don't have the native driver installed/enabled for some reason, you'll have to build the full array yourself, but it's a simple matter of:

$array = [];//empty array
while ($row = $result->fetch_assoc()) {
    //fetch_array or fetch_row are alternatives here, read the docs
    //on what each of these methods does
    $array[] = $row;//append row to array
}

---

Last bit:

    return $array;

}

Ok, what you could do here if mysqlnd is installed, as I've shown earlier, is simply fetch and return the results in one go:

return $result->fetch_all(MYSQLI_ASSOC);

But even so, I wouldn't, and here's why: even though PHP manages the resources for you, I firmly believe that it's good practice to clean up things after you're done with them. In this case: $result holds an instance of mysqli_result. This object has got a mysql resultset associated with it. If you don't need those resources anymore, you can free them using the mysqli_result::free method (mysqli_result::close is an alias of the same method if ever you encounter a close call), so what I'd do is either:

//no mysqlnd
$array = [];
while ($row = $result->fetch_assoc()) {
    $array[] = $row;
}
//free the resources manually
$result->free();
return $array;

//with mysqlnd
$array = $result->fetch_all(MYSQLI_ASSOC);
$result->free();
return $array;

Although it's been some time since I've actively used mysqli, when I last did, you sometimes really needed those free or close calls when you were calling stored procedures, especially if those SP's relied on MySQL session variables. Not doing so could give you an error about MySQL being out of sync, and you would get errors calling SP's until you freed the offending result resources.