Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some experience with PHP but I have never even try to do this wit pure PHP, but now a friend of mine asked me to help him with this task so I sat down and write some code. What I'm asking is for opinion if this is the right way to do this when you want to use only PHP and is there anything I can change to make the code better. Besides that I think the code is working at least with the few test I made with it.

Here it is:

<?php
session_start();

// define variables and initialize with empty values
    $name = $address = $email = "";
    $nameErr = $addrErr = $emailErr = "";
    $_SESSION["name"] = $_SESSION["address"] = $_SESSION["email"] = "";
    $_SESSION["first_page"] = false;


    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        if (empty($_POST["name"])) {
            $nameErr = "Missing";
        }
        else {
            $_SESSION["name"] = $_POST["name"];
            $name = $_POST["name"];
        }

        if (empty($_POST["address"])) {
            $addrErr = "Missing";
        }
        else {
            $_SESSION["address"] = $_POST["address"];
            $address = $_POST["address"];
        }

        if (empty($_POST["email"]))  {
            $emailErr = "Missing";
        }
        else {
            $_SESSION["email"] = $_POST["email"];
            $email = $_POST["email"];
        }
    }
    if ($_SESSION["name"] != "" && $_SESSION["address"] != "" && $_SESSION["email"] != "") {
    $_SESSION["first_page"] = true;
    header('Location: http://localhost/formProcessing2.php');
    //echo $_SESSION["name"]. " " .$_SESSION["address"]. " " .$_SESSION["email"];
    }
?>
<DCTYPE! html>
<head>
<style>
.error {
    color: #FF0000;
}
</style>
</head>
<body>
<form method="POST" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">
Name <input type="text" name="name" value="<?php echo htmlspecialchars($name);?>">
<span class="error"><?php echo $nameErr;?></span>
<br />
Address <input type="text" name="address" value="<?php echo htmlspecialchars($address);?>">
<span class="error"><?php echo $addrErr;?></span>
<br />
Email <input type="text" name="email" value="<?php echo htmlspecialchars($email);?>">
<span class="error"><?php echo $emailErr;?></span>
<br />
<input type="submit" name="submit" value="Submit">
</form>
</body>
</html>
share|improve this question

migrated from programmers.stackexchange.com Nov 18 '12 at 23:14

This question came from our site for professional programmers interested in conceptual questions about software development.

3 Answers 3

up vote 1 down vote accepted

Just two small notes:

  1. I think

    <DCTYPE! html>
    

    should be

    <!DOCTYPE html>
    
  2. From Code Complete, 2nd Edition, p761:

    Use only one data declaration per line

    [...] It’s easier to modify declarations because each declaration is self-contained.

    [...]

    It’s easier to find specific variables because you can scan a single column rather than reading each line. It’s easier to find and fix syntax errors because the line number the compiler gives you has only one declaration on it.

share|improve this answer

It's incredibly repetitive. Repetitive code is hard to maintain because you have to apply the same change in many places. It's also hard to see at a glance where the similarities and differences lie.

Here is how it could be rewritten:

<?php
    $params = array('name', 'address', 'email');

    session_start();
    $_SESSION['first_page'] = true;

    foreach ($params as $p) {
        $$p = $_SESSION[$p] = '';
        if ($_SERVER['REQUEST_METHOD'] == 'POST') {
            if (empty($_POST[$p])) {
                $errorVarName = "${p}Error";
                $$errorVarName = 'Missing';
            } else {
                $$p = $_SESSION[$p] = $_POST[$p];
            }
        }
        if ($_SESSION[$p] == '') {
            $_SESSION['first_page'] = false;
        }
    }
    if ($_SESSION['first_page'])) {
        # None of the expected params is an empty string
        header('Location: http://localhost/formProcessing2.php');
    }
?>

To go one step further, I would suggest using $val[$p] and $err[$p] instead of $$p and $$errorVarName, respectively. That's because variably named variable names are icky, and smell vaguely of the stink associated with register_globals. An additional benefit of using an associative array is that you can pass all of the processed parameter values as a single argument to a function, which you can't do if they are individual variables. The template would need to be modified to say

Name <input type="text" name="name" value="<?php echo htmlspecialchars($val['name']);?>">
<span class="error"><?php echo $err['name'];?></span>
share|improve this answer
    
Nice. And just for my information cause I haven't followed PHP for some time, is it possible now to use session_start() anywhere in the code? –  Leron Nov 19 '12 at 8:11
    
php.net/manual/en/intro.session.php seems to say that you can call session_start() anytime before you access $_SESSION, and that the call to session_start() can be omitted if session.auto_start is enabled. –  200_success Nov 19 '12 at 8:30

This code is not tested but you can see how I have minimized the if- else block.

<?php
session_start();

function isEmptyValue($Value) {
  return empty($_POST[$value]); 
} 

function getValue($value) {
  setSessionValue($value);
  return $_POST[$value];  
}

function setSessionValue($value){
  $_SESSION[$value] = isEmptyValue($value) ? "" : $_POST[$value];
}

if ($_SERVER["REQUEST_METHOD"] == "POST") {
  $name = isEmptyValue("name") ? "" : getValue("name");
  $address = isEmptyValue("address") ? "" : getValue("address");
  $email = isEmptyValue("email") ? "" : getValue("email");
}

$nameErr = isEmptyValue("name") ? "Missing" : "";
$addrErr = isEmptyValue("address") ? "Missing" : "";
$emailErr = isEmptyValue("email") ? "Missing" : "";

if ($_SESSION["name"] != "" 
    && $_SESSION["address"] != "" 
    && $_SESSION["email"] != "") {
  $_SESSION["first_page"] = true;
  header('Location: http://localhost/formProcessing2.php');
}
?>
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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