Simple sign up form

Question

I have tried to build my first MVC sign up form.

Here is the user controller method register:

public function register() {
    if($this->isPost()) {
        $user = new UserModel();
        $result = $user->add($_POST);
        if(!$result) {
            $this->redirect('user/register');
        } elseif(is_array($result)) {
            $this->view->set('errors', $result);
            $this->view->addTemplate('header', 'header', array('title' => 'Error', 'bodyClass' => 'registerError'));
            $this->view->render('user/registerError');
        } else {
            $this->view->set('firstName', $_POST['firstName']);
            $this->view->addTemplate('header', 'header', array('title' => 'Thank you ' . $_POST['firstName'], 'bodyClass' => 'registerTrue'));
            $this->view->addTemplate('footer', 'footer');
            $this->view->render('user/registerTrue');
        }
    } else {
        $this->view->addTemplate('header', 'header', array('title' => 'Register', 'bodyClass' => 'userRegister'));
        $this->view->addTemplate('footer', 'footer');
        $this->view->render('user/register');
    }
}

Here is my user model method add:

public function add($data) {
    $errors = array();
    if(!FormValLib::checkNotEmpty($data)) {
        // return false;
    }
    if(!FormValLib::checkEmail($data['email'])) {
        $errors[] = 'The email you provided is not valid';
    }
    if(!FormValLib::compare($data['email'], $data['confirmEmail'])) {
        $errors[] = 'Your emails do not match';
    }
    if(!FormValLib::checkMinLength($data['password'], 6)) {
        $errors[] = 'Your password must be at least 6 characters long';
    }
    if(!FormValLib::compare($data['password'], $data['confirmPassword'])) {
        $errors[] = 'Your passwords do not match';
    }
    if($errors) {
        return $errors;
    } else {
        $q = $this->db->prepare('INSERT INTO users (name, email, password) VALUES (?, ?, ?)');
        $q->execute(array($data['firstName'] . ' ' . $data['secondName'], $data['email'], hash('sha512', $data['password'])));
        return true;
    }
}

FormValLib is just a very simple form validation class. I am not so bothered about coding standards just the design pattern in general!

Answer 1

add is doing both validation and insertion. It'd probably be better if the two tasks were separated. That'd make it possible to validate something without trying to add to the DB just yet. :P

I personally am a bit leery of returning an array of errors from add...partly because you're using true to indicate success, and $this->add($data) would be truthy even if there were errors. In a public method, IMO, that is a recipe for confusion. I'd prefer to see a result type more explicitly indicating success or error.

You might want to store the errors elsewhere (maybe as a property of the object, considering they're a direct result of its state) rather than returning them. Or, don't even store them -- if you separate validation and insertion, that list of errors can be recreated at will. Either way, you can then simply return a boolean indicating whether the insert succeeded. If false, then the caller can get those errors if it cares why the add failed.

Answer 2

I'm not a PHP expert but:

The Register method is too long. Extract each branch of the top-level conditional into separate methods.

The line $this->view->addTemplate('header'... is too long. Assign the array to an intermediate variable first.

Assign $this->view to something so you can reference it with a shorter name.

registerTrue seems like a poor name, how about registrationSuccess?