Can You Review My Code Please?

Love2c0de · November 18, 2012

Hello.

I think I have completed the validation of my code and was wondering if anyone can kindly give my any criticism as to anything I have missed or anything I can do better, which I'm sure both will generate some posts.

There are little things I want to tweak but I wanted to know what some of the pros think (please go easy, im rubbish).

Here is my contact.template.htm:

				

<p id="contact_intro">It is a long established fact that a reader will be distracted by the readable content of a page when looking at its 				
                     layout. The point of using Lorem Ipsum is that it has a more-or-less normal distribution of letters. It is a long 				
established fact that a reader will be distracted by the readable content of a page when looking at its layout. 				
The point of using Lorem Ipsum is that it has a more-or-less normal distribution of letters</p>				

<form method="post" action="index.php?page=contact">				
    <fieldset>				
        <legend>Gardenable Contact Form</legend>				

   <p class="form_heading">Your Details</p>				
<p class="form_instructions">Please leave us your details so we can contact you back!</p>				
<hr class="form_hr" />				
            <p><label for="name">Name:</label><input type="text" name="name" id="name" size="36" maxlength="36" /><span class="red">*</span></p> 				
            <p><label for="email">Email:</label><input type="text" name="email" id="email" size="36" maxlength="70" /></p> 				
            <p><label for="phone">Phone:</label><input type="text" name="phone" id="phone" size="36" maxlength="16" /><span class="red">*</span></p>				
<p><label for="user_comments">Additional Comments:</label><textarea name="user_comments" id="user_comments" rows="5" cols="34" maxlength="400"></textarea></p>				

<hr />				

   <p class="form_heading">Product Details</p>				
<p class="form_instructions">If you wish to <span class="italic">order</span> or <span class="italic">query</span> a product, please specify below.</p>				
<hr class="form_hr" />				

   <p><label for="product">Product:</label>				
   <select name="product_options">				
<option value="default">Choose a product...</option>				
       <option value="benches">Benches</option>				
       <option value="bin_stores">Bin Stores</option>				
       <option value="bird_housing">Bird Housing</option>				
       <option value="gates">Gates</option>				
       <option value="pet_housing">Pet Housing</option>				
       <option value="planters">Planters</option>				
       <option value="sheds">Sheds</option>				
       <option value="tables">Tables</option>				
</select>				
</p>				
<p><label for="product_ref">Product ID:</label><input type="text" name="product_ref" id="product_ref" size="20" maxlength="7" />				
<p><label for="product_comments">Product Comments:</label><textarea name="product_comments" id="product_comments" rows="5" cols="34" maxlength="400"></textarea></p>				

<p><input type="submit" name="submit" value="Submit" />				
  <input type="reset" name="reset" value="Reset" />				
   </p>				
<span id="form_required">Fields marked with a red asterix (<span class="red">*</span>) are required.</span>				
</fieldset>				
</form>				

<div id="error_div">				
    <?php if(isset($output)){ print_r($output);} ?>				
</div>

Here is validation relating to it:

								

<?php 								
include("core/init.inc.php");								
$get_values = array("benches","tables","bird_housing","planters","gates","bin_stores","sheds","pet_housing","default");								

if(isset($_POST['name'])){								

    //if script does not die, the user submitted the form. delete last element (submit button) as we do not need it.								
    (isset($_POST['submit'])) ? array_pop($_POST) : "";								

    //create array to hold any errors.								
$errors = array();								

//firstly, check to see if my required fields contain any data. if they dont we add errors to the error array.								
if(empty($_POST['name']) || empty($_POST['phone'])){								
   $errors[] = "You must fill in the required fields marked with a RED asterix(*).";								
}								

//check to see if the errors array contains anything. if it does, we need to send the user back to the form and display the error.								
//do not carry on if the if statement executes because we dont want to process any more as we know we are going to have to send them back anyway.								
    if(!empty($errors)){								
   $output = $errors;								
}								
    else{								
//if the code reaches here, we have data inside the two required fields so carry on processing all of the data now.								
//pass a reference of the value so that if any ARE set to string NULL, it also changes the original $_POST value.								
foreach ($_POST as $post => &$value) {								
if($value == ""){								
$value = "NULL";								
}								
else{								
switch ($post) {								

case "name": 								
if(!ctype_alpha($value)){								
$errors[] = "The name field can only contain alphabetical characters.";//specify just a first name in form								
}								
break;								

case "email": 								
if(!filter_var($value,FILTER_VALIDATE_EMAIL)){								
$errors[] = "You did not enter a valid email address.";//give an example of an email someone@provider.com in form								
} 								
break;								

case "phone":								
//replaces all characters that are NOT digits 0-9.								
$value = preg_replace("/\D/","",$value);								

//we need to check if it is not equal to an empty string again because if they entered all letters, the preg_replace will replace them								
//and my second if statement here will show an undefined index error. if it is an empty string, add to error array and break out of case								
//prematurely.								
if($value == ""){ $errors[] = "You did not enter a phone number."; break;}								

//checks to see if the first character of the string is not equal to a 0 or if the length of the string isn't 11 (which means its not valid).								
if($value[0] != "0" || strlen($value) != 11){								
$errors[] = "You did not enter a valid phone number.";								
}								
break;								

case "user_comments":								
$len = strlen($value);								

if ($len > 400){								
$less = ($len - 400);								
$errors[] = "You must enter {$less} LESS characters in the 'Additional Comments' field.";								
}								
break;								

case "product_options":								
//if value is not found in the array, could be potential hack. Locate them straght away to the contact page again. 								
if(!in_array($value, $get_values)){								
header("Location: index.php?page=contact");								
}								
break;								

case "product_ref":								

//checks to see if the length of the string is not equal to 7								
if(strlen($value) != 7) {								
$errors[] = "The product id you entered was not long enough, must be 7 numbers.";								

}								
//checks to see if any of the characters entered were not digits. if this executes, we know that the user entered something different								
//than 7 digits so there is no need to carry on and check the ref no against the records so we break out of case prematurely.								
if(!ctype_digit($value)){								
$errors[] = "Product id's can only contain numbers.";								
break;								
}								

//prepared statement which checks the product ref no submitted against a product ref in the database. 								
require("core/prepared_select_pref.php");								

if($row != 1){								
$errors[] = "Your Product ID did not match one of our products.";								
}								

break;								

case "product_comments":								
$len = strlen($value);								

if($len > 400){								
$less = ($len - 400);								
$errors[] = "You must enter {$less} LESS characters in the 'Product Comments' field.";								
}								
break;								
}								
}								

}								
}								

//if the error array contains data, we had some errors during validation, so we display all of these error(s) to the user.								
if (!empty($errors)){								

$output = "<ul>";								
      foreach ($errors as $err => $error_value){								
      $output .= "<li>".$error_value."</li>";								
$output .= "<hr>";								
  }								
  $output .= "</ul>";								
    }								
else{//if there were no errors after all the validation, insert data to database.								
  require("core/prepared_insert.php");								
if($row >= 1){								
      $output = "Your information has successfully sent!";								
}								
else{								
  //maybe send their information to my email instead if there is an issue with insert....probably the best idea rather than displaying an error.								
  $output = "There was an error receiving your information.";								
}								
}								

}								

if (isset($_GET['page']) && $_GET['page'] == "products") {								



  if (isset($_GET['order'])){								

if(in_array($_GET['order'],$get_values)){								
   require("core/get_products.php");								
}								
else{								
   header("Location: index.php?page=products");								
}								
  }								
  else{								
     require("core/get_products.php");								
  }								
}								


?>								
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" 								
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">								

<html xmlns="http://www.w3.org/1999/xhtml">								
<head>								
<title><?php echo "Gardenable - ".$title; ?></title>								
<link rel="stylesheet" type="text/css" href="css/style.css" />								
<script type="text/javascript" src="js/clock.js"></script>								
</head>								

<body>								
<div id="container">								

  <div id="header">								
     <img src="images/gardenable1.fw.png" alt="Gardenable Logo" title="Gardenable" id="logo" border="0" />								

<div id="navigation_div">								
   <img src="images/flowerbed.fw.png" alt="Navigation Image" id="flowerbed_img" border="0" />								
<ul>								
   <li><a href="?page=home">Home</a></li>								
   <li><a href="?page=about">About</a></li>								
   <li><a href="?page=products">Products</a></li>								
   <li><a href="?page=contact">Contact</a></li>								
   <li><a href="?page=find">Find Us</a></li>								
</ul>								
</div>								
  </div>								

  <div id="content">								

<?php include($include_page); ?>								

  </div>								



  <div id="footer">								


  </div>								

</div>								
<p id="pageviews"><?php echo "Page Hits: ".$page_views; ?></p>								
</body>								
</html>

Thanks very much for any help/tips you can give me.

Kind regards,

BnGl

Andy123 · November 18, 2012

I am not a big fan of looping over the POST array. I would just check the fields one by one with if sentences, but I guess that's just a matter of taste. If someone decides to modify a POST request, you will be looping for as many values as they send. If your project is supposed to grow in complexity, I would probably write some simple validation helpers to quickly validate each element and to reduce the amount of code, although this is currently not a problem for you.

What I am really not fond of, though, is the way you are using your included files; the code within those files is executed directly, which drastically decreases the readability of your code in my opinion. Others would probably have to open up the files and see what is happening inside - especially if other code depends on it. For instance, if you are using a variable that was set within an included file, this can be hard to read. I would include functions whenever possible and call those functions as needed. Giving the functions meaningful names will further increase the readability of your code. By using functions, you do not rely on variables set within those files; you'll be relying on return values, which is much better in my opinion.

Love2c0de · November 18, 2012

Thank you very much for your reply.

Well, this is a fairly simple site which has 1 form on the contact page. It's nearly completed, my code works great and my errors/success message shows exactly when they are supposed to. I have written the validation in separate if statements before and I was trying to gain a different concept of validation. I wanted to loop through the $_POST array rather than writing individual statements to check the fields as I have done many times previously.

My included files have nothing to do with the validation of the form. All validation is done within my index.php script right at the top. The only included files relating to the validation are my prepared insert and select statements. I was actually using the $row variable (which is set in the prepared statement files) back in my index.php file and using an if statement to see if the query was a success or fail. I wanted to keep all my connection code in a separate file for readability. I'm fairly good with functions. Are you saying that I should, for example, put my connection code inside a function just so that it is in the same file and there will be less chance of variables not being set/defined etc?

Thanks very much once again for the reply. I'd love to hear more thoughts.

Regards,

AoTB

Edited November 18, 2012 by AoTBuNgLe

Andy123 · November 18, 2012

My included files have nothing to do with the validation of the form. All validation is done within my index.php script right at the top. The only included files relating to the validation are my prepared insert and select statements. I was actually using the $row variable (which is set in the prepared statement files) back in my index.php file and using an if statement to see if the query was a success or fail. I wanted to keep all my connection code in a separate file for readability. I'm fairly good with functions. Are you saying that I should, for example, put my connection code inside a function just so that it is in the same file and there will be less chance of variables not being set/defined etc?

You are very welcome.

I did not mean that your included files had anything to do with the form validation. What I meant was that instead of using variables that are defined within included files, I would use functions. For instance, consider the two examples below.



require_once('some_file.php');

// $result is defined in some_file.php
if ($result) {

}

In this case, let's assume that $result is defined within the included file. It could, however, be defined elsewhere. You might know where it comes from, but in two months you may not remember - or someone else who may have to maintain your script will not know. In these cases, people would have to open the included file to determine if the variable that is being used ($result in this example) is defined there or not. This makes the flow of your script hard to understand.

What I would do instead is something like this:


require_once('is_user_logged_in.php');

$result = isUserLoggedIn();

if ($result) {

}

In this case, you are defining (or at least assigning) a value to your variable ($result) in the current script, so when you read it, you can clearly see what $result represents and where it comes from. It is true that you can not easily see where the function is defined, but the file name should help you out here. Also, most of the time you will not be concerned with the inner workings of the function unless you want to change it. The function used above could then be defined like this in the included file:


function isUserLoggedIn() {
return $_SESSION['isLoggedIn'];
}

I hope that helps to clarify what I mean.

Edited November 18, 2012 by Andy123

Love2c0de · November 18, 2012

Thank you very much for taking the time to explain. I was in a rush this morning so couldn't sit down properly and get my head around it. As an example then, could i do something like:

prepared_insert.php

//do db stuffs
$row = $stmt->affected_rows;
return $row;

index.php

$query_result = include("prepared_insert.php");

I know it's not EXACTLY what you were saying, but I believe it is the same concept, making it easier to read as I know for a fact that variable's VALUE has been set in my included file?

Kind regards,

AoTB.

Edited November 18, 2012 by AoTBuNgLe

Andy123 · November 18, 2012

Yes, I believe you will be able to do like that. It's definitely an improvement in my opinion.

Love2c0de · November 19, 2012

Cheers for that. I will keep it in mind in my future projects!

Regards,

AoTB.

Sign In

Can You Review My Code Please?

Recommended Posts

Love2c0de

Link to comment

Share on other sites

Andy123

Link to comment

Share on other sites

Love2c0de

Link to comment

Share on other sites

Andy123

Link to comment

Share on other sites

Love2c0de

Link to comment

Share on other sites

Andy123

Link to comment

Share on other sites

Love2c0de

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information