User Registration

Question

I'm fairly new to PHP and am very interested in learning best practices, etc. I have just created a user registration system mostly in OOP that will dump user info into a database I've created. This is nothing revolutionary, but can you all tell me if my code is sound and clean? I want to be sure I'm on track for using best practices.

INDEX

<?php

require('./includes/functions.php');
require('./classes/usernameAuth.php');
require('./classes/checkPassword.php');
require('./classes/insertUser.php');
require('./classes/checkEmail.php');

if (isset($_POST['register'])) {

    $conn = dbConnect('localhost', 'db_admin', 'kfor.com', 'bobs_auto_parts');

    $checkUsername = new usernameAuth($_POST['username'], 6);

    $checkPassword = new checkPassword($_POST['password'], $_POST['confirmPassword'], 8);
    $checkPassword->reTyped();
    $checkPassword->requireMixedCase();
    $checkPassword->requireNumbers(2);
    $checkPassword->requireSymbols(2);
    $passwordOK = $checkPassword->check();

    $emailCheck = new checkEmail($_POST['email']);
    $emailCheck->validEmail($_POST['email']);
    $statusOK = $emailCheck->getStatus();

    if ($statusOK && $passwordOK) {

        $insertUser = new insertUser($_POST['username'], $_POST['password'], $_POST['email'], $conn);
        $success = $insertUser->checkSuccess();
    }

    $errors = array_merge($checkPassword->getErrors(), $checkUsername->getErrors(), $insertUser->getErrors());
}

?>

usernameAuth.php

<?php

    class usernameAuth
    {
        protected $username;
        protected $minUsernameChars;
        protected $errors = array();

        public function __construct($username, $minUsernameChars = 6)
        {
            $this->username = $username;
            $this->minUsernameChars = $minUsernameChars;

            if (strlen($this->username) < $this->minUsernameChars) {
                $this->errors[] = "Username must be at least $minUsernameChars characters.";
            }

            if (preg_match_all('/\s/', $this->username)) {
                $this->errors[] = "Username should not contain any spaces";
            }

            return $this->errors;
        }

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

checkPassword.php

<?php

    class checkPassword
    {
        protected $password;
        protected $reTyped;
        protected $minPasswordChars;
        protected $minNumbers = 0;
        protected $minSymbols = 0;
        protected $mixedCase = false;
        protected $errors = array();

        public function __construct($password, $confirmPassword, $minPasswordChars = 8)
        {
            $this->password = $password;
            $this->reTyped = $confirmPassword;
            $this->minPasswordChars = $minPasswordChars;
        }

        public function reTyped() {
            if ($this->password != $this->reTyped) {
                $this->errors[] = "Passwords don't match.";
            }
        }

        public function requireMixedCase()
        {
            $this->mixedCase = true;
        }

        public function requireNumbers($num = 1)
        {
            if (is_numeric($num) && $num > 0) {
                $this->minNumbers = (int)$num;
            }
        }

        public function requireSymbols($num = 1)
        {
            if (is_numeric($num) && $num > 0) {
                $this->minSymbols = (int)$num;
            }
        }

        public function check()
        {
            if (preg_match('/\s/', $this->password)) {
                $this->errors[] = "Password cannot contain spaces.";
            }

            if (strlen($this->password) < $this->minPasswordChars) {
                $this->errors[] = "Password must be at least $this->minPasswordChars characters.";
            }

            if ($this->mixedCase) {
                $pattern = '/(?=.*[a-z])(?=.*[A-Z])/';
                if (!preg_match($pattern, $this->password)) {
                    $this->errors[] = "Password should include uppercase and lowercase characters.";
                }
            }

            if ($this->minNumbers) {
                $pattern = '/\d/';
                $found = preg_match_all($pattern, $this->password, $matches);

                if ($found < $this->minNumbers) {
                    $this->errors[] = "Password should contain at least $this->minNumbers number(s).";
                }
            }

            if ($this->minSymbols) {
                $pattern = "/[-!$%^&*(){}<>[\]'" . '"|#@:;.,?+=_\/\~]/';
                $found = preg_match_all($pattern, $this->password, $matches);

                if ($found < $this->minSymbols) {
                    $this->errors[] = "Password should contain at least $this->minSymbols non-alphanumeric character(s).";
                }
            }

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

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

insertUser.php

<?php

    class insertUser
    {
        protected $password;
        protected $username;
        protected $email;
        protected $errors = array();
        protected $success;

        public function __construct($username, $password, $email, $conn)
        {
            $this->username = $username;
            $this->password = $password;
            $this->email = $email;

            $salt = time();
            $pwd = sha1($this->password . $salt);

            $sql = 'INSERT into users (username, password, salt, email) VALUES (?, ?, ?, ?)';
            $stmt = $conn->stmt_init();
            $stmt = $conn->prepare($sql);
            $stmt->bind_param('ssis', $this->username, $pwd, $salt, $this->email);
            $stmt->execute();

            if ($stmt->affected_rows == 1) {
                $this->success = "$this->username has registered.  You can now log in.";
            } elseif ($stmt->errno == 1062) {
                $this->errors[] = "$this->username or $this->email is already in use.  Please redo.";
            } else {
                $this->errors[] = 'Sorry, there was an issue with the database.';
            }

            return $this->success;
        }

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

        public function checkSuccess() {
            return $this->success;
        }
    }

checkEmail.php

    class checkEmail {

        protected $email;
        protected $status;

        public function __construct($email) {
            $this->email = $email;

            if (empty($this->email) || !$this->validEmail($email)) {
                $this->status = "Please provide a name and a valid email address.";
            } else {
                $this->status = 'Great Email!';
            }
        }

        public function validEmail($email) {
            return filter_var($this->email, FILTER_VALIDATE_EMAIL);
        }

        public function getStatus() {
            return $this->status;
        }


    }

Answer 1

I don't think you understand OOP. Well actually, you don't understand OOP!

Naming

Class names should be nouns. E.g.: UserPasswordValidator or EmailValidator. The methods on those classes should be verbs e.g. EmailValidator::validate().

If you stick to that rule, code should read like poetry. And everytime somethings seems of, a big loud error should appear. For instance, the actor of the given verb should be the object itself. Writing $user->sendEmail() generally indicated bad design for it is not the user that is sending an email, but instead it's a MailerClient that sends an Email to a User:

$mailerClient->sendEmailToUser($email, $user);

Now that is poetry. It tells us everything we need to know about the code. Comments are not required

Single responsibility

Every class should have one SINGLE responsibility. All your classes fail for they do some stuff, and they somehow all know what message to display to the User.

Let's look at the following code:

$emailCheck = new checkEmail($_POST['email']);
$emailCheck->validEmail($_POST['email']);
$statusOK = $emailCheck->getStatus();

It's hard to say what goes on. $emailCheck is an Object of type CheckEmail. No idea what that means. It also needs the same variable twice.

validEmail probably needs a valid email to check the first one against? I think... It might set some valid email internally? I think.

En the last line returns a status. Probably along the lines of true|false. Or some constants CheckEmail::STATUS_OK. Turns out to be a hard-coded string. Useless.

Comparte that to how I would write it (this doesn't mean it's the best ever - it will be close to it)

$emailValidator = new EmailValidator();
$isValidEmail = $emailValidator->validate($_POST['email']);

Tell me how to use it

Interfaces, where are they? Your password checker and your email checker are in fact both concrete implementation of a 'checker'. The first one uses check() and the other one validEmail(). Why? Thisonly makes code hard to read.

So let's fix that

interface Validator
{
    public function validate($input);
}

A very simple implementation will be:

class BooleanValidator
{
    public function validate($input) {
        return is_boolean($inpu);
    }
}

This abstract Validator might be a little over simplistic. It will eventually make us write code like this:

class StringSmallerThenTwhoHundredAndFiftyFiveValidator
{
    public function validate($input)
    {
        return strlen($input) < 255;
    }
}

class StringSmallerThenSixtyFourValidator
{
    public function validate($input)
    {
        return strlen($input) < 64;
    }
}

Or even worse: class StringLargerThenSixtyFourAndSmallerThenFourHundred. To tackle this problem, we go back to our natural language. You know, human language.

A Validator can validate() a given $input according to a set of validation rules.

$validator = new Validator(
    new SmallerThenSixtyFour,
    new LargetThenZero
);
$validator->validate($input);

A part from that the Validator should be able to tell what validation rules passed an what didn't:

$validator->passed('SmallerThenSixtyFour');  //true || false

A smart person would ofcource refractor the rules to:

$validator = new Validator(
    new MaxSizeRule(63),
    new MinSizeRule(0)
);

If writing new all over the place becomes a pain in the a$$. We could use a FactoryPattern that creates the validator for us given some custom validation rule language:

ValidatorFactory::make('max:63,min:0,unique');

Laravel does a pretty good job here. Or if you want to go all the way with validation, why not look into how symfony did it, they implemented the JSR303 Bean Validation specification (143 pages of pure fun).

To sum up

Keep every little piece of code stupid. The less it knows, the better. Use interfaces, or better. Reuse interfaces. Think abstract, and once in a while, go all the way.

Some other small remarks: look into the php-fig standard.