Email text validator

Question

I'm using this pattern for the first time and wanted to check if this is the correct implementation.

class.validator.strategy.php

abstract class ValidatorStrategy
{
    abstract public function check( $name, $val );

}

class.text.validator.php

class TextValidator extends ValidatorStrategy
{

    public function check( $name, $val )
    {
         //logic here
    }
}

class.number.validator.php

class NumberValidator extends ValidatorStrategy
{

    public function check( $name, $val )
    {
         //logic here
    }
}

class.validator.php

include('validator.strategy.php');
include('class.text.validator.php');
include('class.number.validator.php');
include('class.email.validator.php');

class Validator
{

    //holds strategy object
    protected $validatorStrategy = array();

    //holds form field
    protected $fields = array();

    public function __construct()
    {
        $this->validatorStrategy[ 'text' ] = new TextValidator();
        $this->validatorStrategy[ 'number' ] = new NumberValidator();
        $this->validatorStrategy[ 'email' ] = new EmailValidator();
    }

    public function ruleForTextField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'text';
    }

    public function ruleForNumbertField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'number';
    }

    public function ruleForEmailField( $name, $value )
    {
        $this->fields[ $name ][ 'value' ] = $value;
        $this->fields[ $name ][ 'type' ] = 'email';

    }

    public function isValidate()
    {
        $status = 0;

        foreach ( $this->fields as $key => $val )
        {
            if ( !$this->validatorStrategy[ $val[ 'type' ] ]->check( $key, $val ) )
            {
                $status++;
            }

        if ( $status == 0 )
        {
            return true;
        }
        else
        {
            return false;
        }
    }

}

Uses:

$validator = new Validator();
$validator->ruleForTextField('username', $_POST['username'] );
$validator->ruleForNumberField('age', $_POST['age'] );
$validator->ruleForEmailField('email', $_POST['email'] );

if($validator->isValidate())
{
    echo 'validated';
}
else
{
   echo 'fail validation';
}

Inside the isValidate() method, it iterate against all validator strategy and perform appropriate logic.

Is this is the correct way to use this pattern?

Answer 1

I am not sure this is really a strategy pattern: you never rely on the ValidatorStrategy interface, there is no real choice of a strategy (there are direct calls to Validator methods) and you cannot add another strategy without braking your context object (Validator class).

What you can do is refactor Validator to inject a ValidatorStrategy instance for each field to validate. Let call this modified class FormValidator (a Validator instance validates one field only) and see how it can be implemented:

class FormValidator
{
  /**
   * @var array of StrategyValidator
   */
  protected $validators;

  /**
   *
   * @param string $name
   * @param ValidatorStrategy $strategy 
   */
  public function addValidator($name, ValidatorStrategy $strategy)
  {
    $this->validators[$name] = $strategy;
  }

  /**
   *
   * @param array $form 
   */
  public function isValid($form)
  {
    foreach ($this->validators as $name => $validator)
    {
      if (!$validator->isValid($form[$name]))
      {
        return false;
      }
    }

    return true;
  }
}

$formValidator = new FormValidator();
$formValidator->addValidator('username', new TextValidator());
$formValidator->addValidator('age', new NumberValidator());
$formValidator->addValidator('email', new EmailValidator());

// Now you can reuse $formValidator where you want
$values = array(
  'username'  => $_POST['username'],
  'age'       => $_POST['age'],
  'email'     => $_POST['email'],
);

if($formValidator->isValid($values))
{
    echo 'validated';
}
else
{
   echo 'fail validation';
}

Some advices:

• John Kraft's naming advices are excellent, you should follow them: a method starting with is*, has* returns a boolean, methods starting with get* and set* are accessors, etc.
• Always take a moment to think about you classes role, and deduce their name from it
• When manipulating booleans, use boolean vars ($status)
• When using new Myclass() in a class method, ask yourself if you are not closing the classes to future evolution, especially if MyClass implements an interface: this is a sign that the class can vary and we should rely on the interface instead (see Dependency Injection)

This last advice is illustrated by the method addValidator() in my example.

Answer 2

No, strategy pattern relies on an interface to allow composition to determine functionality at run time. Notice that it is easier to extend with new speaking functionality; and can be used to execute at run time. Here you are just using the array of the interface instantiations to do multiple validations.

I.E.

Interface for the pattern

public interface Speak {
  public void execute();
}

Implementations of the pattern

public class Quack extends Speak {
  public void execute() { /* Quack */ }
}

public class Bark extends Speak {
  public void execute() { /* bark */ }
}

Class that uses the strategy

public Animal {
  private Speak _speak;
  public Animal(Speak s) { _speak = s; }
}

Example usage

main() {
  new Animal(new Bark()); /* creates animal that barks */
  new Animal(new Quack()); /* creates animal that quacks */
}

Answer 3

I don't write php, so please bear with me; but I don't care for the isValidate method. In pseudo-code... I usually write something like

foreach(var validator in validatorCollection)
    if validator->Check = false
        return false

return true

Also, some of the method names are a bit unclear. A list of names I'd prefer to see...

• IsValid rather than Check
• IsValid rather than IsValidate

Additionally, I think your example is a poor one for the usage of the strategy pattern, but I'm assuming that's because it is a simple sample problem.

As and example, we use the strategy pattern to determine shipping methods for packages. We have a validator for UPS, and a validator for USPS, and a validator for FedEx. We pass a package object to each of the validators to determine eligibility. Each of those validators has a list of rules (weight, height, hazardous materials, etc) to determine if that package can be shipped that method.

ListOfCarriers result
ListOfCarriers carriers
foreach(carrier in carriers)
    if(carrier.canShip(package)
        result.add(carrier)
return result

A carrier's canShip method might look like...

function canShip(package)
    return weightRule.Applies(package)
        and heightRule.Applies(package)
        and hazmatRule.Applies(package)

and the weightRule might look like...

function Applies(package)
    return package.weight < 70.0