2
\$\begingroup\$

I need some improvements on my code. I did some validation by preg_match and functions. I need some special characters restrictions every field.

<?php
$error='';
if(isset($_POST['submit']))
{
if(nameField($_POST['name'])=='')
{
    $error="Invalid Name";
}

if(numberField($_POST['phone'])=='')
{
    $error.="<br>Invalid Number";
}
if(nameField($_POST['address'])=='')
{
    $error.="Invalid Address";
}
if($error)
{
    echo $error;
}
else
{
    echo "Form submitted successfully";
}
}

function nameField($val)
{
if(preg_match("/([%\$#\*\'\"\()]+)/", $val)) //Not allowed some of the special characters
{
return false;
}
else
{
return true;
}
}
function numberField($val)
{
if(preg_match("/^[0-9]*$/", $val))
{
return true;
}
else
{
return false;
}
}
?>

<html>
<body>
<form method="post">
<input type="text" name="name"/>
<input type="text" name="phone">
<input type="text" name="address">
<input type="text" name="city">
<input type="submit" name="submit" value="Submit"/>
</form>
</body>
</html>
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

I'll try to keep things brief, for a change:

nameField seems reasonable enough. Although I do have some suggestions (see below).
The numberField function is another matter. If you want to check for a valid phone number, there are a couple of things to consider:

  • User tend to enter a phone number in a human-readable format: 213-1234-4454 or 123/1234.4567
  • International phone numbers can start with Either a double 0, or a plus sign. both of which are valid: (0012) 123/1234-5678 is a possible value you'd have to process, just as +12 123/1234-5678

Bottom line: First trim the non-numeric chars, then check if the input is valid, I'd say:

function sanitizeAndValidate($phoneNr)
{
    //replace all non-numeric chars, after replacing a single + char with 00
    $phone = preg_replace('/[^\d]/', '', str_replace('+', '00', $phone, 1));
    $len = strlen($phone);
    //perhaps changing these constants to argumenst might be preferable
    if (is_numeric($phone) && $len >= MIN_PHONE_LEN && $len <= MAX_PHONE_LEN)
        throw new \InvalidArgumentException(
            sprintf(
                '%s is an invalid phone number (%s after sanitation)',
                $phoneNr,
                $phone
            )
        );
    return $phone;

}

Pas the raw phone number to this function, if the phone number is valid, you'll get a santized version back. If the phone number is invalid, catch the exception and present the user with an error message.

The nameField function is OK, but it's a bit redundant. If you only use it in one place, I'd avoid the overhead of a function-call, and simply inline the check, by replacing if (nameField($_POST['name'])) with this:

if (preg_match('/([%_\$#*\'"\()]+)/', $_POST['name']))

You might have noticed that I don't escape all of the chars, simply because there's no reason to escape them all. I've also added _ to the restricted chars. Simply because everybody knows that % is a MySQL wildcard, but not everybody seems to be aware that _ is, too.

Regular expressions can impact preformance, which is, I suspect, why you're asking for ways to improve your code. An alternative to preg_match here could be to have an array of prohibited chars, and use str_replace:

$notAllowed = array('%', '_', '$', '#', ...);//and so on
if (str_replace($notAllowed, '', $_POST['name']) !== $_POST['name'])
{//values are not the same, a forbidden char was replaced
    //name contains prohibited char
}

I'm not so sure if that would be faster then a preg_match, though, and besides: premature optimization is the root of all evil.

Last comment:
Please, try to adhere to the coding standards as much as possible. You seem to have some formatting issues

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your suggestion. The nameField() I've to use more times and other pages also. \$\endgroup\$
    – PNG
    Commented Aug 13, 2014 at 13:17
  • \$\begingroup\$ @Prakash: In that case, keep the function, but make sure that str_replace isn't the faster option. Also simplify it a bit: write function nameField($n) { return preg_match('/<your regex>/', $n); } instead of the if-else business. It'll return what illegal char was found, which could be useful \$\endgroup\$
    – Elias Van Ootegem
    Commented Aug 13, 2014 at 13:47
2
\$\begingroup\$

  • Please, please, please indent your code.

  • HTML5 includes <input type="tel" /> for phone numbers, though it's currently unsupported by the major browsers. You can choose whether or not you want to use this; it'll just default to a text field.

  • There is no point in using if (condition) return true; else return false;. Ever. Instead, just write:

    return preg_match('/^[0-9]*$/');

  • You could name numberField better; the function name should include a verb. Furthermore, do you really mean to match empty string? If not, use + instead of *.

  • Your use of $error is inconsistent. I propose using an array instead:

    $errors = array();
if (nameField...) {
    $errors[] = "Invalid Name";
}
if (numberField...) {
    $errors[] = "Invalid Number";
}
...
echo $errors ? implode("<br />", errors) : "Form submitted successfully";
\$\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.