Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to develop a better function and curious what others would suggest I do to it so that its not so robust and still accomplish what I need.

A couple of things I need to account for is when I have a last 5 messages as well as getting just the regular number of messages.

/**
 * Gets all or last $x number of personal messages of the specified user
 *
 * @param integer $user_id   User ID of the user specified
 * @param integer $limit   Limit of how many messages to retrieve
 * @param integer $timezone   The timezone offset of the user's geo location
 * @param string  $box   Loading either the inbox or outbox of messages
 * @param string  $date_format   The format for which the date needs to be ouputted
 * @param array   $display  Tells what kind of messages to retrieve (important or specific datetime or datetime range)
 * @return object/NULL
 */
public function get_personal_messages($user_id, $limit = NULL, $timezone, $box, $date_format, $display = NULL)
{
    $this->db->select('personal_messages.message_id');
    $this->db->select('personal_messages.subject');
    $this->db->select('personal_messages.datetime_sent');
    $this->db->select('personal_messages.attachments');
    $this->db->select('personal_messages.priority');
    $this->db->select('personal_messages.message_content');
    if ($box == 'inbox')
    {
        $this->db->select('personal_messages.to_read AS message_read');
    }
    else
    {
        $this->db->select('personal_messages.from_read AS message_read');
    }
    if ($box == 'inbox')
    {
        $this->db->select('personal_messages.is_inbox_favorite AS is_favorite');
    }
    else
    {
        $this->db->select('personal_messages.is_outbox_favorite AS is_favorite');
    }
    $this->db->select('CONCAT(users.first_name, " ", users.last_name) AS sender_name', FALSE);
    $this->db->select('users.email_address AS sender_email_address');
    $this->db->select('user_profiles.user_avatar AS sender_avatar');
    $this->db->from('personal_messages');
    $this->db->join('users', 'users.user_id = personal_messages.from_user_id');
    $this->db->join('user_profiles', 'users.user_id = user_profiles.user_id');
    if ($display['type'] == 'today')
    {
        $this->db->where('datetime_sent >', $display['values']['start_time']);
        $this->db->where('datetime_sent <', $display['values']['end_time']);
    }
    elseif ($display['type'] == 'this_week')
    {

    }
    elseif ($display['type'] == 'last_week')
    {

    }
    if ($box == 'inbox')
    {
        $this->db->where('personal_messages.to_user_id', $user_id);
    }
    else
    {
        $this->db->where('personal_messages.from_user_id', $user_id);
    }
    if ($limit != NULL)
    {
        if (is_numeric($limit))
        {
            $this->db->limit($limit);
        }
    }
    $query = $this->db->get();
    $personal_messages = array();
    $personal_messages['messages'] = $query->result();
    if (count($personal_messages['messages']) > 0)
    {
        for ($x = 0; $x < count($personal_messages['messages']); $x++)
        {
            $attachments = $personal_messages['messages'][$x]->attachments;
            if ($this->functions_model->null_check($attachments) === FALSE)
            {
                $attachments = json_decode($attachments, TRUE);
                for ($i = 0; $i < count($attachments); $i++)
                {
                    $file_name = $attachments[$i];
                    $attachments[$i] = array();
                    $attachments[$i]['file_name'] = $file_name;
                    if ($this->functions_model->is_file('assets/downloads/'.$file_name, FALSE) === TRUE)
                    {
                        $attachments[$i]['is_file'] = TRUE; 
                        $file_size =  $this->functions_model->bytes_to_size(filesize('assets/downloads/'.$file_name));
                        $attachments[$i]['file_size'] = $file_size;   
                        $attachments[$i]['file_location'] = 'assets/downloads/'.$file_name;                
                    }
                    else
                    {
                        $attachments[$i]['is_file'] = FALSE;    
                    }
                }
                $personal_messages['messages'][$x]->attachments = $attachments;
            }
            $personal_messages['messages'][$x]->datetime_sent = $this->functions_model->actual_time('Y-m-d g:i:s', $timezone, strtotime($personal_messages['messages'][$x]->datetime_sent));
            $personal_messages['messages'][$x]->datetime_sent = date($date_format, strtotime($personal_messages['messages'][$x]->datetime_sent)); 
            $personal_messages['messages'][$x]->datetime_sent = $this->functions_model->time_since(strtotime($personal_messages['messages'][$x]->datetime_sent));
            $avatar = $this->functions_model->site_url().'assets/themes/'.$this->config->item('default_theme').'/images/avatars/avatar.jpg';
            if ($this->functions_model->null_check($personal_messages['messages'][$x]->sender_avatar) === FALSE)
            {
                if ($this->functions_model->is_file('assets/themes/supr/images/avatars/'.$personal_messages['messages'][$x]->sender_avatar, FALSE) === TRUE)
                {
                    $avatar = $this->functions_model->site_url().'assets/themes/'.$this->config->item('default_theme').'/images/avatars/'.$personal_messages['messages'][$x]->sender_avatar;
                }
            }
            $personal_messages['messages'][$x]->sender_avatar = $avatar; 
        }
        $personal_messages['total_unread_messages'] = $this->get_users_unread_messages($user_id);
    }
    return $personal_messages;
}
share|improve this question
    
Any ideas for me? –  Jeff Davidson Nov 3 '12 at 16:14
add comment

1 Answer

You have a lot of if ($box == 'inbox') statements. Why not bunch them together into one if statement?

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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