3
\$\begingroup\$

I have the following code that I need help with refactoring:

  1. Validation class for objects you may create:
    • Email address
    • URL
    • Norwegian Mobile Number (must start with 4, 9 or 59 and be 8 digits long)
<?php
/**
    Validation Class
 * Class  to  to validate any Data send  by  user depend  in Data type  and Rules     
 *  @package Front
 */
//namespace Front;

class ValidationSystem {

    /**
     *  @var Array  for Validation Error  in System 
     *  @access private
     */
    private Static $validation_errors;

    /**
     * 
     *   Class Construct Function 
     *   @return void
     *  @access public
     */
    public function __construct() {
        self::$validation_errors = array();
    }

    /**
     *  function for Validate  input Value by Validation Result  like boolean value 
     * @param type $input_value
     * @return boolean
     * @access public
     */
    public static function do_validation($input_value) {
        $validation_result = TRUE;
        foreach ($input_value as $obj_name => $data_obj) {

            switch ($data_obj['rule']) {
                case 'EMAIL':
                    if (!self::is_valid_email($data_obj['value'], $obj_name)) {
                        $validation_result = FALSE;
                    }
                    break;
                case 'URL':
                    if (!self::is_valid_url($data_obj['value'], $obj_name)) {
                        $validation_result = FALSE;
                    }
                    break;
                case 'NorwayMobile':
                    if (!self::is_norwegian_number($data_obj['value'], $obj_name)) {
                        $validation_result = FALSE;
                    }
                    break;
                default :
                    self::$validation_errors[] = "INVALID_RULE_ERROR_MESSAGE";
                    $validation_result = FALSE;
                    break;
            }
        }
        return $validation_result;
    }

    /**
     *  function  for return  validation Error  
     *  @return array  Error Array
     *  @access public
     */
    public static function get_validation_error_message() {
        return self::$validation_errors;
    }

    /**
     *  function  for Validate Email Value; 
     * @param String  $email_str  Email Text value
     * @return Boolean
     * @access Public 
     */
    public function is_valid_email($email_str, $obj_name) {
        if (!filter_var($email_str, FILTER_VALIDATE_EMAIL)) {
            self::$validation_errors[$obj_name] = "INVALIED_EMAIL_ERROR_MESSAGE";
            return FALSE;
        }
        return TRUE;
    }

    /**
     *   Function  for Validate URL Value
     * @param String $url_str  URL Text Value 
     * @return  boolean
     * @access public 
     */
    public function is_valid_url($url_str, $obj_name) {

        $url_str = trim($url_str);
        //add  Http  to link if  not have any protocal 
        $url_str = parse_url($url_str, PHP_URL_SCHEME) === null ? 'http://' . $url_str : $url_str;
        // check   if  is invalid URL  or  Not
        $is_valid = TRUE;
        //first check with php's FILTER_VALIDATE_URL
        if (!filter_var($url_str, FILTER_VALIDATE_URL, FILTER_FLAG_SCHEME_REQUIRED | FILTER_FLAG_HOST_REQUIRED)) {
            $is_valid = FALSE;
        } else {
            //check host format is valid  ex : www.domainName.com or domain.com
            $host = parse_url($url_str, PHP_URL_HOST);
            $dotcount = substr_count($host, '.');
            if ($dotcount > 0) {
                if ($dotcount == 1) {
                    if (strpos($host, 'www.') === 0) {
                        $is_valid = FALSE;
                    }
                }
            } else {
                $is_valid = FALSE;
            }
        }
        if ($is_valid) {
            return TRUE;
        } else {
            self::$validation_errors[$obj_name] = "INVALIED_URL_ERROR_MESSAGE";
            return FALSE;
        }
    }

    /**
     *  function  for check mobile Number as mobile Number Norway Format
     * @param integer $phone_number
     * @return boolean
     * @access Public 
     */
    public function is_norwegian_number($phone_number, $obj_name) {
        if (preg_match("/^(((4|9){1})\d{7})|(((59){1})\d{6})$/", $phone_number) == 0) {
            self::$validation_errors[$obj_name] = "INVILAD_NORWAY_MOBILE_ERROR_MESSAGE";
            return FALSE;
        }
        return TRUE;
    }

    /**
     *  function  for testing class with some Defult value   for systems 
     *  @return Output Testing Result;
     *  @access public
     */
    public static function validation_test() {
        $email_info = "nobody@exmple.com";
        $url_info = "domain.com";
        $norwegian_mobile = "41111111";

        /*  make  test  with  above  Default Value */
        $vildation_Result = self::do_validation(
                        array(
                            'email_Filed' => array('rule' => 'EMAIL', 'value' => $email_info),
                            "url_field" => array('rule' => "URL", "value" => $url_info),
                            "mobile_feild" => array("rule" => 'NorwayMobile', "value" => $norwegian_mobile)
                        )
        );
        if ($vildation_Result) {
            echo "vildation was corrct  and all Field  input correcttly ";
        } else {
            echo "<pre>";
            print_r(self::get_validation_error_message());
            echo "</pre>";
        }
    }

}

?>

Procedural code for this class:

<?php error_reporting(0);
/**
 *  Validate.php  
 *  Control  for Validation submited Data  and Return valdition Result  as  JOSN object .
 *  @package Front
 *  @example ../validate.phpemail=nobady@dmainname.com&url=domainname.com&mobile=95001122
 */
//add Validation Calss 
require_once('ValidationSystem.php');

/**
 * @var  Array  Array for All Text Message  in System  to make easy for  translate and edit 
 */
$message_txt_EN = array();
$message_txt_EN['INVALID_EMPTY_DATA_MESSAGE'] = "No any object to Valid it";
$message_txt_EN['INVALID_RULE_ERROR_MESSAGE'] = "No any of submit objects is URL ,E-mail or Mobile Number.";
$message_txt_EN['INVALIED_EMAIL_ERROR_MESSAGE'] = " E-mail is invalid format, please check it again. ";
$message_txt_EN['INVALIED_URL_ERROR_MESSAGE'] = "URL is invalid format, please check it again. ";
$message_txt_EN['INVILAD_NORWAY_MOBILE_ERROR_MESSAGE'] = "Mobile number is not Vaild mobile number in Norway";
$message_txt_EN['SUCCESS_VALIDATION_MESSAGE'] = "Congratulation all data you were submitted is valid format.";

// get  sumbmited Data  form Api (get) or  index page "POST"
if (isset($_POST) && !empty($_POST)) {
    $object_data = $_POST;
} else {
    $object_data = $_GET;
}
/* Decodes URL */
foreach ($object_data as $data_key => $data_value) {
    $object_data[$data_key] = urldecode($data_value);
}
/**
 *  @var Array  Validation Result  
 */
$respond_result = array();
/**
 * check is Data not Empty and fileds is correct and make Validation process on it   
 */
if (!empty($object_data)) {
    $vildate_data = array();
    /* create rule name  for every object  to validate depend on it . */
    $vaid_fields = array('email' => 'EMAIL', 'url' => 'URL', 'mobile' => 'NorwayMobile');

    foreach ($object_data as $obj_key => $obj_value) {

        if (!empty($obj_value)) {

            if (array_key_exists($obj_key, $vaid_fields)) {

                $vildate_data [$obj_key] = array('rule' => $vaid_fields[$obj_key], 'value' => $obj_value);
            }
        }
    }

    /* check  if Data is valid or  not  and return Json Result */
    if (!empty($vildate_data)) {

        if (ValidationSystem::do_validation($vildate_data)) {

            $respond_result = array('result' => TRUE, "message" => array($message_txt_EN['SUCCESS_VALIDATION_MESSAGE']));
        } else {
            $error_result = ValidationSystem::get_validation_error_message();

            foreach ($error_result as $er_key => $err_txt) {

                $error_result[$er_key] = $message_txt_EN[$err_txt];
            }
            $respond_result = array('result' => FALSE, 'message' => $error_result);
        }
    } else {

        $respond_result = array('result' => FALSE, "message" => array($message_txt_EN['INVALID_EMPTY_DATA_MESSAGE']));
    }
} else {

    $respond_result = array('result' => FALSE, "message" => array($message_txt_EN['INVALID_EMPTY_DATA_MESSAGE']));
}
echo json_encode($respond_result);
exit();
?>

Please provide any advice for improved scalability, speed, and any recommend tools or design pattern for re-factoring this legacy code.

\$\endgroup\$
2
\$\begingroup\$

There is too many comments. Write comments when code can't explain this part. But instead you commented every line. These comments create a mess, and do not help.

Of course I'm not about phpdoc.


Your phpdoc is invalid for this functions: is_valid_email, is_valid_url, is_norwegian_number. You pass 2 arguments, but you phpdoc contain only 1.

Write primitives, like a string, array, boolean and others in lowercase. Because when I see String I can think about some wrapper for string primitive. But indeed it isn't.

@param type $input_value type? Your code doesn't contain a class with name type. Rename it to array. Same in @return Output Testing Result; This function doesn't return anything, so write @return void.

BUT will be better if this function return true/false depends of passed/failed tests. BUT more better -- to place test correctly in separate file, and run via phpunit.


    foreach ($input_value as $obj_name => $data_obj) {
        switch ($data_obj['rule']) {

You named your variable $data_obj, but indeed it's array. You should rename it. When I see $obj, I'm thinking about stdClass, but not about an array.


You should use non-static methods there. Really I don't see a reason why you've used static methods. I think better way will be -- set all is_... methods private, pass your rules/data in constructor and then use do_validation method.

You code will look like this:

$validator = new ValidationSystem($data);
if ($validator->do_validation) {
} else {
    $validator->getErrors();
}

article about static method


any recommend tools or design pattern for re-factoring this legacy code.

Martin Fowler. Refactoring: Improving the Design of Existing Code.


Also your code contain a lot of typos.

\$\endgroup\$
  • \$\begingroup\$ thanks for you answer but actually i concern about improve code performance , make it more Extensible too \$\endgroup\$ – MR Gharibeh Nov 1 '14 at 8:03
0
\$\begingroup\$

In addition to all the good points of @podliy16:

Usability

I know that this is still common in PHP, but I really don't like using strings for control flow the way you are doing it here (with EMAIL, URL, and NorwayMobile).

There are two main problems with this approach:

  • it's unstable: if a user mistypes (eg NorwayMabile) it will not work (and typos are easy to introduce).
  • it's hard to use: how does a user know what values are acceptable? If you do use this approach, good documentation is really important. As you currently don't document this at all, a user would have to actually look into your source code to use your class.

I would either define these strings as static fields (public static $EMAIL = 'EMAIL';), and then use them from outside the class, or do it how PHP does it for eg filter_var by using define to create a global variable (I would actually prefer the first approach).

Still, good documentation would still be nice, I would add an example case to the do_validation PHPDoc (the current @param type $input_value isn't helpful at all).

Misc

  • for the default case, I wouldn't use validation_errors with INVALID_RULE_ERROR_MESSAGE, because it's not a validation error (which would be an error with the user input, and the message will most likely be echoed to the user), it's a programming error, and should be handled as such (ie, throw an exception).
  • tests should not be placed inside the class they are testing, but in a separate class (ideally in a separate test directory).
  • I'm not sure if your additional checks for the url make all that much sense. With FILTER_VALIDATE_URL we know that it is quite forgiving, and definitely doesn't offer any protection against anything. But your additional checks make the behavior of the function unexpected (and you don't document it very well either). Your additional checks do catch eg javascript://alert or http://x, but they let through javascript://al.ert or http://x.. I would just remove them, so a user gets what they expect.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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