[SOLVED] Am I doing this right?

[bena8705]

First of all hello!  I've just finished university for the summer and thought i'd have a play with some code, I plan on starting my third year project a little early to get a head start on it and so have started writing some code.  I've written a PHP based support system and a e-commerce shopping solution before but i'm not overly confident i'm coding with best practices and such.  Since i'm pretty much a newbie to PHP I figured i'd post some of my code in here and get a little feedback before delving into the more technical parts of the website.

 

Okay so the following is a short verification script which currently just checks for the string "bobbob" (I'm going to be changing this later to check whether or not the name exists in the database) and checks the length of the string passed to it.

 

My main concern is the way in which i'm using echo to display errors in the form labels? Is this the theoreticaly "right" way to do things?

 

<?php
include("functions.inc");

if(!checkLength($_POST['title'], 6, 30) && !checkLength($_POST['category'], 6, 30) && !checkLength($_POST['content'], 6, 30))
{
if(!checkForBob($_POST['title']))
{	
	echo "all is well";
}
}

?>

<form id="postnews" name="postnews" method="post" action="postnews.php?do=post">

  <input type="text" name="title" id="title" value="<?php if($_POST['title'] != NULL) { echo $_POST['title']; } ?>" />
  <label>
  <?php
  if($_GET['do'] == "post")
  {
  	if(checkLength($_POST['title'], 6, 30))
{
	echo "Incorrect length";
}
else if($_POST['title'] == "bobbob")
{
	echo "You can't have such a ludicrous title";
}
else 
{
	echo "Valid";
}
  } 
  else 
  {
  	echo "Title";
  } 
  ?>
  </label>
  <br />
  
  <input type="text" name="category" id="category" value="<?php if($_POST['category'] != NULL) { echo $_POST['category']; } ?>" />
  <label>
  <?php
  if($_GET['do'] == "post")
  {
  	if(checkLength($_POST['category'], 6, 30))
{
	echo "Incorrect length";
}
else 
{
	echo "Valid";
}
  } 
  else 
  {
  	echo "Category";
  } 
  ?>
  </label>
  <br />
  
  <textarea cols="20" rows="5" name="content" id="content"><?php if($_POST['content'] != NULL) { echo $_POST['content']; } ?></textarea>
  <label>
  <?php
  if($_GET['do'] == "post")
  {
  	if(checkLength($_POST['content'], 6, 30))
{
	echo "Incorrect length";
}
else 
{
	echo "Valid";
}
  } 
  else 
  {
  	echo "Content";
  } 
  ?>
  </label>
  <br />
  
  <input type="submit" name="postnewssubmit" value="Post Story" />
</form>

 

functions.inc

<?php	
function checkLength($passedVar,$min, $max)
{
if(strlen($passedVar) < $min) 
{
	$result = TRUE;
	return $result;
}
else if(strlen($passedVar) > $max)
{
	$result = TRUE;
	return $result;
}
else
{
	$result = FALSE;
	return $result;
}
}

function checkForBob($passedVar)
{
if($passedVar == "bobbob")
{
	$result = TRUE;
	return $result;
}
}
?>

 

Any help would be very much appreciated.

[GingerRobot]

Well there's nothing necessarily particularly wrong with it. However, if it were me, i'd move all the processing of the form to the top of the script. It would cut down on repetition and make things cleaner.

 

All the while you are performing the same checks on the fields, you could put them in a loop - again, just makes things tidy.

 

My only other comment would be that instead of doing this kind of thing:

 

if($_GET['do'] == "post")

 

It would be better to use the isset() function. Whilst the above method 'works'; if PHP is setup to include notices in error reporting, you would recieve one, since $_GET['do'] is not always set. You could make your if statement read:

 

if(isset($_GET['do'] && $_GET['do'] == "post")

 

This will first check to see if the variable is set. Since the if statement contains two parts, which both most be true for the statement to be true, if the first fails the second will not be tested - therefore you wouldn't recieve a notice regarding an undefined variable.

 

All that said, if $_GEt['do'] will only ever take the value post, you could just check to see if it is set:

 

if(isset($_GET['do']){
}

[bena8705]

Thank you for the advice on isset();  i'm currently developing on a localhost ran by XAMMP (which seems to be more leniant on PHP errors than my uni servers??) so I should probably keep this in mind for when I make it live.

 

As for doing the processing at the top of the script i'm a little stumped as to how i'd display the error next to the field? Unless you mean placing the code from functions.inc inside the main PHP file instead?

[GingerRobot]

Well, the idea is that you would set a variable at the top of the script with the result of the processing. This would have some default/blank value if no processing is done (i.e. the form has not been submitted) You then echo this value next to your field

 

Oh, and one more thought: for a really good article/tutorial on form submission/handling see this two-part article by one of the moderators(roopurt18) here: http://www.rbredlau.com/drupal/node/10

[haku]

Putting errors in labels will work, but its not semantic markup. Semantic markup is the practice of making sure the content inside of tags matches the tags. So putting an error in a label is not semantic, as an error isn't a label, its an error.

 

"Why does this matter?" you may be asking. Well, at the moment, it matters slightly as far as SEO (search engine optimization) goes, in that having semantic markup seems to improve your search engine rankings. But Yahoo is developing a search engine right now that will work more with semantics, so while semantic markup may only slightly help your site now, in the future it will help it much more.

 

I believe semantic markup also helps people with accessibility issues (seeing problems), as screen readers handle semantic markup better than non-semantic markup.

 

And finally, it will make your code a lot easier to read.

[bena8705]

Thank you all very much for your time and help!