current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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;

            }
share|improve this question

1 Answer 1

up vote 2 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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