Can I get feedback on this PHP function that tests if a user has signed up?

Question

I'm just getting started on writing functions instead of writing everything inline. Is this how a reusable function is typically written?

function test_user($user) {
$conn = get_db_conn();
$res = mysql_query("SELECT * FROM users WHERE uid = $user");
$row = mysql_fetch_assoc($res);
if (count($row) == 1) {
return true;
}
else {
    return false;
}
}

When someone logs in, I have their UID. I want to see if that's in the DB already. It's basic logic will be used in a

"If exists, display preferences, if !exists, display signup box" sort of flow. Obviously it's dependent on how it's used in the rest of the code, but will this work as advertised and have I fallen for any pitfalls? Thanks!

Answer 1

That is reuseable, yes. You may want to consider moving the SQL out of the PHP code itself.

Although you weren't asking for optimization necessarily, you might want to consider querying for the user's display preferences (which I assume are stored in the DB) and if it comes back empty, display the signup box. You'll save a trip to the database and depending on your traffic, that could be huge. If you decide to keep this implementation, I would suggest only selecting one column from the database in your SELECT. As long as you don't care about the data, there's no reason to fetch every single column.

Answer 2

Try this:

$conn = get_db_conn(); # should reuse a connection if it exists

# Have MySQL count the rows, instead of fetching a list (also prevent injection)
$res = mysql_query(sprintf("SELECT COUNT(*) FROM users WHERE uid=%d", $user));

# if the query fails
if (!$res) return false;

# explode the result
list($count) = mysql_fetch_row($res);
return ($count === '1');

Thoughts:

• You'll want better handling of a failed query, since return false means the user doesn't already exist.

• Use the database to count, it'll be faster.

• I'm assuming uid is an integer in the sprintf statement. This is now safe for user input.

• If you have an if statement that looks like if (something) { true } else { false } you should collapse it to just return something.

HTH

Answer 3

First off, you need to call

$user = mysql_real_escape_string($user);

because there's an sql injection bug in your code, see the manual. Second, you can simplify your logic by changing your query to:

SELECT COUNT(1) FROM user WHERE uid = $user;

which just lets you evaluate a single return value from $row. Last thing, once you have the basics of php down, consider looking at a php framework. They can cause you trouble and won't make you write good code, but they likely will save you a lot of work.

Answer 4

Indent! Overall it looks not bad...check the comments..

function test_user($user)
{
   $conn = get_db_conn(); //this should be done only once. Maybe somewhere else...?
   $res = mysql_query("SELECT uid FROM users WHERE uid = $user");
   $row = mysql_fetch_assoc($res);
  //I can't remember...can you return count($row) and have that forced to boolean ala C? It would reduce lines of code and make it easier to read.
   if (count($row) == 1) {
      return true;
   }
   else {
      return false;
   }
}

Answer 5

Also,

if (condition) {
    return true;
}
else {
    return false;
}

can be rewritten as:

return condition;

which saves quite a bit of typing and reading :)