Forum Moderators: coopster
Problem is, for a user of only 1 picture, it outputs 2 pictures (the same picture twice). Any ideas as to what I am doing wrong?
Here's the code:
function getUserPicName($myusername) //get user pics
{
$dbConn = new dbConnection();
$dbConn->connect();$username = '';
$picName = '';
$picCount = 0;
$value = '';
$username = mysql_real_escape_string(stripslashes($myusername));
$picCount = $dbConn->getUserPicCount($username);
if($picCount == 0){//Don't do anything if picCount is = 0
}else if($picCount >0){
$sql = "SELECT picName FROM userpictures WHERE username='$username'";
$result = mysql_query($sql) or die(mysql_error());
$count=mysql_num_rows($result) or die("Could not get picName.");
if(!empty($count)){
while($info=mysql_fetch_array($result))
{
$myProfilePic = "<img src='(path to user pic)'/>";
$this->value .= '
<div id="imageContainer">
<div id="selectImage" class="smallTxt"><p>'.$picName.'</p></div>
<div id="showImage">
<p>'.$myProfilePic.'</p>
</div>
</div>';
}
return $this->value;
}else{die("Could not find a picName value");}
}
}
By the way, I also tried using a for loop, but same the thing happened.
Thanks for the help!
Well, I just checked the mysql query and for a user with 2 pictures. It returned just that (2 pictures).
I think I am screwing up on the logic somewhere. I am going to try and rewrite the whole thing from scratch and see if I catch/fix the error. In the meantime, if anybody sees my error, please point it out.
Maybe you meant to set
$this->value = '';
near the top of the function instead of
$value = '';
Hope this helps.
By the way, care to point out those "other potential bugs" in my code. I am up for some criticism:)Thanks.
- $picName, you seem to initialize to '' and never set it again. But then you use it inside the loop.
- $picCount, you're checking the number of pics twice + fetching all the pics in the loop. Best is to calculate picCount in only one way so you don't confuse yourself. Personally, i'd forget about the picCount, get rid of all the if statements, and just fetch the rows:
[pre]$this->value = '';
$picCount = 0; // only if you need to use it later.
$sql = "SELECT picName FROM userpictures WHERE username='$username'";
$result = mysql_query($sql) or die(mysql_error());
while($info=mysql_fetch_array($result)) {
$picCount++; // only if you need to use it later.
...
$this->value .= 'whatever';
}[/pre]Return value, you return something when there are pics, nothing when there aren't.
- HTML code, you enclose some attribute values in single-quotes (src=), and others in double-quotes. Pick one style and use it everywhere. Personally, i use double-quotes inside HTML because both htmlspecialchars() and htmlentities() encode that one without any extra flags.
- mysql_real_escape_string(stripslashes(...)), that's a big warning bell for me. If you need to strip anything inside such a specific function, you're in trouble. It means that the passed-in value has already been encoded (escaped) for a specific format and you need to undo that to carry on. It's best to pass around values in their rawest format and encode them for specific output formats at the last moment. If $myusername has escaped characters because of magic_quotes or some other reason, it's best to make the calling code undo that. Personally, i would simply do this:
[pre]$sql = "SELECT picName FROM userpictures "
. " WHERE username='".mysql_real_escape_string($myusername)."'";[/pre]Hope that wasn't too harsh. :)
Criticism for me = learning opportunity, so thanks for all the feedback. I ll try and implement these suggestions you gave me in my code from now on. Thanks again!
