2
\$\begingroup\$

What would be the best approach to write this class.

This class validates 3 types of passwords,

  1. Normal
  2. Medium
  3. Strong

Github Link


<?php namespace azi\Rules;


use azi\Arguments;
use azi\Rules\Contracts\RuleInterface;

/**
 * Class Password
 *
 * @package azi\Rules
 */
class Password implements RuleInterface
{
    const NORMAL = 'normal';
    const MEDIUM = 'medium';
    const STRONG = 'strong';

    protected $message = null;

    /**
     * @param $field
     * @param $password
     * @param Arguments $args
     * @return mixed
     */
    public function validate($field, $password, Arguments $args)
    {
        $type = $args->getVariables();

        if ($type && $type != self::NORMAL) {
            return $this->is($type[ 0 ], $password);
        }

        return $this->isNormal($password);
    }

    /**
     * @param $strength
     * @param $password
     * @return bool
     */
    public function is($strength, $password)
    {
        $method = sprintf("is%s", ucwords($strength));
        return call_user_func([$this, $method], $password);
    }

    /**
     * @param $password
     * @return bool
     */
    public function isNormal($password)
    {
        if (strlen($password) < 8) {
            $this->message = '{field} must be at least 8 characters long';
            return false;
        }

        if (!preg_match("#[a-zA-Z]#", $password)) {
            $this->message = '{field} must contain at least one letter';
            return false;
        }


        return true;
    }

    /**
     * Checks a strong password
     *
     * @param $password
     * @return bool
     */
    public function isStrong($password)
    {
        if (!$this->isMedium($password)) {
            return false;
        }

        if (!preg_match("#[!@\#\$%^&*()_+\-=\[\]{};':\"\\|,.<>\/?]+#", $password)) {
            $this->message = '{field} must contain at least special character';
            return false;
        }

        return true;
    }

    /**
     * @param $password
     * @return bool
     */
    public function isMedium($password)
    {
        if (!$this->isNormal($password)) {
            return false;
        }

        if (!preg_match("#[a-z]#", $password)) {
            $this->message = '{field} must contain at least one lowercase letter';
            return false;
        }

        if (!preg_match("#[A-Z]#", $password)) {
            $this->message = '{field} must contain at least one uppercase letter';
            return false;
        }

        return true;
    }

    /**
     * @return mixed
     */
    public function message()
    {
        if ($this->message) {
            return $this->message;
        }

        return "{field} must be a good password";
    }
}
\$\endgroup\$
  • \$\begingroup\$ How do you expect to use the class and it’s methods? Could you add sample usage? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 16 '18 at 21:49
  • \$\begingroup\$ The class is part of a validation engine. these type of classes work as validation rules. here is the full source-code : github.com/azeemhassni/envalid \$\endgroup\$ – Azeem Hassni Dec 17 '18 at 7:08
5
+25
\$\begingroup\$
/**
 * @param $strength
 * @param $password
 * @return bool
 */
public function is($strength, $password)
{
    $method = sprintf("is%s", ucwords($strength));
    return call_user_func([$this, $method], $password);
}

This is not okay.

You're taking user input... (or something that I assume to be user input...) and then looking for a method in your own class, and then executing it. This whilst you know or at least can reasonably assume that you know which values the user input might contain.

So let's nuke it.

If we nuke it, then we need a new implementation here.

/**
 * @param $field
 * @param $password
 * @param Arguments $args
 * @return mixed
 */
public function validate($field, $password, Arguments $args)
{
    $type = $args->getVariables();

    if ($type && $type != self::NORMAL) {
        return $this->is($type[ 0 ], $password);
    }

    return $this->isNormal($password);
}

Well, we can use a switch. See also the documentation here: http://php.net/manual/en/control-structures.switch.php (I'm not familiar with PHP syntax, but you should understand the gist of the idea.)

/**
 * @param $field
 * @param $password
 * @param Arguments $args
 * @return mixed
 */
public function validate($field, $password, Arguments $args)
{
    $type = $args->getVariables();

    if ($type && $type != self::NORMAL) {
        switch ($type [ 0 ]) {
            case self::NORMAL:
                return $this->isNormal($password);
                break;
            case self::MEDIUM:
                return $this->isMedium($password);
                break;
            case self::STRONG:
                return $this->isStrong($password);
                break;
        }
    }

    return $this->isNormal($password);
}

Lastly, I'd say you should reorder your methods so they are ordered as "normal", "medium" and "strong" in the code as well. It reads better.

\$\endgroup\$

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.