Page 1 of 1

Newbie - Someone suggest ways to improve my code

Posted: Thu Nov 14, 2002 4:22 am
by hairyjim
Hi all,

I am very new to PHP, but I am finding allsorts of great info, tutorials and advice from the internet.

Would people be willing to overlook this code I have written and suggest ways in which to improve my code. Or even suggest resources for writing better PHP?

It takes a form input, called suggestion and then e-mails it off. I know it is very basic.

<?php

if ("$_POST[Suggestion]" == "")
{
echo "<center><FONT SIZE=2 Face=verdana>";
echo "Sorry you have not entered a suggestion<br>";
echo "Please use your browsers back button.<br>";
echo "</FONT></center>";
}
else
{
echo "<center><FONT SIZE=2 Face=verdana>";
echo "Thanks you entered the following suggestion<br>";
echo "$_POST[Suggestion]";
echo "</FONT></center>";

/* recipients */
$to = "myaddress@somewhere.com";

/* suggestion */
$subject = "Good ideas submission";

/* message */
$message = "$_POST[Suggestion]";

/* and now mail it */
mail($to, $subject, $message);
}
?>

Cheers
James

Posted: Thu Nov 14, 2002 4:38 am
by f1nutter
Looks good to me. There are always ways to improve code or do it differently. Everyone believes their way is best!

Code: Select all

&lt;?php

if ($_POST&#1111;"Suggestion"] == "") 
{
  $referer = $_SERVER&#1111;"HTTP_REFERER"]; // Could be $_ENV
  header("Location: " .$referer);
} 
else 
{
?&gt;

&lt;center&gt;&lt;FONT SIZE=2 Face=verdana&gt;Thanks you entered the following suggestion&lt;br&gt;

&lt;?php
echo $_POST&#1111;"Suggestion"];
?&gt;

&lt;/FONT&gt;&lt;/center&gt;

&lt;?php
/* recipients */ 
$to = "myaddress@somewhere.com"; 

/* suggestion */ 
$subject = "Good ideas submission"; 

/* message */ 
$message = $_POST&#1111;"Suggestion"]; 

/* and now mail it */ 
mail($to, $subject, $message); 
} 
?&gt;
If the suggestion is empty, the user is returned to the previous page using the referer. However, you may want to do it the way you did, because you at least have an error message.

Remove the quotes from the $_POST variable, otherwise PHP will treat it as a string. The quotes need to go around the array index Suggestion. I have also placed the block of HTML outside PHP because this does not need parsing by the PHP engine, saving a bit of time (Makes a difference on giant pages!)

Otherwise, looks good. :)

Posted: Thu Nov 14, 2002 5:40 am
by hairyjim
Great. Thanks for the quotation usage tip.

I am now hoping to add a name field.

The field is optional, so I want the $message variable to contain the persons name if it is entered, and obiously not if it is not.

I tried the following changes in my code, at /* message */ section. For some reason I get a parse error, any idea why? Plus is there some way of specifying a carriage return in the message field?

Notice: Undefined index: Name in c:\inetpub\wwwroot\php\tellus.php on line 125

Code: Select all

<?php 
if ($_POST&#1111;"Suggestion"] == "")  
&#123; 
?>
    <center><FONT SIZE=2 Face=verdana>
    <p>Sorry you have not entered a suggestion</p>
	<p>Please use your browsers back button.</p>
	</FONT></center>
<?php	
&#125;  
else  
&#123; 
?> 

<center><FONT SIZE=2 Face=verdana>Thanks you entered the following suggestion<br> 

<?php 
echo $_POST&#1111;"Suggestion"]; 
?> 

</FONT></center> 

<?php 
/* recipients */  
$to  = "j.dadd@improvision.com";  

/* suggestion */  
$subject = "Good ideas submission";  

/* message */  
	if ($_POST&#1111;"Name"] == "")  
	&#123; 
	$message = $_POST&#1111;"Suggestion"]; 
	&#125;
	else
	&#123; 
	$message = $_POST&#1111;"name"] submitted the following good idea $_POST&#1111;"Suggestion"]; 
	&#125;
/* and now mail it */  
mail($to, $subject, $message);  
&#125;  
?>

Posted: Thu Nov 14, 2002 6:15 am
by f1nutter
hairyjim wrote:

Code: Select all

/* message */  
if ($_POST&#1111;"Name"] == "")  
&#123; 
 $message = $_POST&#1111;"Suggestion"]; 
&#125;
You are getting an "Undefined index" because you are trying to access index "Name" in the $_POST array, when one hasn't been entered.

I think you need

Code: Select all

&lt;?php
if (!isset($_POST&#1111;"Name"]))
{
 $message = $_POST&#1111;"Suggestion"]; 
}
else
...
?&gt;
means: if NOT isset. The manual will explain it better.

Posted: Thu Nov 14, 2002 6:27 am
by twigletmac
You may want to use the empty() function instead of the isset() function in order to also check that $_POST['Name'] does not contain an empty string.

Code: Select all

if (empty($_POST&#1111;'Name']) {
this is equivalent to doing something like

Code: Select all

if (!isset($_POST&#1111;'Name']) || ($_POST&#1111;'Name'] == '' || $_POST&#1111;'Name'] == 0)) {
Mac

Posted: Thu Nov 14, 2002 6:47 am
by hairyjim
Cheers guys,

i will play around with your suggestions and then post back to you let you know how I got on.

I gotta say this though, I have been using Lasso for 2 years, and it is not nowhere near as easy as PHP to use ;) Plus it seems that I do far fewer key strokes with PHP than Lasso :)

Cheers

James

Posted: Thu Nov 14, 2002 7:23 am
by hairyjim
Hey guys,

When I submit my form and say I submitted the following text:

Wouldn't it be great if we had a coke machine

Why does PHP put a \ before the ' like thus Wouldn''t

I thought it was me typing rubbish at first, but I can repeat this every time. I presume it is some sort of encoding, could you point me in the right directions?

James

Posted: Thu Nov 14, 2002 7:28 am
by twigletmac
It adds slashes in case you want to put the data into a database. You can either turn off magic_quotes_gpc in your php.ini or if you don't have access to that or you want to make sure that your script will work as expected whether magic_quotes are on or off, you can use stripslashes() on each of the variables passed from the form to remove all the ''s.

Mac

Posted: Thu Nov 14, 2002 7:38 am
by f1nutter
PHP has special characters, like double quote to indicate a string, or an exclamation mark means logical NOT.

If you want to use these characters as they are, they need to be "escaped" so that PHP doesn't treat them as a special character, but as they are written.

Look at the colour formatting below to see how this affects a string:

Code: Select all

&lt;?php

print('Wouldn't it be great if we had a coke machine?');


?&gt;
Between the single quotes is a string, and PHP doesn't know that it is part of one long string. Compare with this.

Code: Select all

&lt;?php

print('Wouldn''t it be great if we had a coke machine?');

?&gt;

Posted: Thu Nov 14, 2002 7:53 am
by hairyjim
Ahhh....

I could see that being a bit of a bummer if you have alot of form inputs :?

I have now changed the way in which my form works now, turned all the form inputs into variables at the top and stripped them there instead of every point I would specify the $_post in my code.

Cheers.

Works a treat.

James

Posted: Thu Nov 14, 2002 8:00 am
by twigletmac
A quick way of stripping all your form inputs and to assign them to the same value as the particular form element's name attribute is to do:

Code: Select all

foreach ($_POST as $key =&gt; $value) {
    /* The double dollar sign on $key that follows is intended   */
    $$key = stripslashes($value);
}
If you just want to strip all of the $_POST array elements and store the new stripped value in $_POST you can do:

Code: Select all

foreach ($_POST as $key =&gt; $value) {
    $_POST&#1111;$key] = stripslashes($value);
}
Mac

Posted: Thu Nov 14, 2002 10:18 am
by hairyjim
Hey thats great, works an absolute charm :D

Most greatful for the help guys.

Posted: Thu Nov 14, 2002 10:42 am
by BDKR
Yeah, the Twig is awesome :!: