Question

Here is simplified version of one method i have.
Still look very complex
How would one refactor this insanity?

protected function isTextValid()
{
    if( $this->data['allow_num'] ){
        if( $this->data['allow_space'] ){
            if( preg_match( '#^[a-zA-Z0-9\s]$#', $this->val ) ){
                return true;
            }
            else{
                $this->messages = foo ? foo : bar;
                return false;
            }
        }
        else{
            if( preg_match( '#^[a-zA-Z0-9]$#', $this->val ) ){
                return true;
            }
            else{
                $this->messages = foo? foor : bar;
                return false;
            }
        }
    }
    else{
        if( $this->data['allow_space'] ){
            if( preg_match( '#^[a-zA-Z\s]$#', $this->val ) ){
                return true;
            }
            else{
                $this->messages = foo ? foor : bar;
                return false;
            }
        }
        else{
            if( preg_match( '#^[a-zA-Z]$#', $this->val  ) ){
                return true;
            }
            else{
                $this->messages =  foo ? foo: bar;
                return false;
            }
        }
    }
}

I try refactor it using state pattern but up to no avail since im not quite familiar with the pattern.
This is what i done but quickyly abondon it.

interface ValidatorState{
  public function isValid();
}

class AllowNumberAndSpace implement ValidatorState{
   protected $context;
   public function __construct(Context $context){$this->context = $context}

    public function isValid(){
       if( preg_match( .. ) ){
            return true;
        }
        else{
            $this->messages = foo ? foo : bar;
            return false;
        }
      $this->context->setState(OtherState);
    }
}

Class Context{
    protected $state;
    protected $allow_num_space_state;

    public function __construct(){
        $this->allow_num_space_state = new AllowNumberAndSpace($this);
       $this->state = $this->allow_num_space_state;
    }

   public function isValid(){
       return $this->state->isValid();
   }

  public function setState($state){$this->state = $state}
}

Obviously this only test first if branch, how can i automatically check other branch too?
Im quite sure there is something wrong with my approach.
Is there a way to fix this state pattern to test for all if branch?

Edited
What this method does is, it check whether $this->value contain expected value based on configuration attribute stored in $this->data

Example $this->data = array('allow_num'=>true), if $this->value='a1' it is consider valid Example $this->data = array('allow_num'=>false), if $this->value='a1' it is consider invalid

Is there a way to simplified this method?

Was it helpful?

Solution

First of all, try not to over-complicate this. In my opinion, the code is not complex enough to justify the use of object-oriented design patterns.

As I see it, your code basically boils down to validating an input with different regular expressions (which depend on some user-specified flags like allow_num and allow_space.

So my suggestion is the following (the basic refactoring is constructing part of the regular expression based on configuration before using this expression in any validation logic):

protected function isTextValid() {
    $allowedCharacters = 'A-Za-z';

    if ($this->data['allow_spaces']) {
        $allowedCharacters .= '\s';
    }
    if ($this->data['allow_num']) {
        $allowedCharacters .= '\d';
    }

    if (!preg_match("#^[{$allowedCharacters}]+\$#", $this->value)) {
        $this->messages = foo ? foor : bar;
        return false;
    }
    return true;
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top