Get user name function

Bonesnap

Hello all! 🙂

I wrote a small function that retrieves a user name based off a certain type of ID. I'm mostly wondering if this function would serve better with a SWITCH statement instead of the three IF statements. Would it even be worth it? My understanding is that each IF statement is evaluated before moving onto the next one, whereas with a SWITCH statement the variable is only evaluated once, and then compared to the cases.

Please let me know what you think, or if you see any other issues.

Thanks! 🙂

/**
 * Gets the user name.
 *
 * Gets the user name based off of a user, post or thread ID.
 * In the case of a thread ID, it's the ID of the first poster
 * in the thread.
 *
 * Acceptable arguments for $type: user, post or thread.
 *
 * @since version 2.0
 * @param int $id The ID.
 * @param string $type The type of ID. Default is user.
 * @return string|false Returns the user name or false on failure.
 */
function get_user_name($id, $type = 'user')
{
	if(!is_int((int)$id))
		return false;
	if(trim($type) == 'user')
	{
		if($user = mysql_fetch_assoc(mysql_query("SELECT user_name FROM users WHERE user_id = ".$id)))
			return $user['user_name'];
		return false;
	}
	if(trim($type) == 'post')
	{
		if($user = mysql_fetch_assoc(mysql_query("SELECT user_name FROM users NATURAL JOIN posts WHERE post_id = ".$id)))
			return $user['user_name'];
		return false;
	}
	if(trim($type) == 'thread')
	{
		if($user = mysql_fetch_assoc(mysql_query("SELECT MIN(post_id), user_name FROM threads NATURAL JOIN posts NATURAL JOIN users WHERE thread_id = ".$id)))
			return $user['user_name'];
	}
	return false;
}

bradgrafelman

First problem I see is this:

    if(!is_int((int)$id)) 
        return false;

You might as well say if(TRUE) since that conditional will always evaluate to TRUE; you're checking the value you just forced to be an integer by casting it is... an integer. What's more, you're checking the value after this forced type-casting, but you never actually store that value or use it anywhere; instead, you use the original value in $id that may not actually pass the is_int() check.

As for your if vs. switch arguement, you're right - multiple if() branches in a row will be evaluated individually. However, if only one of them should ever be executed at a time, then you shouldn't be using a bunch of if() statements but instead a single [man]if[/man]/[man]elseif[/man]/[man]else[/man] chain instead. That way, PHP will stop when it finds a match rather than evaluating the other conditionals.

In other words, it's really just a matter of style whether you use switch/case statements or just a series of if/elseif(s)/else statements instead.

traq

bradgrafelman;10986107 wrote:
First problem I see is this:
    if(!is_int((int)$id)) 
        return false; 
You might as well say if(TRUE) since that conditional will always evaluate to TRUE...

actually, wouldn't that always evaluate to FALSE? in either case, no, it doesn't do anything useful.

Bonesnap

bradgrafelman;10986107 wrote:
First problem I see is this:
    if(!is_int((int)$id)) 
        return false; 
You might as well say if(TRUE) since that conditional will always evaluate to TRUE; you're checking the value you just forced to be an integer by casting it is... an integer. What's more, you're checking the value after this forced type-casting, but you never actually store that value or use it anywhere; instead, you use the original value in $id that may not actually pass the is_int() check.

I knew there was something iffy about that.

Originally I wasn't type casting it, but it would always return false no matter what I did, and the variable I was passing it was always an integer. The only way I could get it to return true was to cast it.

Would this be sufficient:

$id = intval($id);
if(!is_int($id))
    return false;

bradgrafelman;10986107 wrote:
As for your if vs. switch arguement, you're right - multiple if() branches in a row will be evaluated individually. However, if only one of them should ever be executed at a time, then you shouldn't be using a bunch of if() statements but instead a single [man]if[/man]/[man]elseif[/man]/[man]else[/man] chain instead. That way, PHP will stop when it finds a match rather than evaluating the other conditionals.

But it won't evaluate all the conditionals since I have a return statement in each IF statement. Only one IF statement will be evaluated unless the value of $type is something that doesn't match.

bradgrafelman;10986107 wrote:
In other words, it's really just a matter of style whether you use switch/case statements or just a series of if/elseif(s)/else statements instead.

I personally prefer the SWITCH statement, but I'll use whatever is faster/better.

laserlight

Bonesnap wrote:
Would this be sufficient:

That would be the same as before, except that the compound expression was broken up. Guessing your intention here, I suggest [man]ctype_digit/man.

Bonesnap wrote:
But it won't evaluate all the conditionals since I have a return statement in each IF statement. Only one IF statement will be evaluated unless the value of $type is something that doesn't match.

That is true. However, it is better to be explicit.

Bonesnap

laserlight;10986134 wrote:
That would be the same as before, except that the compound expression was broken up. Guessing your intention here, I suggest [man]ctype_digit/man.

That still returns false. I think I am just going to remove it from the function and escape $id with mysql_real_escape_string.

laserlight;10986134 wrote:
That is true. However, it is better to be explicit.

In a more complex script I would agree, but I think it's simple enough that it's clear.

laserlight

Bonesnap wrote:
That still returns false. I think I am just going to remove it from the function and escape $id with mysql_real_escape_string.

Instead of changing your code, maybe you should check if your input really is what you think it is. If $id is supposed to be an integer, then using mysql_real_escape_string, while workable, is not ideal. If ctype_digit($id) returns false, then it means that $id is not an integral string, despite what you think.

Bonesnap

laserlight;10986189 wrote:
Instead of changing your code, maybe you should check if your input really is what you think it is. If $id is supposed to be an integer, then using mysql_real_escape_string, while workable, is not ideal. If ctype_digit($id) returns false, then it means that $id is not an integral string, despite what you think.

I know it's the correct information because the query returns exactly what it's supposed to.

If I echo out $id I get 7. Or whatever the case may be.

traq

Bonesnap;10986165 wrote:
That still returns false. I think I am just going to remove it from the function and escape $id with mysql_real_escape_string.

[FONT="monospace"]ctype_digit($id)[/font] returns false, or [FONT="monospace"]ctype_digit((int)$id)[/FONT] returns false?

if it's the former, maybe $id contains whitespace?
if it's the later, then it's supposed to be false. or, have you cast [FONT="monospace"]$id[/font] as an integer anywhere else in your script?

what does [FONT="monospace"]preg_match('/^{\d+$/',$id)[/font]} return?

Bonesnap

traq;10986277 wrote:
[FONT="monospace"]ctype_digit($id)[/font] returns false, or [FONT="monospace"]ctype_digit((int)$id)[/FONT] returns false?

if it's the former, maybe $id contains whitespace?
if it's the later, then it's supposed to be false. or, have you cast [FONT="monospace"]$id[/font] as an integer anywhere else in your script?

what does [FONT="monospace"]preg_match('/^{\d+$/',$id)[/font]} return?

It returns true.

I added:

if(!preg_match('/^\d+$/',$id))
    return false;

At the top where my original test was, and it passes.

traq

Bonesnap;10986279 wrote:
It returns true.

I added:
if(!preg_match('/^\d+$/',$id))
    return false;
At the top where my original test was, and it passes.

hmm.. [FONT="monospace"]ctype_digit()[/font] ought to pass as well, then, unless it really is an integer instead of a numeric string.

Your notes in the first post say $ID is an int. do you actually cast it as one? or is it pulled directly from a form submission? you could intentionally cast it as a string (coming from the form, it should be a string anyway), and then the ctype_digit() test should work.

(while preg_match() will work, I suspect it's slower than ctype_digit.)

Bonesnap

traq;10986282 wrote:
hmm.. [FONT="monospace"]ctype_digit()[/font] ought to pass as well, then, unless it really is an integer instead of a numeric string.

Your notes in the first post say $ID is an int. do you actually cast it as one? or is it pulled directly from a form submission? you could intentionally cast it as a string (coming from the form, it should be a string anyway), and then the ctype_digit() test should work.

(while preg_match() will work, I suspect it's slower than ctype_digit.)

Don't ask me what happened, but ctype_digit() works now. I think I might have messed up the IF statement when I tested it :o.

The function is used in some places where the $id is being pulled directly from the database (as an integer, of course), and in other places where it's coming from a URL.

Thanks for your input! 🙂

traq

In that case, you'd need to cast it (probably as a string would be best) before testing. ctype_digit returns false for integers.