3
\$\begingroup\$

I have the following controller function. What I want to do is make this function do less. Most of this code is just random checks on various things that is also done on most of my other controllers. So to prevent from consistently copying and pasting this code THAT IS NOT where the access_level_id is checked in the if statement to be greater than or equal to four. That needs to stay.

/**
 * Content_pages::read()
 * 
 * @return
 */
public function read()
{
    // Initializes the user id with the value from the session user id if it exists. If there isn't a value present then the value is set to 0.
    $user_id = $this->session->userdata('user_id');

    // Place to dump the user id to verify it is the expected value.
    // vardump($user_id);

    // Verifies that the user id is an accepted formed user id.
    if (($user_id !== FALSE) && (is_numeric($user_id)) && (strlen($user_id) >= 5))
    {   
        // User data is an object that gathers all of the user data from the users table along with the email address from the login table with use of the user id variable.
        $user_data =  $user = $this->user->with_login($user_id);

        // Place to dump the user data object to verify it is the expected properties and values for the user data object.
        // vardump($user_data);

        // Checks to verify that there is data inside of the user data object and that it is not empty.
        if (!empty($user_data))
        {
            // Set a default for the user avatar.
            $default_avatar = 'default.jpg';

            // Define where the avatars are located on the server.
            $avatar_directory = 'assets/globals/images/avatars/';

            // Check to see if the user's avatar is null.
            if (!is_null($user_data->avatar))
            {
                $avatar = $avatar_directory . $user_data->avatar;

                // Find out if the user's defined avatar exists on the server avatars directory.
                if (file_exists(FCPATH . $avatar))
                {
                    $user_data->avatar = base_url() . $avatar_directory . $user_data->avatar;
                }
                else
                {
                    $user_data->avatar = base_url() . $avatar_directory . $default_avatar;
                }
            }
            else
            {
                $user_data->avatar = $default_avatar;
            }

            // Checks to see if the user has a role id of four and if they do then it shows the admin dashboard and if not then shows the user dashboard.
            if ($user_data->access_level_id >= 4)
            {
                // Retrieve all the users from the database that handle characters and assign it to the users variable.
                $themes = $this->theme->get_all();

                // Place to dump the users array to verify it is the expected value.
                // vardump($news_categories);

                // Checks to verify that there is data inside of the users array and that it is not empty.
                if (!empty($themes))
                {
                    $this->template->set('themes', $themes);
                }

                // Add the breadcrumbs to the view.
                $this->breadcrumb->add_crumb('<li><a href="' . base_url() . 'wrestling-manager/control-panel" class="glyphicons home"><i></i> Control Panel</a></li>');
                $this->breadcrumb->add_crumb('<li><i></i> Themes</li>');
                $this->breadcrumb->change_link('<li class="divider"></li>');

                // Sets all the properites for the template view.
                $this->template
                    ->set_theme('smashing')
                    ->set_layout('control_panel_view')
                    ->set_partial('header', 'partials/header')
                    ->set_partial('sidebar','partials/sidebar')
                    ->set_partial('footer', 'partials/footer')
                    ->title('Themes')
                    ->set('user_data', $user_data)
                    ->build('themes_view');    
            }
            else
            {
                // Redirect to custom error page
                // TODO: Make custom error page.
            }

        }
        else
        {
            // Getting redirected here means the user data object is empty.
            redirect('wrestling-manager/login');
        }

    }
    else
    {
        // Getting redirected here means the user id was not an acceptable value.
        redirect('wrestling-manager/login');
    }
}

EDIT : This was done yesterday before I received both answers. Could both people update their answers if possible.

Control Panel Controller

<?php

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

class Control_panel extends MY_Controller
{

    /**
     * Control_panel::__construct()
     * 
     * Load the parent construct and any additional models, helper, libraries available. 
     * 
     * @return void
     */
    public function __construct()
    {
        parent::__construct();
    }

    /**
     * Control_panel::index()
     * 
     * If user passes then we load the dashboard with the defined template and add the neccessary breadcrumbs and enable the profiler. 
     * 
     * @return void
     */
    public function index()
    {   
        $this->template
            ->title('Wrestling Manager Control Panel');

        if ($this->user_data->access_level_id >= 4)
        {
            $this->template
                ->build('admin_dashboard_view'); 
        }
        else
        {
            $this->template
                ->build('user_dashboard_view'); 
        }
    }

    /**
     * Control_panel::logout()
     * 
     * Logs out the current user from their session. 
     * Then we redirect the user to the login page.
     * 
     * @return void
     */
    public function logout()
    {
        $this->session->sess_destroy();
        redirect('wrestling-manager/login');
    }

    public function show_dashboard()
    {

    }
}

MY_Controller

<?php

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

class MY_Controller extends CI_Controller 
{   
    public $user_data;

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

        $this->load->helper('flash_helper');
        $this->load->model('user_model', 'user');
        $this->output->enable_profiler(TRUE);

        $this->_check_auth();  

        $this->breadcrumb->add_crumb('<li><a href="' . base_url() . 'wrestling-manager/control-panel" class="glyphicons home"><i></i> Control Panel</a></li>');
        $this->breadcrumb->change_link('<li class="divider"></li>');

        $this->template
            ->set_theme('smashing')
            ->set_layout('control_panel_view')
            ->set_partial('header', 'partials/header')
            ->set_partial('sidebar','partials/sidebar')
            ->set_partial('footer', 'partials/footer')
            ->set('user_data', $this->user_data);
    }

    private function _check_auth()
    {
        $user_id = $this->session->userdata('user_id');

        // vardump($user_id);

        if (($user_id !== FALSE) && (is_numeric($user_id)) && (strlen($user_id) >= 5))
        {
            $this->user_data = $this->user->with_login($user_id);

            // vardump($user_data);

            if (!empty($this->user_data))
            {
                $default_avatar = 'default.jpg';

                $avatar_directory = 'assets/globals/images/avatars/';

                if (!is_null($this->user_data->avatar))
                {
                    $this->avatar;
                    $avatar = $avatar_directory . $this->user_data->avatar;

                    if (file_exists(FCPATH . $avatar))
                    {
                         $this->user_data->avatar = base_url() . $avatar_directory . $this->user_data->avatar;
                    }
                    else
                    {
                         $this->user_data->avatar = base_url() . $avatar_directory . $default_avatar;
                    }
                }
                else
                {
                    $this->user_data->avatar = $default_avatar;
                }
            }
            else
            {
                redirect('wrestling-manager/login');
            }
        }
        else
        {
            redirect('wrestling-manager/login');
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

DRY

The idea behind DRY is fairly deep:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

Avatar

Developers often miss the idea that assigning a value to an instance variable in multiple places violates the DRY principle. Reading the code top-to-bottom reveals:

$user_data->avatar = base_url() . $avatar_directory . $user_data->avatar;
$user_data->avatar = base_url() . $avatar_directory . $user_data->avatar;
$user_data->avatar = base_url() . $avatar_directory . $default_avatar;
$user_data->avatar = $default_avatar;

The knowledge of how the avatar is assigned occurs in multiple places throughout the code, thus breaking the DRY principle. The culprit begins with these lines:

$user_data =  $user = $this->user->with_login($user_id);
if (!is_null($user_data->avatar)){

Surely we can rewrite this:

$user = $this->user->with_login($user_id);

if( !$user->has_avatar() ) {
}

Now there is no handle to the user's data. Perfect. This allows how the data is represented to change without affecting the calling code (in this case, the code you've shown). There's something subtle happening here. What does it imply when you can check to see if a user has a valid avatar?

It implies that you should also be able to set the user's avatar! Consider:

$default_avatar = 'default.jpg';
...

# Define where the avatars are located on the server.
$avatar_directory = 'assets/globals/images/avatars/';

# Avatar based on user_data
$avatar = $avatar_directory . $user_data->avatar;

# Find out if the user's defined avatar exists on the server avatars directory.
if(file_exists(FCPATH . $avatar)){
  $user_data->avatar = base_url() . $avatar_directory . $user_data->avatar;
} else {
  $user_data->avatar = base_url() . $avatar_directory . $default_avatar;
}

The above code is not really the responsibility of the controller. It is the responsibility of the "business object", in this case the user. All the code above is actually:

$user->set_default_avatar();

Within that method, each line that looks like this:

$user_data->avatar = ...

Would be rewritten as:

$user_data->set_avatar( ... );

This could then be rewritten using an Avatar class. The Avatar class would know about:

  • Default avatars
  • Location of avatars on the server

Consider this tempting code slipped into the controller:

$avatar = new Avatar();  
$user_data->set_avatar( $avatar );

You wouldn't need the controller to set the avatar, until it changes. The $user_data would have a reference to a default Avatar instance. You could go further with an AvatarFactory that hides knowledge of how the Avatar instance is itself created (does it come from a local JPG image, a web server, or an uploaded file stream -- all should be irrelevant to the controller).

Once the Avatar class exists, at that point all knowledge about Avatars would have a single authoritative representation in the system.

Roles and Source Comments

The following line is also missing the point of DRY:

$user_data->access_level_id >= 4

What does 4 mean? What other places in the system will be comparing the "access_level_id" to magic numbers?

The code should read more like a sentence:

if( $user->in_role( $ROLE_ADMIN ) ) {
  ...

Source comments should not parrot what the code is doing, but focus on why the code is there:

// Checks to see if the user has a role id of four

That comment was apparent from the code. The comment need only state the latter part:

// Show the dashboard based on the user's role.

That should give you a clue that the controller, again, is probably doing too much. Once you move the logic for determining the user's role back into the user, you can do magic like:

$user->show_dashboard();

The controller's responsibility is to cause the dashboard to appear. What type of dashboard is shown is not really its concern. The show_dashboard method then might do something like:

if( $user->in_role( $ROLE_ADMIN ) ) {
  ...
  $this->show_admin_dashboard();
}
else {
  $this->show_user_dashboard();
}

The in_role method might look like:

function in_role( $role ) {
  if( $role == $ROLE_ADMIN ) {
    return $this->get_role() >= 4;
  }
}

Except you'd want to not hard-code the value of 4.

Or show_dashboard could resemble:

// Gets the dashboard based on the user's role.
$dashboard = $this->get_dashboard();
$dashboard->show();

Or, to avoid the comment:

$dashboard = $this->get_dashboard( $this->get_role() );
$dashboard->show();

Either way, the underlying implementation can vary without affecting the controller, and that's what is important.

\$\endgroup\$
  • \$\begingroup\$ That is all really awesome. Hopefully I can work on that and try and get something better formed for my controller. Is there anything non avatar related you would change? \$\endgroup\$ – user2576961 Aug 30 '13 at 17:20
  • \$\begingroup\$ If I worked on it and posted back in an hour would you scan over it again and tell me how the edits look. Only issue is I had placed some of that inside of a MY_Controller yesterday before I was able to update this post. \$\endgroup\$ – user2576961 Aug 30 '13 at 18:24
  • \$\begingroup\$ I created an Avatar Library on my github account. Thoughts? github.com/jeffreydavidson/Avatar \$\endgroup\$ – user2576961 Aug 30 '13 at 18:58
  • \$\begingroup\$ Post another question, rather than adding comments here. You'll get more eyes on it. \$\endgroup\$ – Dave Jarvis Aug 30 '13 at 19:02
  • \$\begingroup\$ Well I want to update my question here with what it pertains still to. \$\endgroup\$ – user2576961 Aug 30 '13 at 19:04
0
\$\begingroup\$

Here it's a bit more DRY, perhaps someone else can take a crack at trying to get it DRYer:

<?php
/**
 * Content_pages::read()
 * 
 * @return
 */
public function read(){
    # Initializes the user id with the value from the session user id if it exists. If there isn't a value present then the value is set to 0.
    $user_id = $this->session->userdata('user_id');


    # User data is an object that gathers all of the user data from the users table along with 
    # the email address from the login table with use of the user id variable.
    $user_data =  $user = $this->user->with_login($user_id);

    # Verifies that the user id is an accepted formed user id.
    if (($user_id !== FALSE && is_numeric($user_id) && strlen($user_id) >= 5) || (empty($user_data))){
        # Set a default for the user avatar.
        $default_avatar = 'default.jpg';

        # Check to see if the user's avatar is null.
        if (!is_null($user_data->avatar)){
            # Define where the avatars are located on the server.
            $avatar_directory = 'assets/globals/images/avatars/';

            # Avatar based on user_data
            $avatar = $avatar_directory . $user_data->avatar;

            # Find out if the user's defined avatar exists on the server avatars directory.
            if (file_exists(FCPATH . $avatar)){
                $user_data->avatar = base_url() . $avatar_directory . $user_data->avatar;
            } else {
                $user_data->avatar = base_url() . $avatar_directory . $default_avatar;
            }
        } else {
            $user_data->avatar = $default_avatar;
        }


        # Checks to see if the user has a role id of four and if they do then it shows the admin dashboard and if not then shows the user dashboard.
        if ($user_data->access_level_id >= 4){
            // Retrieve all the users from the database that handle characters and assign it to the users variable.
            $themes = $this->theme->get_all();

            # Checks to verify that there is data inside of the users array and that it is not empty.
            if (!empty($themes)){
                $this->template->set('themes', $themes);
            }

            # Add the breadcrumbs to the view.
            $this->breadcrumb->add_crumb('<li><a href="' . base_url() . 'wrestling-manager/control-panel" class="glyphicons home"><i></i> Control Panel</a></li>');
            $this->breadcrumb->add_crumb('<li><i></i> Themes</li>');
            $this->breadcrumb->change_link('<li class="divider"></li>');

            # Sets all the properites for the template view.
            $this->template
                ->set_theme('smashing')
                ->set_layout('control_panel_view')
                ->set_partial('header', 'partials/header')
                ->set_partial('sidebar','partials/sidebar')
                ->set_partial('footer', 'partials/footer')
                ->title('Themes')
                ->set('user_data', $user_data)
                ->build('themes_view');    
        } else {
            # Redirect to user dashboard (used to say custom error page here, but above in if statement it says user dash)
            redirect('user/dashboard');
        }
    } else {
        # Redirect since user_id was not an acceptable value OR the user data object is empty.
        redirect('wrestling-manager/login');
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.