1
\$\begingroup\$

Just wanted to post my registration controller and see what anybody thought about it.

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
/**
 * Register
 * 
 */
class Register extends CI_Controller
{
    /**
     * Register::__construct()
     * 
     * @return
     */
    public function __construct()
    {
        parent::__construct();

        $this->load->model('user_model', 'user');
    }

    /**
     * Register::index()
     * 
     * @return
     */
    public function index()
    {        
        $this->data['assets']['css_includes'] = array('mycss.css');
        $this->data['assets']['js_includes'] = array('register.js');

        $this->template
            ->title('Wrestling Manager', 'Register')
            ->set_layout('usermanagement_layout_view')
            ->set_partial('header', 'partials/header_view')
            ->set_partial('footer', 'partials/footer_view')
            ->build('registration_form_view', $this->data);
    }

    /**
     * Register::process()
     * 
     * @return
     */
    public function process()
    {
        $output_array = array();
        $output_array['status'] = 'notice';
        $output_array['message'] = 'The following action failed to submit. Please try again later.';
        $output_array['errors'] = 'No errors to report.';

        $this->form_validation->set_rules('username', 'Username',
            'trim|required|xss_clean|callback__unique_username');
        $this->form_validation->set_rules('email_address', 'Email Address',
            'trim|required|xss_clean|valid_email|callback__unique_email_address');
        $this->form_validation->set_rules('password', 'Password',
            'trim|required|xss_clean|min_length[6]|max_length[12]');

        if ($this->form_validation->run() == FALSE)
    {
            // Form validation did not pass successfully. Report back to the user there was error(s) on the form.
            $output_array['status'] = 'error';
            $output_array['message'] = 'The following form did not validate successfully. Please fix the form errors and try again.';
            $output_array['errors'] = $this->form_validation->error_array();
    }
    else
    {
    // Form validation passed successfully.
            // Set up variables from post data.
            $post_username = $this->input->post('username');
            $post_email_address = $this->input->post('email_address');
            $post_password = $this->input->post('password');
            $registration_date = gmdate('Y-m-d H:i:s', time());
            $hashed_password = $this->user->generate_password_hash($post_password);
            $registration_key = sha1($registration_date.';;'.$post_email_address.';;'.$hashed_password[0]);

            // Develop the array of post data for sending to the model.
            $data = array(
                'username' => $post_username,
                'email_address' => $post_email_address,
                'password' => $hashed_password[0],
                'password_hash' => $hashed_password[1],
                'registration_date' => $registration_date,
                'registration_key' => $registration_key
            );

            $user_id = $this->user->create_user($data);

            // Create the user.
            if (!is_numeric($user_id))
            {
                // User was not created successfully.
                $output_array['status'] = 'error';
                $output_array['message'] = 'The user was not created successfully.';
            }
            else
            {
                // User was successfully created and the user needs to verify their account.
                // Send registered an email informing them how to validate their account.
                $company_name = $this->config->item('company_name');
                $company_email_address = $this->config->item('company_email');

                $this->load->library('email');
                $this->email->from($company_email_address, $company_name);
                $this->email->to($post_email_address);
                $this->email->subject($company_name.' Registration');
                $message = 'Welcome to '.$company_name.','."\r\n \r\n";
                $message .= 'Thank you for joining the '.$company_name.' Team. ';
                $message .= 'We have listed your registration details below. Make sure you save this email.';
                $message .= 'To verify this account please click the following link.'."\r\n \r\n";
                $message .= anchor('register/verify/'.$registration_key, 'Click Here To Activate Your Account', '')."\r\n";
                $message .= 'Please verfiy your account within 2 hours, otherwise your registration will become invalid and you will have to register again.'."\r\n \r\n";
                $message .= 'Your email address: '.$post_email_address."\r\n";
                $message .= 'Your Password: '.$post_password."\r\n \r\n";
                $message .= 'Enjoy your stay here at '.$company_name.'.'."\r\n \r\n";
                $message .= 'The '.$company_name.' Team';
                $this->email->message($message);
                $this->email->send();

                $output_array['status'] = 'success';
                $output_array['message'] = 'The user was created successfully. Please check your email for the link to activate your account.';
            }
        }

        echo json_encode($output_array);
    }

    /**
     * Register::verify()
     * 
     * @return
     */
    public function verify()
    {

    }

    /**
     * Register::_unique_email_address()
     * 
     * @param mixed $str
     * @return
     */
    public function _unique_email_address($str)
    {
        $this->db->where('user_login.email_address', $this->input->post('email_address'));

        $user = $this->user->get_user();

        if (count($user))
        {
            $this->form_validation->set_message('_unique_email_address', 'This email address is already available');
            return FALSE;
        }
        return TRUE;
    }

    /**
     * Register::_unique_username()
     * 
     * @param mixed $str
     * @return bool
     */
    public function _unique_username($str)
    {
        $this->db->where('user_info.username', $this->input->post('username'));

        $user = $this->user->get_user();

        if (count($user))
        {
            $this->form_validation->set_message('_unique_username', 'This username is already available');
            return FALSE;
        }
        return TRUE;
    }
}
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Direct access to a class file will just result in a blank page as there is no output being produced by it. However, there are better ways of doing what you are trying to do. For instance, you could use an .htaccess file. The following .htaccess file, if placed in the base directory of your site, will prevent all direct access to PHP files, excepting the index file.

<Files *.php>
    Order Deny,Allow
    Deny from all
</Files>

<Files index.php>
    Order Allow,Deny
    Allow from all
</Files>

There are caveats, such as AJAX scripts being considered direct access, but this is easily overcome with another .htaccess file and a header check.

Your doccomments are more or less useless. Doccomments are meant to provide documentation about methods and classes and are usually used in conjunction with an IDE to provide hoverable tooltips or autocompletion. Your short description, which should briefly describe the purpose, instead reiterates the method's address, which, if this isn't obvious in context, will automatically be provided in a good IDE. Additionally, you emphasize that each method has a return, but don't explain what its supposed to be returning, nor does it actually return anything. No comments are better than misleading comments. See this documentation on doccomments for appropriate usage.

Appending variable type to a variable name is redundant and just adds to the amount of typing you have to do. It should be obvious from its context that $output_array is an array. However, this is minor and could be considered a stylistic choice.

If you find yourself performing a similar task more than once, you can more easily perform said task with a loop/function. This also makes extending functionality later much easier.

$ruleset = array(
    array(
        'username',
        'Username',
        'trim|required|xss_clean|callback__unique_username',
    ),
    //etc...
);

foreach( $ruleset AS $set ) {
    list( $field, $title, $rules ) = $set;
    $this->form_validation->set_rules( $field, $title, $rules );
}

Here's another form of repetition that would be easier to encapsulate in a private method. Every field in your $output_array seems to be changed depending on the state. The overall structure remains the same, just the contents change.

private function _getOutput( $status, $message, $errors = 'No errors to report.' ) {
    return compact( 'status', 'message', 'errors');
}

//usage
$output_array = $this->_getOutput(
    'notice',
    'The following action failed to submit. Please try again later.'
);

Some things you might want to look into would be Single Responsibility Principle and Don't Repeat Yourself (DRY). Both areas would significantly improve your code.

\$\endgroup\$
2
  • \$\begingroup\$ That's awesome. However I made some changes and don't like how big my controller is. What is there that I can do to fix this. pastebin.com/sXH06ds2 \$\endgroup\$ Commented Nov 22, 2013 at 0:31
  • \$\begingroup\$ Any additional ideas? Check my update. \$\endgroup\$ Commented Nov 26, 2013 at 23:00

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.