i have a php class method which determines whether if a class property have got any value. if it holds any value then it will validate and iterate $this->error class property. here is the class method i am using.
public function validate() {
if(!empty($this->name)) {
if(!preg_match('/^[a-zA-z ]{3,50}$/',$this->name)) {
$this->error['name'] = 'Name should be valid letters and should be between 3 and 25 characters';
}
}
if(!empty($this->email)) {
if(!filter_var($this->email,FILTER_VALIDATE_EMAIL)) {
$this->error['invalidEmail'] = 'Invalid email address';
}
if(empty($this->userId) && $this->emailCount($this->email)) {
$this->error['emailExist'] = 'Email already exist';
}
}
if(empty($this->userId) && !empty($this->password)) {
if(strlen($this->password) < 5 || strlen($this->password > 40)) {
$this->error['password'] = 'Password length should be between 5 and 40 characters';
}
}
if(!empty($this->userId) && !empty($this->newPassword)) {
if(strlen($this->newPassword) < 5 || strlen($this->newPassword > 40)) {
$this->error['password'] = 'Password length should be between 5 and 40 characters';
}
}
if(!empty($this->pPhone)) {
if(!preg_match('/^[0-9]{5,10}$/',$this->pPhone)) {
$this->error['invalidpPhone'] = 'Invalid primary phone number';
}
}
if(!empty($this->sPhone)) {
if(!preg_match('/^[0-9]{5,10}$/',$this->sPhone)) {
$this->error['invalidsPhone'] = 'Invalid secondary phone number';
}
}
return (empty($this->error)) ? true : false;
}
i have used lots of if conditions here which i think is not very good, is there any other way i could determine the above condition and rewrite the code in a more better way?
You could use variable variables and loop through your code. But this means you'll have to come up with some sort of standardized validation scheme for your code
$validateFields = array('email', 'username', 'userId'); //list of fields to validate
$rules = array(
'username' => array('type'=> 'regex', 'rule' => '/^[a-zA-z ]{3,50}$/'),
'email' => array('type' => 'filter', 'rule' => 'FILTER_VALIDATE_EMAIL')
);
foreach ($validateFields as $field) {
if (isset($rules[$field])) {
switch ($rules[$field]['type']) {
case 'regex' :
if(!preg_match($rules[$field]['type']['rule'],$this->$field)) {
$this->error[$field] = ucfirst($field) . ' should be valid letters and should be between 3 and 25 characters';
}
break;
case 'filter' :
if(!filter_var($this->$field, $rules[$field]['type']['rule'])) {
$this->error[$field] = 'Invalid email address';
}
break
//more cases
}
}
}
return (empty($this->error)) ? true : false;
This example only uses one rule per field, but you should be able to easily extend it to use multiple rules per field.
You'll initially have to setup all the rules, but the validation won't grow just because you add another property to your class.
if you want to les use of if condition you can use select case with if block
line case 1 if block
case 2 if block
if will help you in better way
I don't think you can rewrite this specific piece code in a substantially better way.
But, looking at the larger picture there are possible improvements. Looking at the class variables this is probably either a form class or a model instance of some sorts, and you're trying to validate the data before saving it. You could generalize and abstract the validation logic in a separate class. For inspiration, take a look at how various PHP frameworks (e.g. Symphony, CakePHP) handle form and model validation.
I dont think you can avoid this multiple If Statements as I see this is where you validating the fields.
Other Alternative for multimple if statements was Swtich statement,
I am not sure that is feasible for this situation as you are passing String as an Arguments, where as Switch wont take String as an Arguments, it takes only integer as an input
Here's a mockup of a much much neater code solution, I'm not compiling it since.. it's not my life's work, but it's definitely the sort of path you want to be taking, since you have soooo much repeating yourself in your code.
But yeah, this is very close to what yours is doing now, I just lost some ID checks and other various pieces of code that you'll have to re-implement.
Also, I'm doing this with the assumption that this is within a class already, and you're accessing class variables $this->name
<?php
public function validate() {
//You can place your simple ID checks around these wherever they're supposed to go, I lost them ;P.
//This code probably won't compile without errors, but it's to give you a much neater idea.
//Never ever repeat code !!! Your life becomes so much better :]
$this->validate_name($this->name);
$this->validate_email($this->email); // I didn't complete this one.. but this is just all for an idea
$this->validate_phone_number($this->pPhone,'Primary');
$this->validate_phone_number($this->sPhone,'Secondary');
return (empty($this->error)) ? true : false;
}
function validate_password($password){
!empty($password)) {
if(strlen($password) < 5 || strlen($password > 40)) {
$this->error['password'] = 'Password length should be between 5 and 40 characters';
}
}
function validate_name($name){
if(!preg_match('/^[a-zA-z ]{3,50}$/',$name)) {
$this->error['name'] = 'Name should be valid letters and should be between 3 and 25 characters';
}
}
function validate_phone_number($number,$type){
if(!preg_match('/^[0-9]{5,10}$/',number)) {
$this->error['invalidsPhone'] = "Invalid $type phone number";
}
}
At first you should extract your error strings into defines or, better, class constants.
define('ERR_BAD_NAME','Name should be valid letters and should be between 3 and 25 characters');
define('ERR_BAD_EMAIL','Invalid email address');
define('ERR_EMAIL_IN_USE','Email already exist');
define('ERR_BAD_PASSWD','Password length should be between 5 and 40 characters');
define('ERR_BAD_PRIMARY_PHONE','Invalid primary phone number');
define('ERR_BAD_SECONDARY_PHONE','Invalid primary phone number');
public function validate() {
if(!empty($this->name)) {
if(!preg_match('/^[a-zA-z ]{3,50}$/',$this->name)) {
$this->error['name'] = ERR_BAD_NAME;
}
}
if(!empty($this->email)) {
if(!filter_var($this->email,FILTER_VALIDATE_EMAIL)) {
$this->error['invalidEmail'] = ERR_BAD_EMAIL;
}
if(empty($this->userId) && $this->emailCount($this->email)) {
$this->error['emailExist'] = ERR_EMAIL_IN_USE;
}
}
if(empty($this->userId) && !empty($this->password)) {
if(strlen($this->password) < 5 || strlen($this->password > 40)) {
$this->error['password'] = ERR_BAD_PASSWD;
}
}
if(!empty($this->pPhone)) {
if(!preg_match('/^[0-9]{5,10}$/',$this->pPhone)) {
$this->error['invalidpPhone'] = ERR_BAD_PRIMARY_PHONE;
}
}
if(!empty($this->sPhone)) {
if(!preg_match('/^[0-9]{5,10}$/',$this->sPhone)) {
$this->error['invalidsPhone'] = ERR_BAD_PRIMARY_PHONE;
}
}
return (empty($this->error)) ? true : false;
}
the second step is to refactor the tests into separate private methods and get rid of the if blocks:
private function validate_name($name) {
return = empty($name) || preg_match('/^[a-zA-z ]{3,50}$/',$name);
}
private function validate_phone($phone) {
return empty($phone) || preg_match('/^[0-9]{5,10}$/',$phone);
}
private function validate_email($email) {
return empty($email)) || filter_var($email,FILTER_VALIDATE_EMAIL);
}
private function emailInUse($email, $userId) {
if(!empty($email)) {
if(empty($this->userId) && $this->emailCount($this->email)) {
$this->error['emailExist'] = ERR_EMAIL_IN_USE;
}
}
}
private function validate_userPassword($userId, $password) {
$passwordLen=strlen($this->password);
return (empty($this->userId) && !empty($this->password)) ||
(5 <= $passwordLen) && (40 >= $passwordLen);
}
private function lor_error($type, $message) {
$this->error[$type] = $message;
return false;
}
public function validate() {
$isValid = true;
$isValid &= $this->validate_name($this->name) ||
$this->logError('name',ERR_BAD_NAME);
$isValid &= $this->validate_email($this->email) ||
$this->logError('invalidEmail',ERR_BAD_EMAIL);
$isValid &= $this->emailInUse($this->userId, $this->email) ||
$this->logError('emailExist',ERR_EMAIL_IN_USE);
$isValid &= $this->validateUserPassword($this->userId, $this->password) ||
$this->log('password', ERR_BAD_PASSWD);
$isValid &= $this->validate_phone($this->pPhone) ||
$this->log('invalidpPhone',ERR_BAD_PRIMARY_PHONE);
$isValid &= $this->validate_phone($this->sPhone) ||
$this->log('invalidsPhone',ERR_BAD_SECONDARY_PHONE);
return $isValid;
}
In the third refactoring phase you should extract the validate_* method into a validator class to detach validation tasks from your main class. This latter phase is, obviously up to you (as the burden to define a proper log class).