Validation system asking for advice ....

[whisher06]

Hi.
This is a rough copy of the system
I would implements:


<?php
error_reporting(E_ALL | E_STRICT);
define("IS_REQUIRED", '"%s" è un campo obbligatorio') ;
define('NOT_NUM', '"%s" non contiene un numero valido') ;
class ValidatorFactory{
public function createValidator($validator,$field,$value){
if(!class_exists($validator)){
throw new Exception('Invalid object parameters');
}
return new $validator($field,$value);
}
}
abstract class AbstractValidator{
public static $error= array();
abstract function validate();
public function setError($msg){
self::$error[]= $msg;
}
public static function getErrors(){
return self::$error;
}
public static function getError(){
return array_shift(self::$error);
}
public static function isValid(){
return !(bool)count(self::$error) ;
}
}//
class IsEmpty extends AbstractValidator{
private $value= '';
private $field= '';
public function __construct($field,$value){
$this->field= $field;
$this->value= $value;
}
public function validate(){
if(empty($this->value)){
parent::setError(sprintf(IS_REQUIRED, $this->field));
}
}
}//
class IsNumeric extends AbstractValidator{
private $value= '';
private $field= '';
public function __construct($field,$value){
$this->field= $field;
$this->value= $value;
}
public function validate(){
if(!is_numeric($this->value)){
parent::setError(sprintf(NOT_NUM, $this->field));
}
}
}//
try{
$valFactory=new ValidatorFactory();
$valFactory->createValidator('IsEmpty','Username','')->validate();
$valFactory->createValidator('IsNumeric','Age','xxx')->validate();
if(AbstractValidator::isValid()){
//insert data
}
else{
foreach(AbstractValidator::getErrors() as $error)
{
echo "<p>".$error."</p>";
}
}
}
catch(Exception $e){
echo $e->getMessage();
exit();
}


?>



What do you think about ?

Bye.

[Shrike]

Just a couple of things which come to mind:

- validate() in each class does not have a return value - to my mind it should return true or false. Perhaps include a getError() method for the error message.

- Static methods: why bother? They just limit reusability. It would be preferable to use instance methods.

- Exceptions: how about using using a typed Exception rather than the standard Exception class - this would allow client code to distinguish between different types of exception.

[whisher06]

- Exceptions: how about using using a typed Exception rather than the standard Exception class - this would allow client code to distinguish between different types of exception.


Thanks for the tip ;)


- validate() in each class does not have a return value - to my mind it should return true or false. Perhaps include a getError() method for the error message.
- Static methods: why bother? They just limit reusability. It would be preferable to use instance methods.


If I don't use static method/property I've got
a system like this:


<?php
error_reporting(E_ALL | E_STRICT);
define("IS_REQUIRED", 'The following (required) field is empty : %s') ;
define('NOT_NUM', 'The following field isn\'t a number: %s') ;
class ValidatorFactory{
public function createValidator($validator,$field,$value){
if(!class_exists($validator)){
throw new Exception('Invalid object parameters');
}
return new $validator($field,$value);
}
}
abstract class AbstractValidator
{
protected $error;
function __construct(){}
abstract function validate();
abstract function setError($msg);
public function getErrors(){
return $this->error;
}
}//
class IsEmpty extends AbstractValidator{
private $value;
private $field;
public function __construct($field,$value){
$this->field = $field;
$this->value = $value;
}
public function validate(){
$isValid = TRUE;
if(empty($this->_value))
{
$isValid = FALSE;
$this->setError(sprintf(IS_REQUIRED, $this->field));
}
return $isValid;
}
public function setError($msg){
$this->error = $msg;
}
}//
class IsNumeric extends AbstractValidator{
private $value;
private $field;
public function __construct($field,$value){
$this->field = $field;
$this->value = $value;
}
public function validate(){
$isValid = TRUE;
if(!is_numeric($this->value))
{
$isValid = FALSE;
$this->setError(sprintf(NOT_NUM, $this->field));
}
return $isValid;
}
public function setError($msg){
$this->error = $msg;
}
}//
$objs= array();
$valFactory=new ValidatorFactory();
$objs[] = $valFactory->createValidator('IsEmpty','Username','');
$objs[] = $valFactory->createValidator('IsNumeric','Age','xxx');
$errors= array();
foreach($objs as $obj){
if(!$obj->validate())
{
$errors[]= $obj->getErrors()."<br>";
}
}

if((bool)count($errors)){
foreach($errors as $err){
echo $err;
}
exit();
}
else{
// insert data
}
?>


It was also an old thread, you took part in ;)


I find the former useful even if I agree with you
that static methods limit the code reusability.


Have you got any tips ?

Bye.

[Shrike]

You might encapsulate the collection of validators with another class, e.g.

class ValidatorCollection
{
private
$validators, $errors;
public function add( Validator $v )
{
$this->validators[] = $v;
}
public function getErrors()
{
return $this->errors;
}
public function isValid()
{
$valid = true;
foreach( $this->validators as $v )
{
if( !$v->isValid() )
{
$valid = false;
$this->errors[] = $v->getError();
}
}
return $valid;
}
}
$collection = new ValidatorCollection;
$collection->add( new IsEmptyValidator( 'Not null' ));
$collection->add( new IsEmptyValidator( null ));
if( $collection->isValid() )
{
// Continue processing
}
else
{
// Do something with $collection->getErrors()
}

If I was using this with an HTML form, I would want to associate values with keys. You might want to work that in somehow too.

[whisher06]

Thanks very much buddy ;)

Waiting for tips I made quite the same
think:


<?php
error_reporting(E_ALL | E_STRICT);
define("IS_REQUIRED", 'The following (required) field is empty : %s') ;
define('NOT_NUM', 'The following field isn\'t a number: %s') ;
class InvalidArgException extends Exception{
public function __construct($message, $code = 0){
parent::__construct($message, $code);
}
public function __toString(){
return " ".__CLASS__ . "<br /> Line : <b>[{$this->line}]</b><br /> Code :<b>[{$this->code}]</b><br /> Msg : {$this->message}<br /> File : <b>{$this->file}</b>\n";
}
}

abstract class ValidatorFactory{
private static $errors= array();
public static function createValidator($validator,$field,$value){
if(!class_exists($validator)){
throw new InvalidArgException("Invalid object parameters class {$validator} doesn't exist in class <b>[".__CLASS__."]</b>");
}
$validator= new $validator($field,$value);
if(!$validator->validate()){
self::$errors[]= $validator->getErrors();
}
}
public static function isValid(){
return !(bool)count(self::$errors);
}
public static function getValidators(){
return self::$errors;
}
}

abstract class AbstractValidator{
protected $error= '';
abstract protected function validate();
abstract protected function setError($msg);
public function getErrors(){
return $this->error;
}
}//

class IsEmpty extends AbstractValidator{
private $value= '';
private $field= '';
public function __construct($field,$value){
$this->field= $field;
$this->value= $value;
}
public function validate(){
$isValid= TRUE;
if(empty($this->value))
{
$isValid= FALSE;
$this->setError(sprintf(IS_REQUIRED, $this->field));
}
return $isValid;
}
public function setError($msg){
$this->error= $msg;
}
}//

class IsNumeric extends AbstractValidator{
private $value= '';
private $field= '';
public function __construct($field,$value){
$this->field= $field;
$this->value= $value;
}
public function validate(){
$isValid= TRUE;
if(!is_numeric($this->value))
{
$isValid= FALSE;
$this->setError(sprintf(NOT_NUM, $this->field));
}
return $isValid;
}
public function setError($msg){
$this->error= $msg;
}
}//

try{
ValidatorFactory::createValidator('IsEmpty','Username','');
ValidatorFactory::createValidator('IsEmpty','Username2','');
ValidatorFactory::createValidator('IsNumeric','Age','xxx');

if(!ValidatorFactory::isValid()){
foreach(ValidatorFactory::getValidators() as $err){
echo $err."<br />";
}
exit();
}
else{
// insert data
}
}
catch(InvalidArgException $e){
echo $e;
exit();
}
?>



But all in all I prefer your code ;)

Bye.

[whisher06]

Yeah, far better your solution :D
Thanks again.

Bye.



Taken http://www.blogial.net/index.php?cat=3 ;)