Welcome Guest, Not a member yet? Register   Sign In
Seeking Advice
#1

[eluser]xjohnson[/eluser]
Hi -

I'm not a professional coder, and I'm working on code that seems too long. I'd like to share it with you and get your advice on how to make it better.

Code:
public function index()
{
    $this->form_validation->set_rules( // code );
    $this->form_validation->set_message( // code );

    if ( $this->form_validation->run() )
    {
        // create a confirmation url to email to the user to click on to confirm the sign up process
        $conf_url = $this->_get_confirmation_url();

        // collect the creation date and account number
        $created = $account_number = strtotime( "now" );
        $created = date( 'Y-m-d H:i:s', $created );

        // generate an account number
        $account_number = $this->_account_number( $account_number );

        // update database with user's information
        $this->load->model( 'users_model' );
        $insertion_success = $this->users_model->addUser( array(
            'firstname'    =>    $this->input->post( 'firstname' ),
            'lastname'    =>    $this->input->post( 'lastname' ),
            'email'        =>    $this->input->post( 'email' ),
            'password'    =>    $this->input->post( 'password' ),
            'conf_email'    =>    $conf_url,
            'status'        =>    'active',
            'created'        =>    $created,
            'account_number'=>    $account_number
        ) );

        if ( $insertion_success !== FALSE )
        {
        # Great!  Sign-up info was successfully inserted into database.
        # So, send confirmation email to new user.

            $message_data = array(
                // code here
            );
            $message = $this->load->view( 'email/email_signup', $message_data, TRUE );
            $mailing_options = array(
                'message'        => $message,
                'to_address'    => $message_data['email'],
                'to_name'        => $message_data['first_name'],
                'from_address'    => '',
                'from_name'    => 'Service Team',
                'subject'        => ''
            );
            $result_mail_to_user = $this->_send_mail( $mailing_options );

            if ( ! $result_mail_to_user )
            {
            # Uh, oh!  Sending the confirmation email to user failed.  So, send an email to admin about it.
            # Let new user know that they'll receive a confirmation email in 24 hours.

                $message = $this->load->view( 'email/anoncamp_signup_email_confirmation_failed', $message_data, TRUE );
                $mail_options = array(
                    'message'        => $message,
                    'to_address'    => '',
                    'to_name'        => 'Service Team',
                    'from_address'    => '',
                    'from_name'    => 'Server Message',
                    'subject'        => ''
                );
                $mail_result = $this->_send_mail( $mail_options );
                if( ! $mail_result )
                {
                # Uh, oh!  Another email failure!  Save a message in the log file.

                    $this->load->helper( 'file' );

                    $message = "<?php if( ! defined('BASEPATH')) exit('what?');\r\n" . $message;
                    $date = date( 'dMYHis' );
                    write_file( "./logs/$date.php", $message, 'a+' );
                }

                $this->data['menu'] = $this->load->view( 'anoncamp/anoncamp_signup_menu', array(), TRUE );
                $this->load->view( '../template/template_frontpage', $this->data );
            }
            else
            {
            # Great!  Sending the confirmation email to user succeeded.  So, send an email to admin about it.
            # Inform the new user of this great news.

                $message = $this->load->view( 'email/anoncamp_signup_email_confirmation_success', $message_data, TRUE );
                $mail_options = array(
                    'message'        => $message,
                    'to_address'    => '',
                    'to_name'        => 'Service Team',
                    'from_address'    => '',
                    'from_name'    => 'Server Message',
                    'subject'        => ''
                );
                $this->_send_mail( $mail_options );

                $this->data['menu'] = $this->load->view( 'anoncamp/anoncamp_signup_menu', array(), TRUE );
                $this->load->view( '../template/template_frontpage', $this->data );
            }
        }
        else
        {
        # Uh, oh!  Inserting sign-up info into database failed.
        # Alert the user, show a phone number to call, and send email to admin about it.
            $message_data = array(
                'first_name'    => $this->input->post( 'firstname' ),
                'email'        => $this->input->post( 'email' ),
                'href'        => $conf_url
            );
            $message = $this->load->view( 'email/anoncamp_signup_email_confirmation_failed', $message_data, TRUE );
            $mail_options = array(
                // code here
            );
            $this->_send_mail( $mail_options );

            $this->data['menu'] = $this->load->view( 'anoncamp/anoncamp_signup_menu', array(), TRUE );
            $this->load->view( '../template/template_frontpage', $this->data );
        }
    }
    else
    {
    # Uh, oh!  The form data is invalid!
    # So, re-display the form with the info re-populated and validation errors displayed.
        $this->data['menu'] = $this->load->view( 'anoncamp/anoncamp_signup_menu', array(), TRUE );
        $this->load->view( '../template/template_frontpage', $this->data );
    }
}

Thanks in advance


Warm Regards
#2

[eluser]waynhall[/eluser]
Use custom callbacks to abstract out the different validations.
#3

[eluser]ShawnMcCool[/eluser]
The way you're doing things is a common method. There's nothing terribly wrong with it but there ARE a lot of ways to clean it up and make it better to work with.

I'm going to make a few simple recommendations on how you can do that. However, there are many other ways to accomplish your goals and I sometimes do so in ways that are more complicated to express than these recommendations.

For one, store your validations in a config file. http://ellislab.com/codeigniter/user-gui...ngtoconfig
This allows you to use the same validation in multiple places. It comes up sometimes.

In my opinion the sending of email can be initiated from the controller but should be configured in the model. People may disagree with me. But, this is fundamentally a data action. I would create a model method something like 'send_service_team_email(necessary_parameters)' to move that all out of your controller.

Maybe use CodeIgniter's built in write_log method. $this->log->write_log('error', $message); (again this can be in the model method that sends the email)

I have a format_date method. The first parameter is a string. format_date('mysql'); The second parameter is optional. It accepts a string then does strtotime and formats it based on the format string. For example:

If I enter format_date('mysql') it'll give me the current time in MySQL datetime format. If I do format_date('short_date') it'll give me the current date in mm/dd/yyyy format. Or, format_date('mysql', 'last tuesday') would return the date of last tuesday in mysql format. The purpose of this is to standardize date formats across the site. You don't want to accidentally use mm/dd/yyyy format sometimes and m/d/yy format in other places.

Again, these are just a few recommendations based on the code snippet above. I could also easily suggest making a controller extension that automatically defaults layout and allows you to configure your content view and content view data, etc. But, that's probably a bit beyond the scope of this post.
#4

[eluser]xjohnson[/eluser]
Thanks Waynhall and ShawnMcCool.




Theme © iAndrew 2016 - Forum software by © MyBB