2
\$\begingroup\$

I have a class that I am using to initialize a form for editing. Once the form is submitted, data validation is done through the setters of my class. Good or bad input will be set in order to re-display them in the form (is this bad?). Everything works fine, but I am not too happy with how I am storing and displaying the error messages.

Any suggestions on how I can improve?

Course.class.php

<?php
class Course {
    protected $_code;
    protected $_name;
    protected $_errors = array();

    public function __constructor($id) {
        if (is_int($id)) {
            $this->_load($id);
        }            
    }

    public function getCode() { return $this->_code; }

    public function setCode($value) { 
        $this->_code = $value;
        if (empty($value)) {
            $this->setError('code', 'Enter a code.');
        } else if (strlen($value) > 10) {
            $this->setError('code', 'The code you provided must be less than 10 characters.');
        }
    }

    public function getName() { return $this->_name; }

    public function setName($value) { 
        $this->_name = $value;
        if (empty($value)) {
            $this->setError('name', 'Enter a name.');
        } else if (strlen($value) > 50) {
            $this->setError('name', 'The name you provided must be less than 50 characters.');
        }
    }

    public function getError($name) {
        return isset($this->_errors[$name]) ? $this->_errors[$name] : '';
    }

    public function setError($name, $value) {
        $this->_errors[$name] = $value;
    }

    public function hasErrors() {
        return !empty($this->_errors);
    }

    public function _load($id) {
        // load record from database
    }

    public function save() {
        if (!this->hasErrors()) {
            // save record to database
        }
    }     
}
?>

I have looked around and many have suggest to use exceptions. However, I don't quite get how I can use them because wouldn't throwing an exception in one setter cause the subsequent setters from not being called.

Here is how I am currently using my class:

edit_course.php

<?php
$filter = array(
    'hidSubmit' => FILTER_VALIDATE_BOOLEAN,
    'code' => FILTER_SANITIZE_STRING, 
    'name' => FILTER_SANITIZE_STRING
);

$c = filter_input(INPUT_GET, 'c', FILTER_VALIDATE_INT);
$course = new Course($c);

$inputs = filter_input_array(INPUT_POST, $filter);
if ($inputs['hidSubmit']) {
    $course->setName($inputs['name']);
    $course->setDescription($inputs['description']);
    $course->save();
    refreshPage();
}

require_once 'header_admin.php';
?>


<form id="editForm" name="editForm" action="" method="post">
<fieldset>
<legend>Edit Course</legend>
    <label>Code</label>
    <input type="text" name="code" value="<?php echo $course->getCode(); ?>" maxlength="10" size="8" />
    <span class="error"><?php echo $course->getError('code'); ?></span>
    <label>Name</label>
    <input type="text" name="name" value="<?php echo $course->getName(); ?>" maxlength="45" size="25" />
    <span class="error"><?php echo $course->getError('name'); ?></span>
    <input type="submit" value="Save" />
    <input type="hidden" name="hidSubmit" value="1" />
</fieldset>
</form>


<?php
require_once 'footer_admin.php';
?>
\$\endgroup\$
3
\$\begingroup\$

The big disadvantage of putting the validation to the setter is that you are never able to set invalid values. In most cases this is fine, but in some scenarios a valid user input differs from a more general valid field content. In addition the validation is also executed to internal setter uses where know that they are valid.

That said, I recommend setters to be only setters and not more.

The validation itself can be placed in a separate method or even in a separate class (depending on the complexity).

If you only want to display one error message, throwing an exception within the validation should be fine.

If you want to collect multiple error messages, you might want to build a generic validation class:

$valid=new Validation();
$valid->assertNotEmpty($name,"name","Insert a name please");
$valid->assertLength($name,3,50,"name","...");
...
if ($valid->hasErrors()) ...

You could extend this to a CourseValidation if you need it at multiple locations.

$valid=new CourseValidation($code,$name);
...
\$\endgroup\$
  • \$\begingroup\$ Thank you, it works but I am not sure if I am understanding your suggestion correctly. I have updated my code to show what I did. Is that what you mean? Any more improvements that can be made? \$\endgroup\$ – Mikey Feb 17 '13 at 16:30
  • \$\begingroup\$ You should also consider Robs comments. Creating a Form class will help you to get rid of the duplication in your HTML. \$\endgroup\$ – mheinzerling Feb 17 '13 at 17:59

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.