2
\$\begingroup\$

Given the following input values from a form, create a method or methods to validate the input based on their requirements. Throw an exception if any data is invalid.

1.1 name: is required
1.2 must be first name AND last name

2.1 email is required
2.2 is a valid email address

3.1 twitter is optional
3.2 is a Twitter handle

The input data will be in the following format:

$input = [
    'name' => '...',
    'email' => '...',
    'twitter' => '...'
];

This is the code snippet I worked on. How can I improve my code? I am not allowed to include or use any other files.

<!DOCTYPE html>
<html>
<head>
    <title>PHP Form Validation</title>
</head>
<body>
<?php

$name = '';
$email = '';
$twitter = '';
$nameError ="";
$emailError ="";
$twitterError ="";

class dataException extends Exception 
{
      public function errorMessage()
      {
        $message = $this->getMessage();
        return $message;
      }
}

try
{
    if(isset($_POST['submit']))
    {   
        if (empty($_POST["name"]) && empty($_POST["email"]))
        {
            throw new dataException('Name and Email are required');
        }
        if (empty($_POST["name"])) 
        {
            throw new dataException('Name is required');
        }
        else
        {
            $name = test_input($_POST["name"]);
            // check name only contains letters and whitespace
            if (!preg_match("/^[a-zA-Z ]*$/",$name))
            {
                throw new dataException('Only letters and white space allowed');
            }   
            else 
            {
                if(str_word_count($name)!=2)
                {
                     throw new dataException('FirstName and LastName is required');
                }
            }
        }
        if (empty($_POST["email"])) 
        {
            throw new dataException('Email is required');
        }    
        else
        {
            $email = test_input($_POST["email"]);
            // check if e-mail address syntax is valid or not
            if (!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/",$email)) 
            {
                throw new dataException('Email is invalid');
            }
        }
        //checked vs internet.
        $twitter = test_input($_POST['twitter']);
        if (!isset($_POST['twitter']) || $_POST['twitter'] === '') 
        {
            $twitter = '';
        }
        else 
        {
            if (!preg_match('/^(\@)?[A-Za-z0-9_]+$/', $twitter)) 
            {
                throw new dataException('Invalid twitter handle');
            }
        }
    }
}
catch (dataException $e)
{   
    echo 'Caught exception: ',  $e->getMessage(), "\n";
}

function test_input($data) 
{
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data);
    return $data;
}
//php code ends here
?>

<form action="" method="post">
Name (FirstName and LastName):
<input type="text" name="name" value="<?php echo htmlspecialchars($name);?>">
* <?php echo $nameError;?>
<br>
<br>
E-mail:
<input type="text" name="email" value="<?php echo htmlspecialchars($email);?>">
* <?php echo $emailError;?>
<br>
<br>
Twitter:
<input type="text" name="twitter" value="<?php echo htmlspecialchars($twitter);?>">
 <?php echo $twitterError;?>
 <br>
 <br>
<input type="submit" name="submit" type="submit" value="Submit">
</form>
</div>
</body>
</html>
\$\endgroup\$

3 Answers 3

Reset to default
1
\$\begingroup\$

Filtering

I'm not a fan of the test_input function. I don't know why w3schools recommends using it, but it's the wrong approach to XSS (you should encode when echoing, not when retrieving user input), and it messes with your data (\' becomes ' without any need for it; maybe you want to have <, for example because you want to send an email to foo<[email protected], not to foo&lt;[email protected], etc.).

Email Validation

Email validation is difficult.

that being said, yours doesn't work as intended. For example, it sais <>'"[email protected] is valid. This is because it only checks if any part of the email address match the pattern, which [email protected] does.

Structure

Your code is quite nested, which makes it hard to read.

For example, if I want to know what happens if submit is not set, I need to scroll down quite a bit. It turns out that nothing happens in that case. You can rewrite your code like this to avoid this issue:

function proccesForm([optionally some arguments]) {
    if(!isset($_POST['submit'])) { 
        return;
    }
}

You can use the same principle for your other checks as well:

function validateForm([optionally some arguments]) {
    if(!isset($_POST['submit'])) { 
        return;
    }

    if (empty($_POST["name"]) && empty($_POST["email"])) {
        throw new dataException('Name and Email are required');
    }

    if (empty($_POST["name"])) {
        throw new dataException('Name is required');
    }

    validateName($_POST["name"];

    if (empty($_POST["email"])) {
        throw new dataException('Email is required');
    }

    validateEmail($_POST["email"]; 

    validateTwitter($_POST["email"]; 
}

Now your code is a lot more readable. Note that I also extracted sub-tasks to their own function. This increases readability, reusability, and testability.

Misc

  • class names should start with an upper-case letter.
  • you have xError variables, but never use them.
  • For an exercise it might be fine, but considering usability, your validation seems non-optimal. Of course, a first validation should happen via js, but even the secondary server-side validation should print all errors. It is quite annoying for a user to enter all fields, get an error that the name is invalid, fix that error, resubmit the form, only to be told that the email is also invalid. You should report all errors at once.
  • I'm also not so sure if exceptions are really the way to go here. First of all, user input not matching a filter doesn't seem that exceptional to me. Additionally, it seems to make it additionally complex to report more than one error at a time (which is probably why you check for the existence of name and email twice).
\$\endgroup\$
3
\$\begingroup\$

Here are just few things I noticed.

Class names
I always try and use uppercase first letters, so DataException rather than dataException.

Exception
The errorMessage() method is redundant, as it just does the same as getMessage(), and it's also not used anywhere.

Function name
test_input doesn't really describe what the function does, as it doesn't actually test, so I renamed it clean_input, which better describes it.

Possible bug
The line if(str_word_count($name) != 2){ would give the error message FirstName and LastName is required if the user enter Homer J Simpson, so I change to see if the count is lower than 2, anything else is fine.

Unused variables
The $*Error variables are echoed, but never have a value assigned to them other than the empty string, so removed references to those.

Checking for data
This maybe more personal preference than anything, but when checking for POST data, I use if(!empty($_POST)).

<!DOCTYPE html>
<html>
<head>
    <title>PHP Form Validation</title>
</head>
<body>
    <?php
    $name            = '';
    $email           = '';
    $twitter         = '';

    class DataException extends Exception {}

    try {
        if(!empty($_POST)){
            if(empty($_POST["name"]) && empty($_POST["email"])){
                throw new DataException('Name and Email are required');
            }
            if(empty($_POST["name"])){
                throw new DataException('Name is required');
            } else {
                $name = clean_input($_POST["name"]);
                // check name only contains letters and whitespace
                if(!preg_match("/^[a-zA-Z ]*$/", $name)){
                    throw new DataException('Only letters and white space allowed');
                } else {
                    if(str_word_count($name) < 2){
                        throw new DataException('FirstName and LastName is required');
                    }
                }
            }
            if(empty($_POST["email"])){
                throw new DataException('Email is required');
            } else {
                $email = clean_input($_POST["email"]);
                // check if e-mail address syntax is valid or not
                if(!preg_match("/([\w\-]+\@[\w\-]+\.[\w\-]+)/", $email)){
                    throw new DataException('Email is invalid');
                }
            }
            //checked vs internet.
            $twitter = clean_input($_POST['twitter']);
            if(!isset($_POST['twitter']) || $_POST['twitter'] === ''){
                $twitter = '';
            } else {
                if(!preg_match('/^(\@)?[A-Za-z0-9_]+$/', $twitter)){
                    throw new DataException('Invalid twitter handle');
                }
            }
        }
    } catch(DataException $e){
        echo 'Caught exception: ', $e->getMessage(), "\n";
    }

    function clean_input($data){
        $data    = trim($data);
        $data    = stripslashes($data);
        $data    = htmlspecialchars($data);
        return $data;
    }


    //php code ends here
    ?>

    <form action="" method="post">
        Name (FirstName and LastName):
        <input type="text" name="name" value="<?php echo htmlspecialchars($name); ?>">
        *
        <br>
        <br>
        E-mail:
        <input type="text" name="email" value="<?php echo htmlspecialchars($email); ?>">    
        <br>
        <br>
        Twitter:
        <input type="text" name="twitter" value="<?php echo htmlspecialchars($twitter); ?>">
        <br>
        <br>
        <input type="submit" name="submit" type="submit" value="Submit">
    </form>
</div>

\$\endgroup\$
2
  • 1
    \$\begingroup\$ see if the count is lower than 2, anything else is fine: Plato, Voltaire, Sukarno or Cher may disagree. Why allow Homer J Simpson if they aren't allowed? As this is just an exercise, proper name validation is probably not that important, but it's something to keep in mind for real-world cases. \$\endgroup\$
    – tim
    Commented Mar 16, 2016 at 14:29
  • \$\begingroup\$ I agree, this was just working from the restriction of must be first name AND last name from OP. \$\endgroup\$
    – TMH
    Commented Mar 16, 2016 at 14:44
0
\$\begingroup\$

Names are not purely upper and lower case letters. A name may have special characters like ' or -. It may have an accent, tilde, or glottal stop. While your example is sound on a simple level, it would be really nice to see examples that accept diverse names.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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