Newbie - Someone suggest ways to improve my code

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Newbie - Someone suggest ways to improve my code

Post 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
f1nutter
Forum Contributor
Posts: 125
Joined: Wed Jun 05, 2002 12:08 pm
Location: London

Post 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. :)
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Post 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;  
?>
f1nutter
Forum Contributor
Posts: 125
Joined: Wed Jun 05, 2002 12:08 pm
Location: London

Post 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.
User avatar
twigletmac
Her Royal Site Adminness
Posts: 5371
Joined: Tue Apr 23, 2002 2:21 am
Location: Essex, UK

Post 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
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Post 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
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Post 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
User avatar
twigletmac
Her Royal Site Adminness
Posts: 5371
Joined: Tue Apr 23, 2002 2:21 am
Location: Essex, UK

Post 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
f1nutter
Forum Contributor
Posts: 125
Joined: Wed Jun 05, 2002 12:08 pm
Location: London

Post 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;
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Post 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
User avatar
twigletmac
Her Royal Site Adminness
Posts: 5371
Joined: Tue Apr 23, 2002 2:21 am
Location: Essex, UK

Post 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
hairyjim
Forum Contributor
Posts: 219
Joined: Wed Nov 13, 2002 9:04 am
Location: Warwickshire, UK

Post by hairyjim »

Hey thats great, works an absolute charm :D

Most greatful for the help guys.
User avatar
BDKR
DevNet Resident
Posts: 1207
Joined: Sat Jun 08, 2002 1:24 pm
Location: Florida
Contact:

Post by BDKR »

Yeah, the Twig is awesome :!:
Post Reply