[D.Mulroy]

I am teaching myself Php & MySQL from Head First Php & MySQL and have are just starting to learn about server side validation. At this point it is simple, just checking to make sure that all the html forms fields are not empty. I am at a conflicting point in the book where they say to code it one way and I am almost positive my way is better. Now I know that this is not a big deal, but I just want to know if I am right.

The goal of the script is to validate an application that sends out emails to everyone listed in a database. The issue was that if you clicked submit with empty fields blank emails were sent. This was fixed by using the

empty()

function and echo'ing back which part of the form was not filled out. In the book they say to use this method:

<?php
	$header = 'From: elmer@makemeelvis.com';
	$subject = $_POST['subject'];
	$text = $_POST['elvismail'];
	$output_form = false;
	if(empty($subject) && empty($text))
	{
		echo 'Please enter a subject and message';
		$output_form = true;	
	}
	if(empty($subject) && (!empty($text))
	{
		echo 'Please enter a subject';
		$output_form = true;	
	}
	if((!empty($subject) && (empty($text))
	{
		echo 'Please enter a message';
		$output_form = true;	
	}
	if((!empty($subject)) && (!empty($text)))
	{
		$dbc = mysqli_connect('localhost', 'xxxxxx', 'xxxxxxx', 'elvis_store') or die('Error connecting to the database.');
		
		$query = "SELECT * FROM email_list";
	
		$result = mysqli_query($dbc, $query) or die('Error querying the database.');
	
		while($row = mysqli_fetch_array($result)) 
		{
			$first_name = $row['first_name'];
			$last_name = $row['last_name'];
		
			$msg = "Dear $first_name $last_name, \n $text";
			$to = $row['email'];
		
			mail($to, $subject, $msg, $header);
			echo 'Email sent to: ' . $to . '</br>';
			
			mysqli_close($dbc);
		}
	}	
	if($output_form)
	{
?>
<form method="post" action="sendemail.php">
	<label for="subject">Subject of email:</label><br />
    <input id="subject" name="subject" type="text" size="30" /><br />
    <label for="elvismail">Body of email:</label><br />
    <textarea id="elvismail" name="elvismail" rows="8" cols="40"></textarea><br />
    <input type="submit" name="Submit" value="Submit" />
</form>
<?php		
	}
?>

Now my method:

<?php
	$header = 'From: elmer@makemeelvis.com';
	$subject = $_POST['subject'];
	$text = $_POST['elvismail'];
	if((!empty($subject)) && (!empty($text)))
	{
		$dbc = mysqli_connect('localhost', 'xxxxxx', 'xxxxxx', 'elvis_store') or die('Error connecting to the database.');
		
		$query = "SELECT * FROM email_list";
	
		$result = mysqli_query($dbc, $query) or die('Error querying the database.');
	
		while($row = mysqli_fetch_array($result)) 
		{
			$first_name = $row['first_name'];
			$last_name = $row['last_name'];
		
			$msg = "Dear $first_name $last_name, \n $text";
			$to = $row['email'];
		
			mail($to, $subject, $msg, $header);
			echo 'Email sent to: ' . $to . '</br>';
			
			mysqli_close($dbc);
		}
	}	
	else
	{
		if(empty($subject))
		{
			echo 'Please enter a subject';
		}
		else if(empty($text))
		{
			echo 'Please enter a message';
		}
		else
		{
			echo 'Please enter a subject and message';
		}
?>
<form method="post" action="sendemail.php">
	<label for="subject">Subject of email:</label><br />
    <input id="subject" name="subject" type="text" size="30" /><br />
    <label for="elvismail">Body of email:</label><br />
    <textarea id="elvismail" name="elvismail" rows="8" cols="40"></textarea><br />
    <input type="submit" name="Submit" value="Submit" />
</form>
<?php		
	}
?>

My way is better because most of the time the form will be filled out and will be the first condition met. Secondly, and on the same note, it only has to check one(if the form is filled out) if statement opposed to five. Lastly, it has an extra unneeded variable and unneeded if statement to display the form.

^Am I right?

I understand that this question is about pointless but I am asking on terms of learning.

This post has been edited by D.Mulroy: 27 April 2011 - 01:34 PM

[sas1ni69]

Hi there Mulroy

First thing I just wanted to say that there's no pointless questions

Programming is a little like maths really, there are quite a few methods of solving one equation. Some will get you the answer faster than the other. The important bit is the answer. You can then work on simplifying the method which is what you've done.

As for the book's method, I believe it's written that way to make it easier for people who are new to programming to read and understand.

Your way looks better definitely with much less code and straight to the point.

Hope this helps. Good luck

[CTphpnwb]

Yes, the second method is more efficient than the first.

I would put the html in a separate html file and include it. Keeping the two languages separated means that editing either is much easier, especially when your project is larger.

[D.Mulroy]

Thanks for your replies guys Could I still conditionally display the form with an include

[sas1ni69]

D.Mulroy, on 27 April 2011 - 11:16 AM, said:

Thanks for your replies guys Could I still conditionally display the form with an include

You're welcome. You can do everything the same. It just separates the code and makes it much easier to read. A lot of errors come from mixing the 2 on the same page.

[Atli]

I would agree as well. The first method is kind of wasteful, really.

There is one thing, though, that I'd point out in your code. This part

if(empty($subject))
{
    echo 'Please enter a subject';
}
else if(empty($text))
{
    echo 'Please enter a message';
}
else
{
    echo 'Please enter a subject and message';
}

You've already verified that one of these variables is empty, so the else clause there can not be reached. If both fields are empty, the first if condition is true, and your code will only warn you that the "subject" is empty.

You'd have to test both first, and then move on to test them individually. More like:

if (empty($subject) && empty($text)) 
{
    echo 'Please enter a subject and message';
}
else if (empty($subject)) 
{
    echo 'Please enter a subject';
}
else {
    echo 'Please enter a message';
}

This post has been edited by Atli: 26 April 2011 - 08:19 PM

[D.Mulroy]

Ahh good find!

[Prototypical]

efficiency vs readability/understandability (is that even a word?!?!?) as sas1in69 noted is pretty much what this is about.

Back in the day, when memory/performance were far larger limitations, it was almost a given you'd be using one letter variables. What was i? what was p ? what was f ?

Now it's much more common to use variable names that make your code easier to read, for yourself and others.

If you are working on code where there is a need to optimize, you often end up with code that is a little harder to follow.

In making games I'm a little more of a stickler for optimization as all the little bits and pieces add up and sometimes it really is just a small optimization that makes all the difference because the code is executing on 50 enemies 30 times a second!

But with a form like this ? The difference in performance is not even significant.The user will not notice at all.

oh.. but i wanted to add a thumbs up for thinking about it. Because in the right situations, that line of thought is very valuable.

This post has been edited by Prototypical: 26 April 2011 - 08:26 PM

[CTphpnwb]

Hmm, Atli's method will fail when neither is empty. I think this should do it:

if(!empty($subject) && !empty($text)) {
	// process form here
} else {
if(empty($subject) && empty($text)) {
  echo 'Please enter a subject and message';
	} elseif(empty($subject)) {
  	echo 'Please enter a subject';
	} else {
  	echo 'Please enter a message';
	}
include("/path/to/form.html");
}

[Atli]

CTphpnwb, on 27 April 2011 - 03:35 AM, said:

Hmm, Atli's method will fail when neither is empty.

He already checks for that earlier in his code

Actually, if he were to use my suggestion in place of what he currently uses, his code would be exactly like what you posted.