By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
459,290 Members | 1,439 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 459,290 IT Pros & Developers. It's quick & easy.

Could someone please critque my code?

100+
P: 348
Hi everyone. Let me start by saying I am pretty new to php and I am looking for the optimal way to code without using OOP. I have the following code that I wrote and I would like to know if there is a better way to do this. The logical side of my brain says that this is not really right.

What I am trying to do is error checking. I have 13 fields that need to be validated and as you can see, I have 13 POST vars. The end of the script checks that all the fields are not blank and then will insert into the db.

Can someone please offer a hand? I would be very grateful.

[PHP] if(isset($_POST['inc'])){
if($_POST['inc'] == ""){
$_POST['err1'] = "err1";
}
}
if(isset($_POST['source'])){
if($_POST['source'] == ""){
$_POST['err2'] = "err2";
}
}
if(isset($_POST['occurred_Date'])){
if($_POST['occurred_Date'] == ""){
$_POST['err3'] = "err3";
}
}
if(isset($_POST['occurred_Time'])){
if($_POST['occurred_Time'] == ""){
$_POST['err4'] = "err4";
}
}
if(isset($_POST['ap'])){
if($_POST['ap'] == ""){
$_POST['err5'] = "err5";
}
}
if(isset($_POST['first_Name'])){
if($_POST['first_Name'] == ""){
$_POST['err6'] = "err6";
}
}
if(isset($_POST['last_Name'])){
if($_POST['last_Name'] == ""){
$_POST['err7'] = "err7";
}
}
if(isset($_POST['address1'])){
if($_POST['address1'] == ""){
$_POST['err8'] = "err8";
}
}
if(isset($_POST['address2'])){
if($_POST['address2'] == ""){
$_POST['err9'] = "err9";
}
}
if(isset($_POST['zip_Code'])){
if($_POST['zip_Code'] == ""){
$_POST['err10'] = "err10";
}
}
if(isset($_POST['phone_Type'])){
if($_POST['phone_Type'] == ""){
$_POST['err11'] = "err11";
}
}
if(isset($_POST['area_Code'])){
if($_POST['area_Code'] == ""){
$_POST['err12'] = "err12";
}
}
if(isset($_POST['phone_Number'])){
if($_POST['phone_Number'] == ""){
$_POST['err13'] = "err13";
}
}
if(isset($_POST['inc']) && (isset($_POST['source']) && (isset($_POST['occurred_Date']) && (isset($_POST['occurred_Time']) && (isset($_POST['ap']) && (isset($_POST['first_Name']) && (isset($_POST['last_Name']) && (isset($_POST['address1']) && (isset($_POST['address2']) && (isset($_POST['zip_Code']) && (isset($_POST['phone_Type']) && (isset($_POST['area_Code']) && (isset($_POST['phone_Number'])))))))))))))){
if($_POST['inc'] !==""){
if($_POST['source'] !==""){
if($_POST['occurred_Date'] !==""){
if($_POST['occurred_Time'] !==""){
if($_POST['ap'] !==""){
if($_POST['first_Name'] !==""){
if($_POST['last_Name'] !==""){
if($_POST['address1'] !==""){
if($_POST['address2'] !==""){
if($_POST['zip_Code'] !==""){
if($_POST['phone_Type'] !==""){
if($_POST['area_Code'] !==""){
if($_POST['phone_Number'] !==""){
echo "INSERT";
}
}
}
}
}
}
}
}
}
}
}
}
}
} [/PHP]
Oct 18 '07 #1
Share this Question
Share on Google+
16 Replies


pbmods
Expert 5K+
P: 5,821
Heya, Frank.

You'll like the empty() function better, as it combines isset() and == ''.

Incidentally, if you're looking for a better way to do this, the first thing that comes to mind is to make it OO :)
Oct 18 '07 #2

100+
P: 348
fjm
Heya, Frank.

You'll like the empty() function better, as it combines isset() and == ''.

Incidentally, if you're looking for a better way to do this, the first thing that comes to mind is to make it OO :)
Hey Pbmods.. Thanks for the reply.. Ok, so I am at least not far off base then.

I make you and Motoma a promise... I will learn OO after this project is completed. I want to learn OO anyway cuz it is the correct way to program. In the mean time, I am going to have to complete this project procedurally.

Thanks bro!

Frank
Oct 18 '07 #3

Motoma
Expert 2.5K+
P: 3,237
Take a look at this:

[php]
$emptyFields = 0;
$errorArray = array();
foreach($_POST as $key=>$value)
{
if($value == '')
{
$emptyFields = 0;
array_push($errorArray, $key);
}
}
if($emptyFields == 0)
{
// INSERT
}
else
{
foreach($errorArray as $missingField)
{
echo "Please supply a value for $missingField<br />";
}
}
[/php]

Another, more secure way would be:

[php]
$fieldArray = ['username', 'password', 'location', 'firstname', 'lastname']; // etc
$emptyFields = 0;
$errorArray = array();
foreach($fieldArray as $index)
{
if($_POST[$index] == '')
{
$emptyFields = 0;
array_push($errorArray, $index);
}
}
if($emptyFields == 0)
{
// INSERT
}
else
{
foreach($errorArray as $missingField)
{
echo "Please supply a value for $missingField<br />";
}
}
[/php]

The second way prevents errors that could happen if someone did not submit through your web page (aka XSS, hacker, or standalone app).
Oct 18 '07 #4

100+
P: 348
fjm
Wow Motoma, thanks!

You see? That is the difference between "knowing" php like you do and me "winging it" with my code. It did occur to me to maybe use an array but I had no idea how. Thanks.. I will use it and also study how it works

I may post back with a question or two about it if I can't understand it

Thanks guys!

Frank
Oct 18 '07 #5

100+
P: 348
fjm
Hey guys,

I attempted to use Motoma's code but I am receiving a parse error:

Expand|Select|Wrap|Line Numbers
  1.  PHP Parse error: syntax error, unexpected '['
The offending line would be this:

[php]$fieldArray = ['username', 'password', 'location', 'firstname', 'lastname'];[/php]

If someone has any ideas, I would be grateful.

Thanks,

Frank
Oct 18 '07 #6

Motoma
Expert 2.5K+
P: 3,237
My apologies, that's a C array. What you are looking for is:
array('firstname', 'lastname', 'nameofyourfirstbornson', 'etc');
Oct 18 '07 #7

100+
P: 348
fjm
Motoma, thanks for the help. I changed the brackets and now I have no more parse error although for some reason I cannot seem to get the code to loop any errors. The code is jumping right to the "insert into"..

I have also verified with print_r and can see all of the post vars but the code isn't seeing the $_POST[$index].

Thanks again!

Frank
Oct 18 '07 #8

Motoma
Expert 2.5K+
P: 3,237
Motoma, thanks for the help. I changed the brackets and now I have no more parse error although for some reason I cannot seem to get the code to loop any errors. The code is jumping right to the "insert into"..

I have also verified with print_r and can see all of the post vars but the code isn't seeing the $_POST[$index].

Thanks again!

Frank
Yup, you will need to increment errorCount inside the loop!
Oct 18 '07 #9

100+
P: 348
fjm
Yup, you will need to increment errorCount inside the loop!
Motoma,

Thanks for the code and the help. I got the code to work by incrementing the value as you suggested.

I am however getting a HUGE amount of "undefined index" errors on this line [php]if($_POST[$index] == '')[/php]

I thought I could circumvent the error by using isset but when I use that, the error messages stop working. I am assuming because the loop no longer works? Am I correct?

I would like to find out why because I don't get why the error messages stop. I am talking about the validation error messages for the user.

Could you please help me to understand?

Thanks,

Frank
Oct 19 '07 #10

Motoma
Expert 2.5K+
P: 3,237
Motoma,

Thanks for the code and the help. I got the code to work by incrementing the value as you suggested.

I am however getting a HUGE amount of "undefined index" errors on this line [php]if($_POST[$index] == '')[/php]

I thought I could circumvent the error by using isset but when I use that, the error messages stop working. I am assuming because the loop no longer works? Am I correct?

I would like to find out why because I don't get why the error messages stop. I am talking about the validation error messages for the user.

Could you please help me to understand?

Thanks,

Frank
Please remember that POST variables are case sensitive. You will need your values in your field array to appear exactly as they do in your HTML FORM. Also make sure you are using POST rather than GET!

Another thing you will want to do is change your test to check for existence first:
[php]
if(!isset($_POST[$index]) || $_POST[$index] == '')
[/php]
Oct 19 '07 #11

100+
P: 348
fjm
Please remember that POST variables are case sensitive. You will need your values in your field array to appear exactly as they do in your HTML FORM. Also make sure you are using POST rather than GET!

Another thing you will want to do is change your test to check for existence first:
[php]
if(!isset($_POST[$index]) || $_POST[$index] == '')
[/php]
Motoma,

Thank you for that. Can you please tell me:

Are these two variables equal? Are they the same?

[php]
$_POST[$index]

$_POST['index']
[/php]

No dollar sign and the use of single quotes. Are these the exact same thing?
Oct 19 '07 #12

Motoma
Expert 2.5K+
P: 3,237
Motoma,

Thank you for that. Can you please tell me:

Are these two variables equal? Are they the same?

[php]
$_POST[$index]

$_POST['index']
[/php]

No dollar sign and the use of single quotes. Are these the exact same thing?

No.

Take a look at some sample code, try it out:
[php]

$three = 'two';
$two = 2;

$arr = array( 2 => 'first element in $arr',
'two' => 'second element in $arr',
'three' => 'third element in $arr');

echo '<pre>';
print_r($arr_)
echo '</pre><hr />';

echo $arr['two'].'<br />';
echo $arr[$two].'<br />';
echo $arr[$three].'<br />';
echo $arr['three'].'<br />';
[/php]

Before you test this though, write down what you think each of the echos is going to display. You may be surprised.
Oct 20 '07 #13

100+
P: 348
fjm
I couldn't get your code to run and got an T-ECHO error on line 11. I noticed you left off the semicolon. I don't know if that was on purpose. I also saw that you referred to print_r($arr_) (note the trailing underscore. Here is what I changed it to to get it to run:

[php]
echo '<pre>';
print_r($arr);
echo '</pre><hr />';
[/php]

Here was the output:
[php]Array
(
[2] => first element in $arr
[two] => second element in $arr
[three] => third element in $arr
)

second element in $arr
first element in $arr
second element in $arr
third element in $arr
[/php]

Here is what I guessed:

1. ?
2. 2
3. two
4. ?

Motoma, thanks for helping me understand this!
Oct 20 '07 #14

Motoma
Expert 2.5K+
P: 3,237
I couldn't get your code to run and got an T-ECHO error on line 11. I noticed you left off the semicolon. I don't know if that was on purpose. I also saw that you referred to print_r($arr_) (note the trailing underscore. Here is what I changed it to to get it to run:

[php]
echo '<pre>';
print_r($arr);
echo '</pre><hr />';
[/php]

Here was the output:
[php]Array
(
[2] => first element in $arr
[two] => second element in $arr
[three] => third element in $arr
)

second element in $arr
first element in $arr
second element in $arr
third element in $arr
[/php]

Here is what I guessed:

1. ?
2. 2
3. two
4. ?

Motoma, thanks for helping me understand this!
Yeah, my laptop keyboard has been giving out, so some of my code will be wrong. I write it all on the spot, so please bare with me as it is untested as of the moment I press the Submit button.

I hope the exercise helped you understand a little more about arrays. If you still need help, I would suggest checking out php.net as it is an excellent resource for programmers of all skill levels. Also remember, experimentation yields the greatest understanding. print_r and echos will help you though a lot of the learning process.
Oct 20 '07 #15

100+
P: 348
fjm
Yeah, my laptop keyboard has been giving out, so some of my code will be wrong. I write it all on the spot, so please bare with me as it is untested as of the moment I press the Submit button.
I figured that it was all freshly written. I really appriciate that Motoma!

I hope the exercise helped you understand a little more about arrays. If you still need help, I would suggest checking out php.net as it is an excellent resource for programmers of all skill levels. Also remember, experimentation yields the greatest understanding. print_r and echos will help you though a lot of the learning process.
It has helped me and I will go over to php's site to gather a bit more info as well. One small issue I have had with reading that material is that it assumes an advanced programming skill level which I unfortunately do not currently have. My only prior programming knowledge is writing UNIX shell scripts and debugging small amounts of PERL scripts.

Thanks Motoma!

Frank
Oct 20 '07 #16

P: 6
Ternary operator would reduce a lot of your if statements.

[PHP]
$inc = isset($_POST['inc']) ? $_POST['inc'] : "error1";
[/PHP]
Oct 21 '07 #17

Post your reply

Sign in to post your reply or Sign up for a free account.