...

View Full Version : convert to function or leave as it is?



CallumJohnson
02-23-2010, 05:10 PM
I have this CMS code:


//instigate code upon user submit
if($action == 'Submit') {
//Get details in order to parse info into database
//Errorchecking (blank)
if(empty($_POST['queryEmail']) || !isset($_POST['queryEmail'])){ $error_blankemail = 1;}
else {
$user_email = $_POST['queryEmail'];
$error_blankemail = 0;

//Errorchecking (Valid Email)
include_once('include/email_validator.php');
$validator = new EmailAddressValidator;
if ($validator->check_email_address(''.$user_email.'')) {
// Email address is technically valid
$error_validemail = 0;
} else {
// Email not valid
$error_validemail = 20;
} //Email Validator

$query_emailtaken = "SELECT wmqueryEmail FROM ".TBL_WMQUERIES." WHERE wmqueryEmail = '$user_email'";
$result = mysql_query($query_emailtaken);
if(mysql_numrows($result) > 0){$error_emailtaken = 40;}
if(mysql_numrows($result) == 0){$error_emailtaken = 0;}

}//Email error checking


if(empty($_POST['querySubject']) || !isset($_POST['querySubject'])){ $error_blanksubject = 3;
} else {
$unsani_subject = $_POST['querySubject'];
$q_subject = mysql_real_escape_string($unsani_subject);
$error_blanksubject = 0;}

if(empty($_POST['queryMessage']) || !isset($_POST['queryMessage'])){ $error_blankmessage = 5;
} else {
$unsani_message = $_POST['queryMessage'];
$q_message = mysql_real_escape_string($unsani_message);
$error_blankmessage = 0;}


//Check if form has empty or incorrect areas
$error = $error_blankemail + $error_blanksubject + $error_blankmessage + $error_validemail + $error_emailtaken;

//Continue code if $error == 0; or
//display correct error message(s)
if ($error == 1){echo '<br /><strong>Error:</strong> Valid Email cannot be blank.<br />';}
if ($error == 3){echo '<br /><strong>Error:</strong> Query Subject cannot be blank.<br />';}
if ($error == 5){echo '<br /><strong>Error:</strong> Query Message cannot be blank.<br />';}
//Multiple errors with form
if ($error == 4){echo '<br /><strong>Error:</strong> Valid Email &amp; Query Subject cannot be blank.<br />';}
if ($error == 8){echo '<br /><strong>Error:</strong> Query Subject &amp; Message cannot be blank.<br />';}
if ($error == 9){echo '<br /><strong>Error:</strong> Valid Email, Query Subject &amp; Message cannot be blank.<br />';}
//Not a valid Email Submitted
if ($error == 20){echo '<br /><strong>Error:</strong> A Valid Email must be provided.<br />';}
if ($error == 23){echo '<br /><strong>Error:</strong> A Valid Email &amp; Subject must be provided.<br />';}
if ($error == 25){echo '<br /><strong>Error:</strong> A Valid Email &amp; Message must be provided.<br />';}
if ($error == 28){echo '<br /><strong>Error:</strong> A Valid Email, Subject &amp; Message must be provided.<br />';}
if ($error > 39){echo '<br /><strong>Error:</strong> You can only submit one query to the webmaster.<br />';}

//Continued code -> [Add query in MySQL]
if ($error == 0) {
$user_ip = $_SERVER['REMOTE_ADDR'];
$date = date('m/d/Y')." at ".date('g:i.s')." ".date('a');
$q = "INSERT INTO ".TBL_WMQUERIES." (wmqueryID, wmqueryTitle, wmqueryText, wmqueryIP, wmqueryEmail,
wmqueryStatus, Timestamp) VALUES ('','$q_subject','$q_message','$user_ip','$user_email','unread','$date')";
if(!($send = $database->query($q))){
echo "<strong>A system error has prevented your query being sent to the system.</strong>
<i>Please try again in a few minutes, this may be a server ever.</i>";
} else {
echo '<br /><strong>Your submission has been received by the system.<br /></strong>
<i>Please allow a few days for the webmaster to read your query.</i><br />';}

}//if ($error > 0)
}//if($action == 'Send')

The question i was asking was that is it better (any benefits involved) by putting this .php snippet into a function in an another file?
Just wondered if it was 'better' practice or there are any benefits involved.

bacterozoid
02-23-2010, 05:21 PM
You may benefit from making it a function if:

a) You re-use the code. Then you prevent duplication
b) Your code is messy and you need a little bit of organization.

Ultimately it's up to you. Although functions are supposed to be short and to the point, in general. The snippet you posted looks like it may best be helped by splitting some of it out into multiple functions.

CallumJohnson
02-23-2010, 05:31 PM
thanks for the comment.

Is there a better way of doing:

//Check if form has empty or incorrect areas
$error = $error_blankemail + $error_blanksubject + $error_blankmessage + $error_validemail + $error_emailtaken;

//Continue code if $error == 0; or
//display correct error message(s)
if ($error == 1){echo '<br /><strong>Error:</strong> Valid Email cannot be blank.<br />';}
if ($error == 3){echo '<br /><strong>Error:</strong> Query Subject cannot be blank.<br />';}
if ($error == 5){echo '<br /><strong>Error:</strong> Query Message cannot be blank.<br />';}
//Multiple errors with form
if ($error == 4){echo '<br /><strong>Error:</strong> Valid Email &amp; Query Subject cannot be blank.<br />';}
if ($error == 8){echo '<br /><strong>Error:</strong> Query Subject &amp; Message cannot be blank.<br />';}
if ($error == 9){echo '<br /><strong>Error:</strong> Valid Email, Query Subject &amp; Message cannot be blank.<br />';}
//Not a valid Email Submitted
if ($error == 20){echo '<br /><strong>Error:</strong> A Valid Email must be provided.<br />';}
if ($error == 23){echo '<br /><strong>Error:</strong> A Valid Email &amp; Subject must be provided.<br />';}
if ($error == 25){echo '<br /><strong>Error:</strong> A Valid Email &amp; Message must be provided.<br />';}
if ($error == 28){echo '<br /><strong>Error:</strong> A Valid Email, Subject &amp; Message must be provided.<br />';}
if ($error > 39){echo '<br /><strong>Error:</strong> You can only submit one query to the webmaster.<br />';}

as it seems to be a little long-winded

angst
02-23-2010, 05:36 PM
could do it more like this example;



if ($error > 39) $ErrMsg = "You can only submit one query to the webmaster.";
if($error > 0) echo "<br /><strong>Error:</strong>" . $ErrMsg . "<br />";


also, you could move that into a function.



function HandErrors($error){
if ($error > 39) $ErrMsg = "You can only submit one query to the webmaster.";
if($error > 0) return "<br /><strong>Error:</strong>" . $ErrMsg . "<br />";
}

bacterozoid
02-23-2010, 05:40 PM
But you would still have to generate $errMsg, so that's really not a shorter way of doing things.

Callum, If you want to have specific errors like that (which is a good thing), you will indeed need to have some long switch like you have there. I would move the whole thing into a function and have it return the error string. It's not making any less code, but it simplifies that body of code somewhat.

angst
02-23-2010, 05:44 PM
yes, but it simplifies the function a little more making it easier to ready, also he doesn't need to have that markup ( <br /><strong>Error:</strong> You can only submit one query to the webmaster.<br /> ) over and over again, so that could make the code a little more neat and tidy.

CallumJohnson
02-23-2010, 08:07 PM
could do it more like this example;



if ($error > 39) $ErrMsg = "You can only submit one query to the webmaster.";
if($error > 0) echo "<br /><strong>Error:</strong>" . $ErrMsg . "<br />";


also, you could move that into a function.



function HandErrors($error){
if ($error > 39) $ErrMsg = "You can only submit one query to the webmaster.";
if($error > 0) return "<br /><strong>Error:</strong>" . $ErrMsg . "<br />";
}


angst, what is the purpose of the 'return' in the second to last line of the code? Haven't seen that before.

bacterozoid thanks for the heads up; i will try and move this error generator to a function to simplify the main body. Although i'm a bit weak on .php functions :P

angst
02-23-2010, 08:17 PM
that just returns the result of the fuction,

so, if an error exists, it would be returned,

echo HandErrors($error);

MattF
02-23-2010, 09:12 PM
Personally, I'd put all of those error messages in their own file and just include that file within the function if there's an error. If you format them as an array, with the key being the error code, i.e:



$error_messages = array(
1 => 'Error message one.',
2 => 'Error message two.',
);


then just:



if ($error > 0)
{
include 'path/to/errormessages/file';

print('<br /><strong>Error:</strong>'.$error_messages[$error].'<br />');
}


Obviously, echo, print or return from the function. Whichever is your preference.

CallumJohnson
02-24-2010, 01:10 PM
MattF, the array looks like the neatest option.

I had a go but:


if ($error > 0){
include ("include/arrays/contactwm_errors.php");
print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');}

//Continued code -> [Add query in MySQL]
else {
$user_ip = $_SERVER['REMOTE_ADDR'];
$date = date('m/d/Y')." at ".date('g:i.s')." ".date('a');

$q = $general->inputsanitization
("INSERT INTO %s (wmqueryID, wmqueryTitle, wmqueryText, wmqueryIP, wmqueryEmail, wmqueryStatus, Timestamp)
VALUES (NULL, '%s', '%s', '%s', '".$user_email."', 'unread', '%s')", TBL_WMQUERIES, $q_subject, $q_message, $user_ip, $date);

if(!($send = $database->query($q))){
echo "<strong>A system error has prevented your query being sent to the system.</strong>
<i>Please try again in a few minutes, this may be a server ever.</i>";
} else {
echo '<br /><strong>Your submission has been received by the system.<br /></strong>
<i>Please allow a few days for the webmaster to read your query.</i><br />';}

}//if ($error > 0)


<?php
//Array for contact webmaster errors

$error_messages = array(
1 => 'Valid Email cannot be blank.',
3 => 'Query Subject cannot be blank.',
5 => 'Query Message cannot be blank.',
4 => 'Valid Email &amp; Query Subject cannot be blank.',
8 => 'Query Subject &amp; Message cannot be blank.',
9 => 'Valid Email, Query Subject &amp; Message cannot be blank.',
20 => 'A Valid Email must be provided.',
23 => 'A Valid Email &amp; Subject must be provided.',
25 => 'A Valid Email &amp; Message must be provided.',
28 => 'A Valid Email, Subject &amp; Message must be provided.',
39 => 'You can only submit one query to the webmaster.',
);

?>

the script knows there are errors but i only get:
Error:

and not the correct error message which means that i think the problem is to do with the array part? Just can't figure out for the life of me whats going wrong?

MattF
02-24-2010, 01:20 PM
Change:



print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');}


to:



print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');
print('Number: '.$error.'<br/>');}


and see what error number it prints.

CallumJohnson
02-24-2010, 01:26 PM
oh!


print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');


works now? It didn't work when i posted that :o



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum