2
\$\begingroup\$

I am creating a form class for validating form data, and would like some advice on how to refactor it for best practices. Keep in mind that this is a rough draft.

Basically, there is a form abstract class (lev_form), which you would extend. In the extended classes, you would write a __construct method that creates all of the form fields, and form field validations. You could then instantiate that extended class, and run its validate_form() method to validate $_POST and $_FILES data against all of the child form fields, and their child form field validations.

It somewhat mimics the way jQuery form validation is operated, and the eventual plan is to give the class the ability to print out a jQuery validation class for the form, based on the same data that the backend validation uses.

Here is the form class:

<?php

    abstract class lev_form {

        protected $fields = array();

        abstract public function __construct() {
            // set up form fields and field vlaidations here

        }

        protected function create_field($name, $label) {
            lev::load_library('lev_forms/lev_field');
            $this->fields[] = new lev_field($name, $label);
        }

        // validates POST and FILES data
        public function validate_form() {
            try {
                foreach($this->fields as $field_object) {
                    $field_object->validate_field();
                }
            } catch (Exception $e) {
                return $e->getMessage();
            }
            return true;
        }

        // validates form using jQuery
        public function print_jquery_validator($selector) {

        }
    }
?>

Here is the class for form fields:

<?php

    class lev_field {

        protected $name;
        protected $label;
        protected $validations = array();

        public function __construct($name, $label) {
            $this->name = $name;
            $this->label = $label;
        }

        public function create_validation($type, $argument = null, $message = null) {
            lev::load_library('lev_forms/lev_field_validation');
            $this->validations[] = new lev_field_validation($type, $argument, $message, $this->name, $this->label);
        }

        public function validate_field() {
            foreach ($this->validation as $validation_object) {
                $validation_object->validate();
            }
        }
    }
?>

And here is the form field validation class:

<?php

    class lev_field_validation {

        protected $type;
        protected $argument;
        protected $message;
        protected $field_name;

        protected static $default_validation_error_messages = array(
            'required' => '[field_label] is a required field.',
            'minlength' => '[field_label] must be at least [validation_argument] characters in length.',
            'maxlength' => '[field_label] must be at most [validation_argument] characters in length.',
            'min' => '[field_label] must be at least [validation_argument].',
            'max' => '[field_label] must be at most [validation_argument].',
            'email' => '[field_label] must be a valid email address.',
            'url' => '[field_label] must be a valid URL.',
            'date' => '[field_label] must be a valid date.',
            'number' => '[field_label] must be a numeric value.',
            'digits' => '[field_label] must only contain digits.',
            'boolean' => '[field_label] must be true or false.',
            'equalto' => '[field_label] must be equal to [validation_argument].',
            'file_accept_extensions' => '[field_label] must be a valid file type.',
            'file_max_size' => '[field_label] must have a file size no greater than [validation_argument] bytes.'
        );

        public function __construct($type, $argument, $message, $field_name, $field_label) {
            $this->type = $type;
            $this->argument = $argument;
            $this->field_name = $field_name;
            if ($message) {
                $this->message = $message;
            } else if (array_key_exists($type, self::$default_validation_error_messages)) {
                $this->message = preg_replace(array('\[field_label\]', '\[validation_argument\]'), array($field_label, $argument), self::$default_validation_error_messages[$type]);
            } else {
                trigger_error('No set error message or default error message for form validation "' . $type . '"', E_USER_ERROR);
            }
        }

        public function validate() {
            $this->error_check_validation();
            $this->check_validation();
        }

        protected function error_check_validation() {
            if (array_search($this->field_name, $_POST) === true) {
                if ($this->type == 'file_accept_extensions' || $this->type == 'file_max_size') trigger_error('Use of invalid form validation "' . $this->type . '" on non-file field.', E_USER_ERROR);
            } else if (array_search($this->field_name, $_FILES) === true) {
                if ($this->type != 'file_accept_extensions' && $this->type != 'file_max_size') trigger_error('Use of invalid form validation "' . $this->type . '" on file field. You may only use this validation on non-file fields.', E_USER_ERROR);
            } else {
                trigger_error('Form field "' . $this->field_name . '" not found in $_POST or $_FILES array even though it exists in this form\'s class.', E_USER_ERROR);
            }
        }

        protected function check_validation() {
            switch ($validation->type) {
                case 'required':
                    if (!$this->required()) throw new Exception($this->message);
                    break;
                case 'callback':
                    if ($_POST[$this->field_name] !== '' && $this->argument($_POST[$this->field_name])) throw new Exception($this->message);
                    break;
                case 'minlength':
                    if ($_POST[$this->field_name] !== '' && strlen($_POST[$this->field_name]) < $this->argument) throw new Exception($this->message);
                    break;
                case 'maxlength':
                    if ($_POST[$this->field_name] !== '' && strlen($_POST[$this->field_name]) > $this->argument) throw new Exception($this->message);
                    break;
                case 'min':
                    if ($_POST[$this->field_name] !== '' && $_POST[$this->field_name] > $this->argument) throw new Exception($this->message);
                    break;
                case 'max':
                    if ($_POST[$this->field_name] !== '' && $_POST[$this->field_name] < $this->argument) throw new Exception($this->message);
                    break;
                case 'email':
                    if ($_POST[$this->field_name] !== '' && !filter_var($_POST[$this->field_name], FILTER_VALIDATE_EMAIL)) throw new Exception($this->message);
                    break;
                case 'url':
                    if ($_POST[$this->field_name] !== '' && !filter_var($_POST[$this->field_name], FILTER_VALIDATE_URL)) throw new Exception($this->message);
                    break;
                case 'date':
                    if ($_POST[$this->field_name] !== '' && !strtotime($_POST[$this->field_name])) throw new Exception($this->message);
                    break;
                case 'number':
                    if ($_POST[$this->field_name] !== '' && !is_numeric($_POST[$this->field_name])) throw new Exception($this->message);
                    break;
                case 'digits':
                    if ($_POST[$this->field_name] !== '' && !is_int($_POST[$this->field_name]) && $_POST[$this->field_name] >= 0) throw new Exception($this->message);
                    break;
                case 'boolean':
                    if ($_POST[$this->field_name] !== '' && !filter_var($_POST[$this->field_name], FILTER_VALIDATE_BOOLEAN)) throw new Exception($this->message);
                    break;
                case 'equalto':
                    if ($_POST[$this->field_name] !== '' && $_POST[$this->field_name] !== $_POST[$this->argument]) throw new Exception($this->message);
                    break;
                case 'file_accept_extensions':
                    $extension = preg_replace('/.+?\.(.+)$/', '$1', $_FILES[$this->field_name]['name']);
                    if (array_search($extension, $this->argument) === false) throw new Exception($this->message);
                    break;
                case 'file_max_size':
                    if (filesize($_FILES[$this->field_name]['tmp_name']) > $this->argument) throw new Exception($this->message);
                    break;
                default:
                    trigger_error('Validation "' . $this->type . '" does not exist. This validation was called by field "' . $this->field_name . '".', E_USER_ERROR);
            }
            return false;
        }

        protected function required() {
            if ($this->argument && !$_POST[$this->argument]) return true;
            return ($_POST[$this->field_name] === '') ? false : true;
        }
    }
?>
\$\endgroup\$
2
\$\begingroup\$

I jumped straight down to lev_field_validation as it's the most interesting class.

  1. Whenever I see a giant switch() block, I immediately suspect there are a lot of subclasses trying to break out. Each validation type should be a different subclass with an appropriate override of check_validation() (and error_check_validation() in a couple cases).
  2. You repeat $_POST[$this->field_name] !== '' twelve times which is a sure sign that this should be encapsulated in its own method. For one thing, you should check if the field exists using array_key_exists() before grabbing its value.
  3. The idiom $x ? false : true can be replaced by !$x. The second line of required() thus becomes return $_POST[$this->field_name] !== '';.
  4. Add some high-level comments explaining what is going on overall. For example, given its name I would expect required() to tell me if the value is required, yet it appears to mean that the value was passed in and must be validated. Shouldn't a "> 5" validation fail if I don't provide a value? If not, explain why or at least point that out.
\$\endgroup\$
2
  • \$\begingroup\$ Or before the switch he could do $empty = empty($_POST[$this->field_name]) and then for each check do iF(!$empty &&) \$\endgroup\$
    – RobertPitt
    Apr 8 '11 at 9:30
  • \$\begingroup\$ great suggestions. \$\endgroup\$ Apr 12 '11 at 2:40
1
\$\begingroup\$
  • The lack of comments is definitely a concern.
  • The code under the case statements is relatively unreadable, and contains a great deal of duplication. Think about how to rewrite this without so much duplicated code. The previous response suggests subclassing, I would suggest avoiding OO entirely.
  • The fact that you've opted to use OO is measurably adding a lot of pretty pointless bloat to your real code. Consider implementing the same thing without OO and comparing the results as a personal exercise.
  • Architectural concern: what if the user wants to perform custom validation?
  • Your code is not internationalisation aware. While it's good to see that you have a dedicated structure for most of the messages you have written, the triggererror() lines contain user-oriented strings right in to your code, which prevents easy internationalisation.
\$\endgroup\$
2
  • \$\begingroup\$ first comments lie. they become stale after refactoring. they become a maintainence concern in of themselves. turning this into procedural code not only clutters up the namespace with function names which in large projects with many developers can become an issue, it stops the code from being able to be autoloaded. meaning, you would have to include the file containing above anywhere that you would like to use it, or even worse, include it in every instance of your script. encapsulating it in a class allows me to autoload the class only when it is needed without adding extra lines of code. \$\endgroup\$ Apr 12 '11 at 2:37
  • \$\begingroup\$ ideally i would use small enough functions, and good enough function names, so that the public routines would be expressive enough to read without any comments at all. \$\endgroup\$ Apr 12 '11 at 2:39

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.