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>
2 Answers 2
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
-
\$\begingroup\$ Thanks for your suggestion. The nameField() I've to use more times and other pages also. \$\endgroup\$PNG– PNG2014年08月13日 13:17:29 +00:00Commented 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: writefunction nameField($n) { return preg_match('/<your regex>/', $n); }
instead of theif-else
business. It'll return what illegal char was found, which could be useful \$\endgroup\$Elias Van Ootegem– Elias Van Ootegem2014年08月13日 13:47:14 +00:00Commented Aug 13, 2014 at 13:47
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 atext
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";