Messed up function?

3raser · March 31, 2011

because your displayBody function doesn't echo anything... it only returns.

It doesn't need to echo, it sets $content to the text I want to be displayed here at the bottom of the code:

<html>
<head>
<title><?php $title; ?></title>
<link rel="stylesheet" type="text/css" href="theme/style.css" />
</head>
<body>
<div id="header">
MCSkins
</div>

<?php echo $content; ?>
</center>

</body>
</html>

Which is why I: return $content;

Zane · March 31, 2011

the $content that is inside displayBody() and the $content that you echo at the bottom of the page are two separate variables.

In other words.. they are in different scopes. You could make $content global but I don't recommend it, just for the fact that globals are a no-no.

What you need to do is set $content to equal the return of displayBody();

for example

$content = displayBody($id, $extract);

3raser · March 31, 2011

the $content that is inside displayBody() and the $content that you echo at the bottom of the page are two separate variables.

In other words.. they are in different scopes. You could make $content global but I don't recommend it, just for the fact that globals are a no-no.

What you need to do is set $content to equal the return of displayBody();

for example
$content = displayBody($id, $extract);

But doing that will disrupt the $content variable if the function isn't necessary. At the bottom of the page, I just want $content = the text in the function, if there is no error. When there is an error, another line of code also uses the variable $content - So if there ever were to be an error, they'd just get a blank page...

And why are globals a "no-no?" I mean, this whole thing could be solved just by setting $content as a global. But what makes them so unstable? I know Thorpe told me something, but I never really got why you shouldn't use them, when they serve their purpose.

Zane · March 31, 2011

Have you tried doing it without globals yet?

Even if you used globals it would act the same way if you were to use my suggestion. You could also set $content by reference inside your function.. like

function displayBody($id, $extract, &$content) {
   $content = "The text you want to display";
}

Regardless of what you do, as soon as you assign something new to $content it will be overwritten.

Setting it global isn't just a no-no, it's a waste of memory, redundant, usually not supported on every server.. and to top it off, PHP6 will not even support globals. Most of this probably won't affect your script enough to notice and you probably won't even care about the "bad practice" of it so long as your script works, but that's the only reason I have behind the global no-no theory. I'm sure you can google plenty of better reasons why not to use them.

Nevertheless, your question isn't about globals, it's about setting $content to something dynamic... and echoing it at the bottom.

If you haven't already, try my suggestion... If you have and it doesn't work... post the revised code and we can work from there.

3raser · March 31, 2011

Have you tried doing it without globals yet?

Even if you used globals it would act the same way if you were to use my suggestion. You could also set $content by reference inside your function.. like
function displayBody($id, $extract, &$content) {
   $content = "The text you want to display";
}
Regardless of what you do, as soon as you assign something new to $content it will be overwritten.

Setting it global isn't just a no-no, it's a waste of memory, redundant, usually not supported on every server.. and to top it off, PHP6 will not even support globals. Most of this probably won't affect your script enough to notice and you probably won't even care about the "bad practice" of it so long as your script works, but that's the only reason I have behind the global no-no theory. I'm sure you can google plenty of better reasons why not to use them.

Nevertheless, your question isn't about globals, it's about setting $content to something dynamic... and echoing it at the bottom.

If you haven't already, try my suggestion... If you have and it doesn't work... post the revised code and we can work from there.

Sorry if you think I'm one of those people that just want to get it to work, whether it's a waste of memory or their is an easier way. I'm all about improving my code, and take all suggestions into consideration. I was just asking WHY the global was a bad function to use.

Anyways, heres my code, yet it still echoes nothing on the page. D:

<?php
include_once("includes/config.php");

if(!$_GET['id'] && $_POST['id'])
{
$id = mysql_real_escape_string($_POST['id']);
}
elseif($_GET['id'] && !$_POST['id'])
{
$id = mysql_real_escape_string($_GET['id']);
}
else
{

}

if(!$id)
{
$content = "Sorry, you have not selected a skin to view.";
}
else
{
$extract_information = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1") or die(mysql_error());

function displayBody($id, $extract, $content)
{
	mysql_query("UPDATE skins SET views = views + 1 WHERE id = '$id'");

	$content = $extract['title']. ", by ". $extract['username'] .".<br/><br/>Description: ". $extract['description'] ." -
	<a href='view.php?download=". $extract['id'] ."'>Download</a><br/><br/>
	<img src='skins/". $extract['id'] .".png' width='500' height='300'>";

	return $content;
}

if(mysql_num_rows($extract_information) == 0)
{
	$content = "Sorry, no skin exists with this ID.";
}
else
{
	$extract = mysql_fetch_assoc($extract_information);

	if($_GET['download'])
	{
		$does_exist_download = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1");

		if(mysql_num_rows($does_exist_download) == 0)
		{
			$content = "No skin exists with this ID, so you may not download.";
		}
		else
		{
		mysql_query("UPDATE skins SET downloads = downloads + 1 WHERE id = '". mysql_real_escape_string($_GET['download']) ."'");

			header("Pragma: public");
			header("Expires: 0");
			header("Cache-Control: must-revalidate, post-check=0, pre-check=0"); 

			header("Content-Type: application/force-download");
			header( "Content-Disposition: attachment; filename=skins/".$extract['id'].".png");

			header( "Content-Description: File Transfer");
			@readfile($file);

		}


	}
	elseif(!$extract['password'])
	{
		displayBody($id,$extract,$content);
	}
	elseif(!$_POST['password'])
	{
		$content = "<br/><br/><div id='header'>Password</div> <center><form action='view.php' method='POST'>
		<input type='hidden' name='id' value='". $id ."'>Password: <input type='password' name='password' maxlength='6'>
		<input type='submit' value='View'></form></center>";
	}
	else
	{
		if($_POST['password'] != $extract['password'])
		{
			$content = "You have entered in an incorrect password. <a href='view.php?id=". $id ."'>Try Again</a> or <a href='index.php'>Home</a>.";
		}
		else
		{
			displayBody($id,$extract,$content);
		}
	}
}
}

?>
<html>
<head>
<title><?php $title; ?></title>
<link rel="stylesheet" type="text/css" href="theme/style.css" />
</head>
<body>
<div id="header">
MCSkins
</div>

<?php echo $content; ?>
</center>

</body>
</html>

And in this code below, the reason I don't do displayBody($id, $extract, $content) - Is because if I do that, and there is an error, the function wont run and so no text would be displayed.

<html>
<head>
<title><?php $title; ?></title>
<link rel="stylesheet" type="text/css" href="theme/style.css" />
</head>
<body>
<div id="header">
MCSkins
</div>

<?php echo $content; ?>
</center>

</body>
</html>

My goal is just to have a function to go to, to change $content (as seen in the function), when there are no errors. Because I make a section of code that displays data from the database, I'd have to put the code in two different sections. Thus the reason why I made the function.

So in summary, when there is no errors, I want the function to execute so it changes $content to the specified data within the function.

function displayBody($id, $extract, $content)
{
	mysql_query("UPDATE skins SET views = views + 1 WHERE id = '$id'");

	$content = $extract['title']. ", by ". $extract['username'] .".<br/><br/>Description: ". $extract['description'] ." -
	<a href='view.php?download=". $extract['id'] ."'>Download</a><br/><br/>
	<img src='skins/". $extract['id'] .".png' width='500' height='300'>";

	return $content;
}

Trying my best not to make this confusing. :/

Zane · March 31, 2011

Ok, I'll explain this suggestion and then I'll post my revision.

First off, you are creating the displayBody function in an else statement. While I can understand the reason you did this, it serves no purpose whatsoever.. other than possibly saving some memory. Remember, the function WILL NOT run unless you tell it to, so simply declaring it in the beginning of the script won't hurt.

Secondly, if the text inside the function is the ultimate text that you want, then why are you mingling it into your validations? The purpose of the validation process is to throw errors.. not success messages. Ideally, you would want to have the function call after all the validations have passed.

Lastly, you lack to use a very important function in PHP known as isset(). This function serves as a way to determine if a variable exists or not..

On last note, if you only have one thing for an IF statement to do when it's true, you DON'T NEED curly braces for it.

With all that said, I'll show you some code, but I wouldn't recommend overwriting everything you have just to test it..as I haven't even tested it. Try making a duplicate file and see how that works out... then tell me what's wrong. It is possible that I didn't even follow the procedure you intended.

include_once("includes/config.php");
/// Put your function up here
function displayBody($id, $extract) {
mysql_query("UPDATE skins SET views = views + 1 WHERE id = '$id'");
$c = $extract['title']. ", by ". $extract['username'] .".

Description: ". $extract['description'] ." - Download

";
return $c;
}

if(!isset($_POST['password']) || empty($_POST['password']))	$content = "

Password Password: ";

if(!isset($_GET['id']) && isset($_POST['id'])) $id = mysql_real_escape_string($_POST['id']);
elseif(!isset($_POST['id']) && isset($_GET['id'])) $id = mysql_real_escape_string($_GET['id']);
else $id = null;


if(empty($id)) $content = "Sorry, you have not selected a skin to view.";
else {
$data = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1") or die(mysql_error());

if(mysql_num_rows($data) == 0) {
	$content = "Sorry, no skin exists with this ID.";
	if(isset($_GET['download'])) $content = "No skin exists with this ID, so you may not download.";
} else { 
	$extract = mysql_fetch_assoc($data);
	if($_POST['password'] != $extract['password'])  $content = "You have entered in an incorrect password. Try Again or Home.";
	else {
		if(isset($_GET['download'])) {  
			mysql_query("UPDATE skins SET downloads = downloads + 1 WHERE id = '". $id . "'");
			header("Pragma: public");
			header("Expires: 0");
			header("Cache-Control: must-revalidate, post-check=0, pre-check=0"); 
			header("Content-Type: application/force-download");
			header( "Content-Disposition: attachment; filename=skins/".$extract['id'].".png");
			header( "Content-Description: File Transfer");
			@readfile($file);
			$content = displayBody($id, $extract);
		}
	}	
}
}
?>










MCSkins

Furthermore, I have no idea why I did all this, I must be extremely bored.

Oh yeah, another thing I noticed in your code is that you repetitively make the same MySQL queries over and over, when you could easily do it once and be done with it.

3raser · March 31, 2011

Edit: Do you mind pointing out where I use multiple queries that are useless?

First off, you are creating the displayBody function in an else statement. While I can understand the reason you did this, it serves no purpose whatsoever.. other than possibly saving some memory. Remember, the function WILL NOT run unless you tell it to, so simply declaring it in the beginning of the script won't hurt.

Why would I have two identical codes, when I could make it easier in a function? Also, isn't displayBody($id, $extract, $content); calling the function?

Ideally, you would want to have the function call after all the validations have passed.

This is pretty much what my function does.

Lastly, you lack to use a very important function in PHP known as isset(). This function serves as a way to determine if a variable exists or not..

Is this really necessary? I mean !$ does the job just as good, and I already know if all my variables are set. If they weren't, the code wouldn't function properly.

On last note, if you only have one thing for an IF statement to do when it's true, you DON'T NEED curly braces for it.

Isn't this really based on the users preference? But I get what your saying, and I might do it just for one lined if states. And to make sure, you mean this, correct?

<?php

if(!$post) echo "bad";

?>

With all that said, I'll show you some code, but I wouldn't recommend overwriting everything you have just to test it..as I haven't even tested it. Try making a duplicate file and see how that works out... then tell me what's wrong. It is possible that I didn't even follow the procedure you intended.

<?php
include_once("includes/config.php");
/// Put your function up here
function displayBody($id, $extract) {
mysql_query("UPDATE skins SET views = views + 1 WHERE id = '$id'");
$c = $extract['title']. ", by ". $extract['username'] .".<br/><br/>Description: ". $extract['description'] ." - <a href='view.php?download=". $extract['id'] ."'>Download</a><br/><br/><img src='skins/". $extract['id'] .".png' width='500' height='300'>";
return $c;
}

if(!isset($_POST['password']) || empty($_POST['password']))	$content = "<br/><br/><div id='header'>Password</div> <center><form action='view.php' method='POST'><input type='hidden' name='id' value='". $id ."'>Password: <input type='password' name='password' maxlength='6'><input type='submit' value='View'></form></center>";

if(!isset($_GET['id']) && isset($_POST['id'])) $id = mysql_real_escape_string($_POST['id']);
elseif(!isset($_POST['id']) && isset($_GET['id'])) $id = mysql_real_escape_string($_GET['id']);
else $id = null;


if(empty($id)) $content = "Sorry, you have not selected a skin to view.";
else {
$data = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1") or die(mysql_error());

if(mysql_num_rows($data) == 0) {
	$content = "Sorry, no skin exists with this ID.";
	if(isset($_GET['download'])) $content = "No skin exists with this ID, so you may not download.";
} else { 
	$extract = mysql_fetch_assoc($data);
	if($_POST['password'] != $extract['password'])  $content = "You have entered in an incorrect password. <a href='view.php?id=". $id ."'>Try Again</a> or <a href='index.php'>Home</a>.";
	else {
		if(isset($_GET['download'])) {  
			mysql_query("UPDATE skins SET downloads = downloads + 1 WHERE id = '". $id . "'");
			header("Pragma: public");
			header("Expires: 0");
			header("Cache-Control: must-revalidate, post-check=0, pre-check=0"); 
			header("Content-Type: application/force-download");
			header( "Content-Disposition: attachment; filename=skins/".$extract['id'].".png");
			header( "Content-Description: File Transfer");
			@readfile($file);
			$content = displayBody($id, $extract);
		}
	}	
}
}
?>
<html>
<head>
<title><?php $title; ?></title>
<link rel="stylesheet" type="text/css" href="theme/style.css" />
</head>
<body>
<div id="header">



MCSkins
</div>

<?php echo $content; ?>
</center>

</body>
</html>

I guess I'll just stop the use of functions, but I'll try this code first.

Zane · March 31, 2011

Do you mind pointing out where I use multiple queries that are useless?

Although I said you had multiple queries, I never said they were "useless"... I did mention their redundancy though.

In your initial code, the one I didn't change, $extract_information and $does_exist_download are the same query.

$extract_information = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1");
.
.
.
.
.
.
$does_exist_download = mysql_query("SELECT title,username,id,password,description FROM skins WHERE id = '$id' LIMIT 1");

Is this really necessary? I mean !$ does the job just as good, and I already know if all my variables are set. If they weren't, the code wouldn't function properly.

!$ determines if a variables is false or not, it doesn't tell you whether it actually exists.

Isn't this really based on the users preference?...one lined IF states

I said you didn't need curly braces .. not that you shouldn't use them. If you want curly braces then so be it, I won't lose sleep, but it helps condense your code and make it easier for another coder to read.

I guess I'll just stop the use of functions, but I'll try this code first.

I don't understand, but ok.

Sign In

Messed up function?

Recommended Posts

3raser

Link to comment

Share on other sites

Zane

Link to comment

Share on other sites

3raser

Link to comment

Share on other sites

Zane

Link to comment

Share on other sites

3raser

Link to comment

Share on other sites

Zane

Link to comment

Share on other sites

3raser

Link to comment

Share on other sites

Zane

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information