0
\$\begingroup\$

I have this code working for submitting a form using PHP and HTML (Bootstrap). I did not post the CSS, so I hope this is not an issue.

I am interested if this code is clean, following good practices, secure etc.

  1. Are the headers needed?
  2. Do I need to declare and initialise all those variables?
  3. Do I need <?php if (!empty ($_POST['lastname'])) echo $_POST['lastname']; ?> or is <?php echo $_POST['lastname']; ?> enough?
  4. Is it properly indented ?
  5. Could I separate PHP and HTML even more?
  6. Would you put the PHP part below the HTML? etc etc

    <?php

header('Cache-Control: no-store, no-cache, must-revalidate');
header('Cache-Control: post-check=0, pre-check=0', false);
header('Pragma: no-cache');
header('Expires: ' . gmdate('D, d M Y H:i:s', time() - 8400) . ' GMT');

error_reporting(0);    

// declare variables and initialise
$error              = "";
$form_errors_array  = array();  // array to hold errors to be displayed to the user
$account            = "";
$error_css_class    = array();  // array to hold which fields will have an "error" class added in the html
$error_css_class_arraykeys = array('firstname', 'lastname', 'email', 'ordervalue', 'account', 'deposit', 'term');

$_POST = array_map('trim', $_POST);

// test if the form was submitted and perform basic validation on all fields
if ( !empty($_POST['postform']) ) {

    $error_css_class = array_fill_keys($error_css_class_arraykeys, '');

    if ( empty($_POST['firstname']) ) {
        $form_errors_array[] = "Please enter first name.";
        $error_css_class['firstname'] = "error"; // firstname input field will have a red border because of the "error" css class
    }
    if ( empty($_POST['lastname']) ) { 
        $form_errors_array[] = "Please enter last name.";
        $error_css_class['lastname'] = "error";
    }
    if ( !filter_var($_POST['email'], FILTER_VALIDATE_EMAIL) ) {
        $form_errors_array[] = "Invalid email";
        $error_css_class['email'] = "error";
    }
    if ( empty($_POST['ordervalue'] ) || !is_numeric($_POST['ordervalue']) ) {
        $form_errors_array[] = "Order value is not valid";
        $error_css_class['ordervalue'] = "error";
    }
    if ( empty($_POST['deposit'] ) || !is_numeric($_POST['deposit']) ) {
        $form_errors_array[] = "Deposit value is not valid";
        $error_css_class['deposit'] = "error";
    }
    if ( empty($_POST['term'] ) || ($_POST['term']) == 0 ) {
        $form_errors_array[] = "Term value is not valid";
        $error_css_class['term'] = "error";
    }
    if ( empty($_POST['account']) )  {
        $form_errors_array[] = "Account value (Account1 or Account2) not selected";
        $error_css_class['account'] = "error";
    }        

    //rest of the code only happens if there are no errors in the form
    if ( count($form_errors_array) == 0 ) { 
        // do some useful stuff
        ;
    }
}

function trim_value($value)
{
    $value = trim($value);
}    

?>

<!DOCTYPE html>
<html>
<head>
    <title>title</title>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta name="description" content="">
    <meta name="author" content="">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <script src="//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
    <script>window.jQuery || document.write('<script src="common/js/jquery-1.11.1.min.js"><\/script>')</script>

    <link href="common/bootstrap/bootstrap/css/bootstrap.css" rel="stylesheet">
    <link href="common/bootstrap/jumbotron-narrow/jumbotron-narrow.css" rel="stylesheet">
    <link href="common/css/default.css" rel="stylesheet">
    <script src="common/bootstrap/bootstrap/js/bootstrap.js"></script>

    <!--[if lt IE 9]>
        <link href="common/css/ie8.css" rel="stylesheet">
    <![endif]-->

</head>    
<body>
    <div id="wrapper" class="container-narrow">
        <div class="header clearfix">

        </div> <!-- /header -->

        <div id="content">
            <div class="formcontainer noborder">
                <form action="" method="post" >
                    <fieldset>
                        <legend>Customer details (don't use initials - cannot be amended!)</legend>
                        <input type='hidden' name='postform' value='1'>
                        <div class="input-group">
                            <span class="input-group-addon input-small">First name: </span>
                            <input type="text" name="firstname" 
                                   class="form-control input-small <?php echo $error_css_class['firstname']; ?>" value="<?php if (!empty ($_POST['firstname'])) echo $_POST['firstname']; ?>" placeholder="First name" />
                        </div>
                        <div class="input-group">
                            <span class="input-group-addon input-small">Last name: </span>
                            <input type="text" name="lastname" 
                                   class="form-control input-small <?php echo $error_css_class['lastname']; ?>" value="<?php if (!empty ($_POST['lastname'])) echo $_POST['lastname']; ?>" placeholder="Last name" />
                        </div>
                    </fieldset>
                    <fieldset>
                        <legend>Customer email</legend>
                        <div class="input-group">
                            <span class="input-group-addon input-small">Email: </span>
                            <input type="text" name="email" class="form-control input-small <?php echo $error_css_class['email']; ?>" value="<?php if (!empty ($_POST['email'])) echo $_POST['email']; ?>" placeholder="Email" />
                        </div>
                    </fieldset>
                    <fieldset>
                        <legend>Order details (cannot be amended!)</legend>
                        <div class="input-group">
                            <span class="input-group-addon input-small">Order value: </span>
                            <input type="text" name="ordervalue" class="form-control input-small <?php echo $error_css_class['ordervalue']; ?>" value="<?php if (!empty ($_POST['ordervalue'])) echo $_POST['ordervalue']; ?>" placeholder="Order value"/>
                        </div>
                        <div class="input-group">
                            <span class="input-group-addon input-small">Deposit: </span>
                            <input type="text" name="deposit" class="form-control input-small <?php echo $error_css_class['deposit']; ?>" value="<?php if (!empty ($_POST['deposit'])) echo $_POST['deposit']; ?>" placeholder="Deposit"/>
                        </div>                        
                    </fieldset> 

                    <fieldset>
                        <legend class="radio-group-legend">Select account</legend>
                        <div class="radio marginbottom">
                            <label for="account1"><input class="radio-button-selector" type="radio" name="account" id="account1" value="account1">Account1</label>
                        </div>
                        <div class="radio">
                            <label for="account2"><input class="radio-button-selector" type="radio" name="account" id="account2" value="account2">Account2</label>
                        </div>
                    </fieldset>
                    <fieldset>
                        <legend>Select term (cannot be amended!)</legend>
                        <!-- Select Basic -->
                        <div id="scs_select" class="form-group">
                            <div class="col-md-4 no-left-padding">
                                <select id="term" name="term" class="form-control <?php echo $error_css_class['term']; ?>">
                                    <option value="0">Please select</option>
                                    <option value="6">6 months</option>
                                    <option value="9">9 months</option>
                                    <option value="10">10 months</option>
                                    <option value="12">12 months</option>
                                    <option value="18">18 months</option>
                                    <option value="24">24 months</option>
                                    <option value="30">30 months</option>
                                    <option value="36">36 months</option>
                                    <option value="42">42 months</option>
                                    <option value="48">48 months</option>
                                </select>
                            </div>
                        </div>
                    </fieldset>

                    <div id="submiter">
                        <button type="submit" class="btn btn-primary" >Submit</button>
                    </div>
                </form>
            </div>
            <div class="formcontainer">
                <?php 
                    if ( count($form_errors_array) > 0 ) { 
                        echo '<div class="alert alert-danger">';

                        foreach ($form_errors_array as $formerror) {
                            echo '<span class="glyphicon glyphicon-exclamation-sign" aria-hidden="true"></span>', "&nbsp;", "$formerror<br>"; 
                        }
                        echo '</div>';
                    }
                ?>
            </div>
        </div> <!-- /content -->
    </div> <!-- /wrapper -->

</body>

</html>
\$\endgroup\$
1
\$\begingroup\$
  1. Headers - why not? You can fine tune your cache control. They are not necessary but they aren't harming anything either as long as that's how you want your caching set.

  2. No, but you should. It causes no harm and makes your application more organized and easier to understand.

  3. Technically no, because if $_POST['lastname'] is empty nothing will be displayed anyway. But having it explicitly like you do is better, in case you want to append or prepend anything to it in the future. For example if you want to do

    <?php echo "Lastname: <h3>" . $_POST['lastname'] . "</h3>"; ?> 
    

    Then it will print with an empty value even if it's not present. If you want something to only print if it's available, set it with a condition like you have it now.

  4. Indentation is fine.

  5. Yes, you could have your HTML standalone and just send requests to the PHP file. It's just a matter of choice. Personally I make most everything into templates anyway and put them into the PHP with require_once().

  6. That part doesn't matter, if I were you I'd likely separate them out anyway.

\$\endgroup\$
  • \$\begingroup\$ On point 2), I think that creates a good habit for a programmer, as other languages are stricter and would indeed require variables declared. Correct me if I'm wrong. \$\endgroup\$ – bg17aw Nov 26 '14 at 9:19
  • \$\begingroup\$ I think point 5) is very important, templating is extremely useful. Could you detail a bit how would you do that? where would you put the require_once? How much PHP would you keep in the template and in what form? You would probably avoid any logic in the template. I actually used SMARTY a bit and noticed how easy it is to get carried away and put logic in the template. \$\endgroup\$ – bg17aw Nov 26 '14 at 9:22
0
\$\begingroup\$

I don't have enough rep to comment yet, so I'll write this as an answer. Those headers aren't really necessary, but if you intend on keeping them, you should move them into their own file and then require them in, so those settings can be re-used.

Also, be careful when using "empty()", if one of your values is supposed to be a 0 for some reason, it'll evaluate to false. I'd instead use "isset()" and then do the second more specific check afterwards like you are for most cases.

But really, I'd just use Symfony's Validator to do all of that much more cleanly.

Also, that trim_value function isn't useful at all. Unless you are somehow extending the functionality of "trim()", then all you are currently doing is making your code more confusing. Not to mention it doesn't return anything.

\$\endgroup\$
  • \$\begingroup\$ Headers: usually you want them different per page. Can't say I've seen them in separate files before. Symfony: So instead of commenting the code, you recommend using a framework. Why not CodeIgniter, Lavarel, Zend etc? \$\endgroup\$ – bg17aw Dec 18 '14 at 16:42
  • \$\begingroup\$ Good point on the trim function. Is not returning anything. \$\endgroup\$ – bg17aw Dec 18 '14 at 16:47
  • \$\begingroup\$ @bg17aw I'm not against any web framework, Symfony just happens to have easy to use decoupled components that can be used outside of the framework itself. The Validator component I linked to just removes the need for all of those "if param exists and has format" checks littered throughout the code. \$\endgroup\$ – Derek Dowling Dec 18 '14 at 21:27
  • \$\begingroup\$ "littered throughout the code" ? That is your opinion and a very poor choice of words to express it. Please provide some reference as to why that is litter and why this can't be used, and what are the alternatives. Just referring to a framework is not really a code review. A project might not be able to use a framework, and maybe not the one you (again, personally) prefer. \$\endgroup\$ – bg17aw Jan 16 '15 at 10:47
  • \$\begingroup\$ @bg17aw they're more than welcome to use that code if they so choose. However, many newcomers to programming don't realize that there are many libs/tools out there that are built specifically to deal with what they are trying to do, mitigating the need to re-invent the wheel. Symfony's Validator is but one example that I'm familiar enough to vouch for which would handle all of the checking that they are doing manually and have probably already handled all of the edge cases we normally would overlook. They are more than welcome to search for their own library. \$\endgroup\$ – Derek Dowling Jan 16 '15 at 21:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.