HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Validation and Form classes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andformvalidationclasses

Problem

I have written Validation and Form classes in PHP.

  • Validation class allow you to define the rules of each field and a error message.



  • Form class allow you to get a error message and also allow you to get a value of POST via $_SESSION.



I stored in $_SESSION because if validation did not pass it will need to redirect back to main Form page and repopulate all the fields using value() method.

What do you think of Validation and Form classes - what can be improved?

Validation Class

```
class Validation
{
protected $fields = array();
private $errors = array();

public function setRules($field, $errorMessage, $rules = array())
{
if (count($_POST) == 0) {
throw new Exception("The array of post parameters is empty");
}

if ($field == '') {
throw new Exception("field parameter is empty");
}

if (!is_array($rules) || count($rules) == 0) {
throw new Exception("The array of rules parameter is empty");
}

$errorMessage = ($errorMessage == '') ? $field : $errorMessage;

$this->fields[] = array(
'name' => $field,
'errorMessage' => $errorMessage,
'rules' => $rules,
);
}

public function validate()
{
if (count($_POST) == 0) {
throw new Exception("The array of post parameters is empty");
}

if (count($this->fields) == 0) {
throw new Exception("Validation rules is not set");
}

foreach ($this->fields as $field) {

$fieldName = $field['name'];
if (isset($_POST[$fieldName])) {
foreach ($field['rules'] as $rule) {

$param = false;

if ($arr = explode('=', $rule)) {
if (isset($arr[0]) && isset($arr[1])) {
$param = $arr[1];
$rule = $arr[0];
}

Solution

Short on time, but here's a few quick things:

  • Consider making individual validators rather than putting all of the different validation rules in one class.



  • One option would be to make a Field class and have Form contain a collection of Fields (with each field having a collection of Validators)



  • Pass in $_POST instead of accessing it in your class (what if you want to validate $_GET or something?)



  • Form shouldn't directly depend on $_SESSION



  • $_SESSION['posts'] = $_POST; The form shouldn't be responsible for storing state. If you want to do that, do it outside of the Form class.



  • How do you know which validation(s) failed?



  • The two exceptions thrown in validate should be reconsidered. A missing field should probably be considered empty (and thus a required validation failed), and a Form with no rules, should probably always validate as true. (Also, they should probably be specialized exception classes, not just the generic Exception)



  • return ($value != '') ? true : false; Is redundant: return ($val != '');



  • In particular, any expression of the form (expr) ? true : false can be written as (expr)



  • Be careful with implicit typecasts (for example, some of your string comparisons might should be ===)



  • $this->errors[$fieldName] = array('errorMessage' => $field['errorMessage']); You're clobbering any old errors



  • There should probably be some way to tell which elements have errors. How did you know name has an error if validate fails? How do you know it's not email?



(I will likely come back to this. Also, I haven't forgotten about your post from 2 days ago that I said the same thing on... Just been very busy :D)

Context

StackExchange Code Review Q#16018, answer score: 5

Revisions (0)

No revisions yet.