CODEWITHSUNDEEP
phpdesign-patterns
veilig
veilig

Reputation: 5135

Would you utilize strategy pattern here?

Is this code a good fit for the strategy pattern?

public function isValidEmail($email, $organization)
{   
    switch ($organization) {
        case 'USAF':
        case 'Army':
        case 'USMC':
        case 'Navy':
        case 'SOCOM':
            return (preg_match('/[.]mil$/i', $email) === 1);
            break;

        case 'Federal Gov.':
            return (preg_match('/[.]gov$/i', $email) === 1);
            break;

        case 'State/Local Gov.':
            $regionCollection = Mage::getModel('directory/region')
                ->getResourceCollection()
                ->addCountryFilter(array('US'))
                ->load();

            $stateAbbr = array();

            // Cycle through state abbreviations for match
            foreach ($regionCollection as $region) {
                $stateAbbr[] = strtolower($region->getCode());
            }
            $states = implode('|', $stateAbbr);

            return (preg_match("/[\.|@]{1}($states){1}\.us$/i", $email) ===
            break;

        case 'USCG':
        case 'DOD':
        case 'Defense Industry':
            return true;
            break;

        default:
            return false;
            break;
    }
    // It got past somehow?
    return false;
}

I was assuming you could have a simple interface to define a validate method but I'm slightly confused on how to handle the two odd ball cases in the switch statement that return true/false w/ no email logic.

interface ValidInterface
{
    public function isValid($email);
}

Upvotes: 0

Views: 98

Answers (2)

Anders Sewerin Johansen
Anders Sewerin Johansen

Reputation: 1666

I think the relevant design pattern here is Chain of Responsibility. It's essentially an object oriented version of the switch statement. You have a list (or tree) of objects. They each have the capability to determine if they should handle the issue (equivalent to the "case" statement). If they don't, the decision is passed on to the successors (the next "case" statement). You may or may not have a catch-all, equivalent to the "default:" statement.

Why then use Chain of Responsibility instead of a switch statement or the proposed map of string to object?

I submit that they are specific implementations that achieve the same thing. A more generic implementation may give you more degrees of freedom or type safety. The switch solution imposes very real practical limits on the amount of code you can put at each case / the number of cases you can reliably handle and maintain. Case in point: in your code the branch for state/gov is quite complex, and putting that in an object or a method would clean up the code considerable.

Upvotes: 0

casablanca
casablanca

Reputation: 70721

You're on the right track with your validator interface (I would name it something more meaningful like EmailValidator). You can then create a mapping of organizations to validators and look this up when performing the validation:

private $validators = array(
  'USAF' => new MilitaryEmailValidator(),
  'Army' => new MilitaryEmailValidator(),
  ...
  'Defense Industry' => new TrueEmailValidator()
);

public function isValidEmail($email, $organization) {
  if (isset($this->validators[$organization]))
    return $this->validators[$organization])->isValid($email);

  // default return
  return false;
}

Upvotes: 2

Related Questions