3
\$\begingroup\$

I have written this function that surprisingly works. (I didn't really know what I was doing; just a little experiment.) It is supposed to count rows selected by a PDO query.

Is it well written? It works, but I am just asking, as it is my first more-complex function.

function countRows($sql, $parameters){
            require("include/db.php");

            $result = $db->prepare($sql); 
            foreach ($parameters as $key => $parameter) {
                    $result->bindParam($key, $parameter);
            }

            $result->execute(); 
            $number_of_rows = $result->fetchColumn();

            return $number_of_rows;

            }
\$\endgroup\$
2
\$\begingroup\$

require

Incorporating code by file inclusion is a bad idea — especially code that has side-effects. At the very least, db.php seems to have the side-effect of defining $db. It makes me suspicious of what other things happen in there.

Recommendations:

  • move the require() call outside countRows()
  • change it to require_once()
  • make the side-effects explicit
require_once('include/db.php');

function countRows($sql, $parameters) {
    $db = connect();    # connect() is defined in db.php
    …
}

Even that is problematic, though: you shouldn't open a new connection for each query. It's probably better to have the caller pass a database connection as a parameter.

function countRows($db, $sql, $parameters) {
    …
}

Purpose

This function is probably a bad idea to begin with. The database sends back a lot of data just so that your PHP code can retrieve the row count.

If you just want to know how many rows that satisfy some criterion exist, then you should run a SELECT count(…) FROM … query, and the database will tell you just the count.

On the other hand, if you are also interested in the data itself, then you should just note the size of the result set when you run a "real" query.

\$\endgroup\$

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.