Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 12 of 12
  1. #1
    Regular Coder
    Join Date
    Dec 2008
    Posts
    133
    Thanks
    15
    Thanked 0 Times in 0 Posts

    Lightbulb convert to function or leave as it is?

    I have this CMS code:

    PHP 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.

  • #2
    Regular Coder bacterozoid's Avatar
    Join Date
    Jun 2002
    Location
    USA
    Posts
    490
    Thanks
    24
    Thanked 35 Times in 35 Posts
    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.

  • #3
    Regular Coder
    Join Date
    Dec 2008
    Posts
    133
    Thanks
    15
    Thanked 0 Times in 0 Posts
    thanks for the comment.

    Is there a better way of doing:
    PHP Code:
    //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

  • #4
    Senior Coder angst's Avatar
    Join Date
    Apr 2004
    Location
    Toronto, Ontario
    Posts
    2,114
    Thanks
    15
    Thanked 122 Times in 122 Posts
    could do it more like this example;

    PHP Code:
    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.

    PHP Code:
    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 />";


  • #5
    Regular Coder bacterozoid's Avatar
    Join Date
    Jun 2002
    Location
    USA
    Posts
    490
    Thanks
    24
    Thanked 35 Times in 35 Posts
    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.

  • #6
    Senior Coder angst's Avatar
    Join Date
    Apr 2004
    Location
    Toronto, Ontario
    Posts
    2,114
    Thanks
    15
    Thanked 122 Times in 122 Posts
    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.

  • #7
    Regular Coder
    Join Date
    Dec 2008
    Posts
    133
    Thanks
    15
    Thanked 0 Times in 0 Posts
    Quote Originally Posted by angst View Post
    could do it more like this example;

    PHP Code:
    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.

    PHP Code:
    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

  • #8
    Senior Coder angst's Avatar
    Join Date
    Apr 2004
    Location
    Toronto, Ontario
    Posts
    2,114
    Thanks
    15
    Thanked 122 Times in 122 Posts
    that just returns the result of the fuction,

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

    echo HandErrors($error);

  • #9
    Senior Coder
    Join Date
    Jul 2009
    Location
    South Yorkshire, England
    Posts
    2,318
    Thanks
    6
    Thanked 304 Times in 303 Posts
    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:

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

    Code:
    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.
    Last edited by MattF; 02-23-2010 at 08:16 PM.

  • Users who have thanked MattF for this post:

    CallumJohnson (02-24-2010)

  • #10
    Regular Coder
    Join Date
    Dec 2008
    Posts
    133
    Thanks
    15
    Thanked 0 Times in 0 Posts
    MattF, the array looks like the neatest option.

    I had a go but:

    PHP Code:
    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 Code:
    <?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?

  • #11
    Senior Coder
    Join Date
    Jul 2009
    Location
    South Yorkshire, England
    Posts
    2,318
    Thanks
    6
    Thanked 304 Times in 303 Posts
    Change:

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

    Code:
    print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');
    print('Number: '.$error.'<br/>');}
    and see what error number it prints.

  • #12
    Regular Coder
    Join Date
    Dec 2008
    Posts
    133
    Thanks
    15
    Thanked 0 Times in 0 Posts
    oh!

    print('<br /><strong>Error:</strong>&nbsp;'.$error_messages[$error].'<br />');
    works now? It didn't work when i posted that


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •