Helping to Make My Controller Function DRY

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(

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.

StackExchange Code Review Q#30391, answer score: 2