CODEWITHSUNDEEP
phpcodeigniteruser-registration
stergosz
stergosz

Reputation: 5860

Codeigniter Signup Controller code review

I just started using a MVC framework, especially Codeigniter and i am having some trouble maintaining my code and where to place my functions(controller or model).

For now i am building a sign up system and i have a controller with the name signup.php

This is my code:

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

Class Signup extends CI_Controller {

    public function __construct()
    {
        parent::__construct();
    }

    public function index()
    {
        $this->form_validation->set_rules('username', 'Username', 'trim|required|callback_check_valid_username|min_length[6]|max_length[20]|xss_clean');
        $this->form_validation->set_rules('email', 'Email', 'trim|required|valid_email');
        $this->form_validation->set_rules('password', 'Password', 'trim|required|min_length[6]|max_length[32]');

        if ($this->form_validation->run() == false){
            $this->load->view("register/index");
        }else{
            $this->submitRegistration();
        }
    }

    public function ajaxup(){
        if ($this->input->isAjaxRequest()){
            header('Content-type: application/json');

            $error = false;
            $message = '';

            $this->form_validation->set_rules('username', 'Username', 'trim|required|callback_check_valid_username|min_length[6]|max_length[20]|xss_clean');
            $this->form_validation->set_rules('email', 'Email', 'trim|required|valid_email');
            $this->form_validation->set_rules('password', 'Password', 'trim|required|min_length[6]|max_length[32]');

            if ($this->form_validation->run() == false){
                $message = validation_errors();
                $error = true;
            }else{
                $this->_submitRegistration();
                $message = 'Successfully registered.';
            }

            $return = array(
                'error' => $error,
                'message' => $message
            );

            $return = json_encode($return);

            echo $return;

        }
    }

    public function _submitRegistration(){
        $username = $this->input->post('username');
        $email = $this->input->post('email');
        $password = $this->input->post('password');

        $data = array(
            'username' => $username,
            'email'    => $email,
            'password' => $password
        );

        $this->load->model('users_model');

        $this->users_model->register_user($data);
    }

    public function check_valid_username($username){

        $this->load->model('users_model');

        if (!$this->users_model->is_valid_username($username)){
            $this->form_validation->set_message('check_valid_username', 'The %s field should contain only letters, numbers or periods');
            return false;
        }

        return true;

    }

}

Is there anything i could write better to maintain my code and be readable?

*NOTE:*the function ajaxup is used when a user clicks the button and does an ajax call.

Thanks

Upvotes: 0

Views: 1067

Answers (2)

birderic
birderic

Reputation: 3765

Looks pretty good to me. Here are few ideas/suggestions for future improvements:

  • In index() you are calling $this->submitRegistration() but I think you want to be calling $this->_submitRegistration().

  • Since you are using the same validation rules in both the index() and ajaxup() methods you could pull pull them out into an array and either make them a property of your controller or put them into a config file.

For documentation see here and here.

$validation_rules = array(
    array(
        'field'   => 'username',
        'label'   => 'Username',
        'rules'   => 'trim|required|callback_check_valid_username|min_length[6]|max_length[20]|xss_clean'
    ),
    array(
        'field'   => 'email',
        'label'   => 'Email',
        'rules'   => 'trim|required|valid_email'
    ),
    array(
        'field'   => 'password',
        'label'   => 'Password',
        'rules'   => 'trim|required|min_length[6]|max_length[32]'
    ),
);

Then in your methods you would do something similar to $this->form_validation->set_rules($validation_rules).

  • Think about reordering your validation rules. For example, let's take a look at the rules for the username field. If check_valid_username() is making a call to the database (through the user model) then it would probably be better to validate the length requirements before. There's no use making an expensive call to the database if we can determine if the username is invalid.

  • Make your callback methods private. Right now check_valid_username() is a public method and could potentially be accessed through the URL. Prefix it with an underscore (_check_valid_username()) and then in your validation rules use callback__check_valid_username. Note the two underscores.

  • If you find yourself needing to use check_valid_username() in multiple controllers you could extend the native form validation library and put it there.

Upvotes: 3

b3n
b3n

Reputation: 1324

This looks fine to me. You seem to have all the relevant functions located in the user model and you are using the controller to access them. All I can suggest is read up on MVC theory if you feel unsure.

This is a good article: http://www.codinghorror.com/blog/2008/05/understanding-model-view-controller.html

Upvotes: 2

Related Questions