2
\$\begingroup\$

Everything in this code is completely working, but I still feel that this code needs to be refactored. any suggestions?

<?php
    class Db_CheckUsername{
        protected $_conn;
        protected $_username;
        protected $_minimumChars = 8;
        protected $_errors = array();

        public function __construct($username, $conn){
            $this->_username = $username;
            $this->_conn= $conn;
        }

        public function check(){

            $sql = "SELECT * FROM accounts WHERE username = '{$this->_username}'";

            $result = $this->_conn->query($sql);
            $numRows = $result->num_rows;

            if(preg_match('/\s/',$this->_username)){
                $this->_errors[] = "Spaces are not allowed";
            }

            if($numRows > 0){
                $this->_errors[] = "Username Taken";
            }

            if(strlen($this->_username) < $this->_minimumChars){
                $this->_errors[] = "Username Should Contain, atleast {$this->_minimumChars} chars";
            }

            return $this->_errors ? false : true;
        }

        public static function isUsernameCorrect($username,$conn){

        }

        public function getErrors(){
            return $this->_errors;
        }
    }
\$\endgroup\$
2
  • \$\begingroup\$ consider writing ORM model \$\endgroup\$
    – Yang
    Jun 24, 2012 at 20:01
  • \$\begingroup\$ one function does perform too many tasks. Drop this one into small ones \$\endgroup\$
    – Yang
    Jun 24, 2012 at 20:08

2 Answers 2

2
\$\begingroup\$

I meant to write one or two brief points when I started writing this, but erm, ended up a bit more :). Anyway, some of the stuff is quite briefly explained, so if anything needs more elaboration/defense, let me know and I'll edit in more information.

SQL Injection

Your query that checks for the username is open to SQL injection. You should use prepared statements.

Don't pass in the username

If you pass the username to check() instead of to the constructor, the object can be reusable.

public function check($username) { ... }

Allow configuration

You might as well allow $_minimumChars to be configured. There's a lot of ways to go about this, but as long as it's only one configuration item, the easiest way would be an argument in the constructor:

public function __construct(mysqli $conn, $minimumChars = 8)
{
    $this->_conn = $conn;
    $this->_minimumChars = $minimumChars;
}

Be strict about types when possible (and reasonable)

If the signature is:

public function __construct(mysqli $conn, $minimumChars = 8)

And the object is created accidentally with something that is not an instance of mysqli, the error will be caught much sooner than if the strict check is not there. Not particularly required, just there's not a very strong reason against it.

Break it up into more classes -- Part 1 - Validators

The benefit of this is very arguable with a small code base, however, later down the road, it will likely be nice. Consider breaking each validation into its own class:

interface Validator {
    public function isValid($input);
    public function getErrors();
}

//Because I'm lazy :p
class Validator_AbstractValidator {

    private $_errors = array();

    abstract public function isValid($input);

    public function getErrors()
    {
        return $this->_errors;
    }

    protected function _addError($message) {
        $this->_errors[] = $message;
    }

}

class Validator_NoSpaces extends Validator_AbstractValidator {
    public function isValid($input) {
        //You would probably want to add a check that $input can be cast to a string
        if (preg_match('/\s/', $input)){
            $this->_addError("Spaces are not allowed");
            return false;
        }
        return true;
    }
}

class Validator_MinimumLength extends Validator_AbstractValidator {

    protected $_minLength;

    public function __construct($minLength)
    {
        if (!is_int($minLength) && !ctype_digit($minLength)) {
            throw new Validator_Exception("minLength must be an integer");
        }
        $this->_minLength = (int) $minLength;
    }

    public function isValid($input)
    {
        //Once again, should probably have some kind of validation that $input
        //can be cast to a string
        if (strlen($input) > $this->_minLength) {
            return true;
        } else {
            $this->_addError("Input must be at least {$this_minLength}");
            return false;
        }
    }

}

The advantage of this is that it allows you to abstract away the idea of validations, and it allows you to centralize the management of validation rules. For example, suppose that your username and email address fields both had the requirement that they not contain spaces.

Now, what if you want to change the logic from the regular expression, to a more strict, strpos($input, ' ') === false? If the validations are all abstracted away, you only have to change this code in one place. If you've repeated the low level validations inside of your Db_CheckUsername, you have to change it in two places. (This is one of the major aspects of DRY by the way.)


You may have noticed that the error messages are hard coded. This would likely not be the desired behavior. What you could do there is store the errors in some kind of array indexed by error type, and then instead of _addError adding a raw message, it would add an error type (which would map to an error). (Note: this approach is 100% stolen from the Zend Framework validators :))

Break it up into more classes -- Part 2 - Element

This obviously leaves your code in a rather awkward spot, though the obvious solution of replacing the validations in the check method with the validators is essentially the solution.

What I would do is abstract away the idea of form element. You could take the idea as far as a you want (rendering, data manipulation, so on), but I'll just focus on validation:

//We want the element to have an isValid method and a getErrors method()
//(you could make a separate interface for this, but really an element is
//just a collection of Validator instances)
interface Form_ElementInterface extends Validator { }

class Form_Element_AbstractElement extends Validator_AbstractValidator implements Form_ElementInterface
{

    protected $_validators = array();

    public function addValidator(Validator $validator)
    {
        $this->_validators[] = $validator;
    }

    public function getValidators()
    {
        return $this->_validators;
    }

    protected function _addErrors(array $errors)
    {
        foreach ($errors as $error) {
            $this->_addError($error);
        }
    }

    public function hasErrors()
    {
        return (count($this->getErrors()) !== 0);
    }

    public function isValid($input)
    {
        foreach ($this->_validators as $validator) {
            if (!$validator->isValid($input)) {
                $this->_addErrors($validator->getErrors());
            }
        }
        return !$this->hasErrors();
    }

}

class Form_Element_Username extends Form_Element_AbstractElement
{
    public function __construct($minLength = 8)
    {
        $this->addValidator(new Validator_NoSpaces());
        $this->addValidator(new Validator_MinimumLength($minLength));
        //...
    }
}

Some of the names have gotten a little bleh, but hopefully the concept is still there. (I can never decide what to name interfaces/abstract classes...)

I don't particularly like the design of Form_Element_Username there. I would probably just use the elements from the outside instead of creating a complicated graph of objects and dependencies (a mysqli would have to be passed through there for example).

$username = new Form_Element();
$username->addValidator(new Validator_NoSpaces());
$username->addValidator(Validator_UserNotExists($db));

Or if you decided to take the flexible error message route, it may look like:

$username = new Form_Element();
$username->addValidator(new Validator_NoSpaces(array(
    'messages' => array(
        Validator_NoSpaces::CONTAINS_SPACE => "Usernames cannot contain spaces"
    ),
)));
$username->addValidator(Validator_UserNotExists(array(
    'db' => $db,
    'messages' => array(
        Validator_UserNotExists::USERNAME_EXISTS => "The username provided already exists",
    ),
)));

Note that passing in an array of arguments instead of proper separate parameters is often bad practice. (In fact, it's definitely bad practice here.) It has the advantage though of allowing you to easily store data in config files and then construct validators based on that.

Break it up into more classes -- Part 3 - Form

(Not going to write code on this as the concept should be clear by now.)

Just as an Element is a collection of validations, a form is, in a lot of ways, just a collection of elements. You could extend the idea of elements and validators to entire forms which would provide centralized management of form logic, and much cleaner use than the 10-20 lines it would take to make each element in the way presented above.

class Form_UserRegistration extends Form_AbstractForm
{
    //In the constructor or some kind of setup method (called by the abstract constructor),
    //you would want to a username element, a password element, and so on
}

Then the usage:

$errors = null;
$regForm = new Form_UserRegistration();

if ($_SERVER['REQUEST_METHOD'] === "POST") {
    if ($regForm->isValid($_POST)) {
        //Create the user
    } else {
        //Render these under the form or something
        $errors = $regForm->getErrors();
    }
}

Design note

It's a bit of an odd design to mutate the errors array behind the scenes while only returning a bool from isValid. In particular, it may be a bit cleaner design to directly return the array of errors. I didn't go with this approach because it complicates use of the objects, and the framework I'm most familiar with does it with two separate methods, so it's just what I'm used to.

Basically, the isValid method could look like:

For completeness:

public function validate($input) {
    $errors = array();

    if (...) {
        $errors[] = "...";
    }

    return $errors;

}

//Usage:
$errors = $obj->validate($input);
if (count($errors)) {
    //errors
} else {
    //success
}

(Or you could return true in place of the empty array)

ZF Credit

I should mention that Validator and Element concepts are highly based on the Zend Framework Zend_Validate_Interface and Zend_Element_Abstract classes.

\$\endgroup\$
6
  • \$\begingroup\$ "If you pass the username to check() instead of to the constructor, the object can be reusable." - Then, the $_error should not be a class instance. Anyway, +1 :) \$\endgroup\$
    – palacsint
    Jun 24, 2012 at 21:46
  • \$\begingroup\$ @palacsint I considered that quite a bit when making the Validator interface and couldn't decide how I feel about it. I suppose it is bad design to silently mutate an object as a side effect. Will edit the post in a bit when I get the chance. (Happen to know if there's a name for that by the way?) \$\endgroup\$
    – Corbin
    Jun 24, 2012 at 22:16
  • \$\begingroup\$ Can you show me how am I going to used Part 1 and Part 2, with implementations, I do get the concept, but the implementation is completely vague \$\endgroup\$
    – user962206
    Jun 25, 2012 at 2:01
  • \$\begingroup\$ @user962206 Have provided complete implementations of parts of it, so am not sure what you mean. Do you mean like an entire form from submission to creation? Or what? \$\endgroup\$
    – Corbin
    Jun 25, 2012 at 2:51
  • \$\begingroup\$ btw in Form_Element_AbstractElement, did you declare hadErrors variable? I can't seem to find it \$\endgroup\$
    – user962206
    Jun 25, 2012 at 3:18
1
\$\begingroup\$

The Spaces are not allowed and the Username should contain, at least... checks could be in the constructor (or at least before the SQL query). Furthermore, there is no point to run the SQL query if the username cannot be valid (it won't be in the database anyway).

The isUsernameCorrect($username,$conn) function is empty. Is it really required?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.