3
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thx for ur explaination, it really help \$\endgroup\$ – slier Jun 20 '11 at 18:07
4
\$\begingroup\$

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 */
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ @Suroot - Your way is the normative way of using a strategy pattern, but the OPs solution is very interesting, in that he can easily extend what he can validate by just implementing his interface and adding it to his array, so, he isn't having multiple classes changing what they are doing, but the basic concept of what he is doing is still present. \$\endgroup\$ – James Black Jun 20 '11 at 2:42
  • \$\begingroup\$ His use is more of a lookup style pattern; I'm not sure if there is a design pattern already created for this; but it does not appear to be strategy. Strategy has one interface and multiple instantiations of that interface followed by one or more classes that contain ONE of those instantiations to give the illusion that the class is changing implementation at run time. \$\endgroup\$ – Suroot Jun 20 '11 at 2:47
  • \$\begingroup\$ @Suroot yes i can implement classic strategy pattern but that will force my application code to instantiate all related class.Let say i need to validate 4 type of validation, my application code need to instantiate 4 time validator class...i dont want to force this in my application code..thats why my code en up like that \$\endgroup\$ – slier Jun 20 '11 at 14:37
  • \$\begingroup\$ @suroot see update, is that more like strategy pattern? \$\endgroup\$ – slier Jun 20 '11 at 15:17
1
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thx for ur explaination \$\endgroup\$ – slier Jun 20 '11 at 18:10

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.