6
\$\begingroup\$

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)
\$\endgroup\$
  • 1
    \$\begingroup\$ Do you really use names like $myArray and $count in production code? I hope not. \$\endgroup\$ – Phrancis Dec 11 '15 at 3:22
  • \$\begingroup\$ I won't upbraid you for using the wrong bracket style ;-) but your code would be more legible with indentation (exactly three spaces, not tabs ;-) \$\endgroup\$ – Mawg Dec 11 '15 at 8:52
  • \$\begingroup\$ Please do not change the code in your question after answers have been posted. See What should I do when someone answers my question? \$\endgroup\$ – Phrancis Dec 11 '15 at 21:12
  • \$\begingroup\$ @Phrancis I'm only changing the variable names to make it more understandable. Well, you said the names doesn't make sense, so I changed them. \$\endgroup\$ – jessica Dec 11 '15 at 21:13
6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is by far, the best answer! It reduced my half page of code into literally 2 lines! Which was the answer I was looking for, instead of answers that use a different library, or an answer with just words and no codes. Thanks! Upvoted, and accepted! \$\endgroup\$ – jessica Dec 11 '15 at 21:04
  • \$\begingroup\$ Also, I don't understand the part about using assoc over row. If I'm looping through the array, wouldn't I have to know all the assoc keys, instead of just using auto increasing numbers? \$\endgroup\$ – jessica Dec 11 '15 at 21:06
  • \$\begingroup\$ As you can see, you're no longer looping through the values. You will now (for example) have $product_array[10]['name'] instead of $product_array[10][4]. If you'd read back code from half a year ago, you wouldnd't have the slightest idea what [4] is. Code should speak for itself \$\endgroup\$ – Martijn Dec 11 '15 at 21:37
  • \$\begingroup\$ OH! So THAT's how it works! \$\endgroup\$ – jessica Dec 11 '15 at 21:41
  • \$\begingroup\$ I'm going to add one more thing: I've been programming quite a while now, I've never needed a construction like this before. Selecting a complete dateset and placing that in an array is not very effecient. Just loop on the page where you need it. This is code where you will start to depend on, and then you're stuck with a system that's not really designed for maintainability. At this point it's a bit hard to explain, but further down the line of experience, you'll thank me :) \$\endgroup\$ – Martijn Dec 11 '15 at 21:44
8
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Can you tell me what dsn, and dbh stands for? \$\endgroup\$ – jessica Dec 11 '15 at 2:39
  • \$\begingroup\$ Those names are a matter of convention, not requirement, but DSN stands for "Data Source Name" and DBH is your DataBase Handle. You can, of course, call those variables whatever you want instead. \$\endgroup\$ – Darwin von Corax Dec 11 '15 at 2:42
  • \$\begingroup\$ So, it's like $connect. Also, why are you using, mysql:host, instead of mysqli:host? \$\endgroup\$ – jessica Dec 11 '15 at 2:44
  • \$\begingroup\$ Because we're not using mysqli. The mysql: bit is what tells PDO what kind of dbms to connect to; alternatives would be pgsql: for PostgreSQL, oci: for Oracle and so forth. From there, PDO automagically chooses the appropriate driver on its own. \$\endgroup\$ – Darwin von Corax Dec 11 '15 at 2:47
  • \$\begingroup\$ Never mind. My mistake. This works fine. \$\endgroup\$ – jessica Dec 11 '15 at 3:00
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I use a separate function to prevent SQL injection. \$\endgroup\$ – jessica Dec 11 '15 at 2:54
  • \$\begingroup\$ Yeah, I've used order by for so long in opposite ways, now that the opposite way is the correct way in my mind. \$\endgroup\$ – jessica Dec 11 '15 at 2:55
  • \$\begingroup\$ So you would be better off eliminating this function and your SQL-escaping function and using the mysqli library directly. \$\endgroup\$ – 200_success Dec 11 '15 at 2:56
  • \$\begingroup\$ Actually, using a prepared statement instead of your custom function would be much more robust at preventing injection attacks. \$\endgroup\$ – Darwin von Corax Dec 11 '15 at 2:57
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Also a good point. Source code is meant to be read by humans and should be written accordingly. \$\endgroup\$ – Darwin von Corax Dec 11 '15 at 3:17
  • \$\begingroup\$ I really like this (+1) so please don't think that I nit-pick if I suggest a few blank lines, to break it up into associated functional blocks and improve legibility \$\endgroup\$ – Mawg Dec 11 '15 at 9:10
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @Jessica: In that case, I hope you don't mind reading a lot. I've not seen any of the other reviewers mention your using the global keyword. I've tried to explain why this is wrong, and what you actually should do instead \$\endgroup\$ – Elias Van Ootegem Dec 12 '15 at 21:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.