Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 [EDITED]

<?php
class Course {
    protected $_code;
    protected $_name;

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

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

    public function setCode($value) { $this->_code = $value; }

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

    public function setName($value) { $this->_name = $value; }

    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?

http://stackoverflow.com/questions/1651964/oop-design-question-validating-properties http://stackoverflow.com/questions/5506033/validate-on-entry-or-before-saving

Here is how I am currently using my class:

edit_course.php [EDITED]

<?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);
$code = $course->getCode();
$name = $course->getName();

$inputs = filter_input_array(INPUT_POST, $filter);
if ($inputs['hidSubmit']) {
    $code = $inputs['code'];
    $name = $inputs['name'];
    $valid = new CourseValidation($code, $name);
    if (!$valid->hasErrors()) {
        $course->setCode($code);
        $course->setName($name);
        $course->edit();
        refresh();
    }
}

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 $code; ?>" maxlength="10" size="8" />
    <span class="error"><?php echo isset($valid) ? $valid->getError('code') : ''; ?></span>
    <label>Name</label>
    <input type="text" name="name" value="<?php echo $name; ?>" maxlength="45" size="25" />
    <span class="error"><?php echo isset($valid) ? $valid->getError('name') : ''; ?></span>
    <input type="submit" value="Save" />
    <input type="hidden" name="hidSubmit" value="1" />
</fieldset>
</form>


<?php
require_once 'footer_admin.php';
?>

Validation.class.php [EDITED]

<?php
class Validation {
    private $_errors = array();

    public function assertNotEmpty($value, $name, $message) {
        if (empty($value)) {
            $this->setError($name, $message);
        }
    }

    public function assertLength($value, $min, $max, $name, $message) {
        $length = strlen($value);
        if ($length < $min or $length > $max) {
            $this->setError($name, $message);
        }
    }

    public function getError($key) {
        return array_key_exists($key, $this->_errors) ? $this->_errors[$key] : '';
    }

    public function setError($key, $value) {
        if (!array_key_exists($key, $this->_errors)) {
            $this->_errors[$key] = $value;
        }
    }

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

class CourseValidation extends Validation {

    function __construct($code, $name) {
        $this->assertNotEmpty($code, 'code', 'Enter a code.');
        $this->assertLength($code, 1, 10, 'code', 'The code you provided must be less than 10 characters.');
        $this->assertNotEmpty($name, 'name', 'Enter a name.');
        $this->assertLength($name, 1, 50, 'name', 'The name you provided must be less than 50 characters.');
    }
}
?>  
share|improve this question
3  
check out how other frameworks use forms for validation : symfony.com/doc/current/book/forms.html –  Rob Apodaca Feb 17 '13 at 15:57
    
Oh. I'd have to rewrite many things but thanks though. I'll keep that in mind next time. –  Mikey Feb 17 '13 at 16:33

1 Answer 1

up vote 2 down vote accepted

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);
...
share|improve this answer
    
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? –  Mikey Feb 17 '13 at 16:30
    
You should also consider Robs comments. Creating a Form class will help you to get rid of the duplication in your HTML. –  mnhg Feb 17 '13 at 17:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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