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 am utilizing CodeIgniter's Pagination Class to implement pagination in the project.
In this project, there are 3 modules: Event,Business & Parts.
Each Module has same "view" for pagination. I have written pagination code for the event module, which has 3 types of pagination depending on the condition (received from end user).
Pagination ScreenShot: http://i.stack.imgur.com/twM3e.png

Following is the code for the event module. The controller:

class Event extends CI_Controller{
      public function event_list($sort_by = 'Description', $sort_order = 'asc', $limit =         3,$offset = 0,$search = false,$category = false){
    $config = array();
        if($category){

                $third_term = $this->uri->segment(3);
                if(is_numeric($sort_by)){
                    if(is_numeric($third_term)){
                        $sort_by = 'Description';
                    }else{
                        $sort_by = 'Price';
                    }
                }
                 $config['base_url'] = site_url("event/categories/$sort_by/$sort_order/$limit");

        }

        elseif($search || $_POST){
            if($_POST){
                $search_data = array(
                    'search_term' => '',
                    'search_category' => '',
                );
                $this->session->unset_userdata($search_data);
                $search_term = $this->input->post('search',TRUE);
                $search_category = $this->input->post('select-search',TRUE);
                $this->session->set_flashdata('search_term',$search_term);
                $this->session->set_flashdata('search_category',$this->input->post('select-search',TRUE));
                $config['base_url'] = site_url("event/search_list/$sort_by/$sort_order/$limit");
            }
            else{
                $this->session->keep_flashdata('search_term');
                $this->session->keep_flashdata('search_category');
                $search_term = $this->session->flashdata('search_term');
                $search_category = $this->session->flashdata('search_category');

                $config['base_url'] = site_url("event/search_list/$sort_by/$sort_order/$limit");


            }
        }else{
            $config['base_url'] = site_url("event/event_list/$sort_by/$sort_order/$limit");
        }

        $data['fields'] = array(
            'Description' => 'Description',
            'Price' => 'Price'

        );

        $config['per_page'] = $limit;

        $this->load->model('event_model');
        $results = $this->event_model->search($limit, $offset, $sort_by, $sort_order,$search,$category);
        $data['num_results'] = $results['num_rows'];
        $data['events'] = $results['rows'];
        $this->load->library('pagination');
        $config['total_rows'] = $data['num_results'];
        $config['uri_segment'] = 6;
        $this->config->load('pagination');          
        //$this->pagination->initialize($config);
        $data['pagination'] = $this->pagination->create_links();
        $data['sort_by'] = $sort_by;
        $data['sort_order'] = $sort_order;
        $data['page_no'] = ceil($this->uri->segment(6)/ $limit) + 1;
        $total_pages = ceil($config['total_rows'] / $limit);
        if($total_pages > 1){
            $data['total_pages'] = $total_pages;
        }
        $config['use_page_numbers'] = TRUE;

        $data['limit'] = $limit;//echo '<pre/>'.print_r($config);exit();
        $this->load->view('templates/event/event-list',$data);      
    }

       public function search_list($sort_by = 'Description', $sort_order = 'asc', $limit = 3,$offset = 0,$search = true,$category = false){
         $this->event_list($sort_by,$sort_order,$limit,$offset,$search);
    }

       public function categories($sort_by = 'Description', $sort_order = 'asc', $limit = 3,$offset = 0,$search = false,$category = true){

        $search_category = $this->uri->segment(3);

        $in_flash =  $this->session->flashdata('search_category');
        if(($in_flash == $search_category) && (!is_numeric($search_category))){
            $this->session->keep_flashdata('search_category');
        }else{
            $search_data = array(
                    'search_term' => '',
                    'search_category' => '',
            );
            $this->session->unset_userdata($search_data);
            $this->session->set_flashdata('search_category',$search_category);
        }

        $this->event_list($sort_by,$sort_order,$limit,$offset,$search=false,$category = true);
    }
}

The Model:

class event_model extends CI_Model{


function search($limit, $offset, $sort_by, $sort_order,$search,$category) {

    $sort_order = ($sort_order == 'desc') ? 'desc' : 'asc';
    $sort_columns = array('Description', 'Price');
    $sort_by = (in_array($sort_by, $sort_columns)) ? $sort_by : 'Description';

    if($search){

        $search_term = "'%".$this->input->post('search',TRUE)."%'";
        $search_category = $this->input->post('select-search',TRUE);

        $query =  sprintf('SELECT *,a.id AS eid,(SELECT Url from oc_storage where Object_Id = a.id and Object = "event" and type= "thumb.normal" limit 0,1 ) as thumb FROM oc_event a,oc_event_type b,oc_address c WHERE  b.id = %s AND a.Event_Type = b.id AND c.Object_type = "event" AND c.Object_Id = a.id AND a.Description like "%s" ORDER BY a.%s %s LIMIT %s ,%s',$search_category,$search_term,$sort_by, $sort_order,$offset,$limit);

        $count = "Select Count(*)FROM oc_event a,oc_event_type b,oc_address c WHERE  a.Event_Type = b.id AND c.Object_type = 'event' AND c.Object_Id = a.id AND a.Description like $search_term";

    }

    if($category){

         $search_category = $this->session->flashdata('search_category');       

        $query =  sprintf('SELECT *,a.id AS eid,(SELECT Url from oc_storage where Object_Id = a.id and Object = "event" and type= "thumb.normal" limit 0,1 ) as thumb FROM oc_event a,oc_event_type b,oc_address c WHERE  b.id = %s AND c.Object_type = "event" AND c.Object_Id = a.id ORDER BY a.%s %s LIMIT %s ,%s',$search_category,$sort_by, $sort_order,$offset,$limit);

         $count = "Select Count(*)FROM oc_event a,oc_event_type b,oc_address c WHERE  a.Event_Type = $search_category AND c.Object_type = 'event' AND c.Object_Id = a.id";
    }
    else{

        $query = sprintf('SELECT *,a.id AS eid,(SELECT Url from oc_storage where Object_Id = a.id and Object = "event" and type= "thumb.normal" limit 0,1 ) as thumb FROM oc_event a,oc_event_type b,oc_address c WHERE  a.Event_Type = b.id AND c.Object_type = "event" AND c.Object_Id = a.id ORDER BY a.%s %s LIMIT %s ,%s',$sort_by, $sort_order,$offset,$limit);

        $count = "Select Count(*)FROM oc_event a,oc_event_type b,oc_address c WHERE  a.Event_Type = b.id AND c.Object_type = 'event' AND c.Object_Id = a.id ";
    }

    $stmt = $this->db->conn_id->prepare($query); 
    $stmt->execute();                   
    $ret['rows']  = $stmt->fetchAll( PDO::FETCH_ASSOC ) ;

    $ret['num_rows'] = $this->db->conn_id->query($count)->fetchColumn();

    return $ret;

}

The view : "Including only whats required";

   <div class="sorting">
            <?php 
            foreach($fields as $field_name => $field_display): ?>
            <?php if (($sort_by == $field_name)){
                (($sort_order == 'asc' && $sort_by == $field_name) ? 'desc' : 'asc')
            ?>
            <a href="<?php 
            $call_by = $this->uri->segment(2);
            echo site_url("event/$$call_by/Description/".(($sort_order == 'asc' && $sort_by == $field_name) ? 'desc' : 'asc')."/$limit");
            ?>">Desc <?php 
                echo ($sort_order == 'asc' && $sort_by == "Description") ? '&#9650;' : '&#9660;'
            ?></a><a href="<?php 
                echo site_url("event/$$call_by/Price/".(($sort_order == 'asc' && $sort_by == $field_name) ? 'desc' : 'asc')."/$limit");
            ?>">Price  <?php 
            echo ($sort_order == 'asc' && $sort_by == "Price") ? '&#9650;' : '&#9660;'?></a>
            <?php } ?>
            <?php endforeach; ?>
        </div>

I am quite sure that I haven't followed all laws of programming.
What I want to know is, how can I use the above code for all 3 modules, and where I can improve these functions, and how can I make this code more Object Oriented and Less Scripted. Any suggestions/modification please.
Thx

share|improve this question
add comment

1 Answer 1

When you have long argument lists, you might consider compacting the arguments into an array. However, sometimes its not an issue of the size of the argument list, its an issue of the size of the method. In this case it is the latter. Your event_list() method is doing entirely too much, which is why your argument list is also so long. If we follow the Single Responsibility Principle, then a function/method should only be responsible for one thing. This method is responsible for at least four different tasks. I would seriously consider breaking this method up into smaller methods. That's the biggest issue I see.

Your variables should be more descriptive, especially when using magic numbers to access unknown values. What is $third_term? Where did "3" come from and why is it important?

Why would $sort_by be numeric? It doesn't appear that you ever use it as a numeric value. Instead of trying to manipulate the sort method based on vague inputs, I would create a whitelist of acceptable sort terms and if the one provided does not match one in the whitelist I would default to one of the accepted ones. For instance:

$whitelist = array(
    'Description',
    'Price',
    //etc...
);

if( ! in_array( $sort_by, $whitelist ) ) {
    $sort_by = array_shift( $whitelist );
}

Along with Single Responsibility, you have "Don't Repeat Yourself" (DRY). As the name implies, your code should not repeat itself. Your "base_url" is very similar throughout this method and seems to be set no matter what. So instead of typing that again and again, I would create a variable to contain the difference and apply it at the end. Another change I would consider making is to use an implode() to accomplish the string. This way you can more easily manipulate it if necessary.

if( $category ) {
    //etc...
    $diff = 'categories';
}
//etc...

$sections = array(
    'event',
    $diff,
    $sort_by,
    $sort_order,
    $limit
);
$site_url = implode( '/', $sections );
$config[ 'base_url' ] = site_url( $site_url );

Your elseif statement is redundant. The inner if/else structure mirrors the outer, so you can just shift those inner statements up a level and reverse the order to ensure that the post is processed before the search. This removes a level indentation and saves processing.

elseif( $_POST ){
    //etc...
} elseif( $search ) {
    //etc...
} else {

Where did $data come from? You just started adding to it without declaring it first. Always declare your arrays before trying to manipulate them, else you could end up accidentally modifying something you didn't mean to or accessing a malicious global. Besides, not declaring issues warnings to your browser. You should have error reporting on high enough to catch these. Also why are you setting all of those values individually? Doing it that way adds a lot of clutter. I would either wait until everything was ready to add into the array all at once, find some way of looping the elements or combining/merging arrays, or compacting variables. The first is probably the method I'd lean to in this situation. The second would be ideal, but because the keys change between arrays it would make this impossible. The third is good if you have already created variables and used them elsewhere. Otherwise you end up creating tons of unused variables just to create an array.

//manually
$data = array(
    'num_results' => $results[ 'num_rows' ],
    'events'      => $results[ 'rows' ],
    //etc...
);

//compacting
$num_results = $results[ 'num_rows' ];
$events      = $results[ 'rows' ];
//etc...
compact(
    'num_results',
    'events',
    //etc...
);

Another principle being violated here would be the Separation of Concerns (SoC). I'm not really sure I can adequately explain this one, so I'll just suggest you look into it and see how it applies to things like POST, flashdata, and the inner workings of MVC. Its one of those things I'm aware of and can notice and avoid in my own work, but have issues trying to explain, sorry.

What is the point of search_list()? Its just a slightly reduced wrapper for an already public method. I would remove it and just use event_list.

The printf functions should only be used when applying different variables to the same template (I'll demonstrate this shortly), not when just applying variables to a string. You can do the same thing with concatenation, or better yet, by using double quotes you can have PHP automatically escape variables and entities. Also, MySQL is just as lenient as PHP when it comes to whitespace. I would seriously contemplate making your SQL statements more legible. I would show you an example here, but I'm not that familiar with SQL to accurately format it.

I was getting a headache trying to decipher your HTML so I just edited it. Unless I miss my guess the $call_by shouldn't change on any iteration so it should be defined outside of the loop. This still feels a little too PHP-bulky for a view, but I don't know if anything can be done about that.

<?php $call_by = $this->uri->segment( 2 ); ?>

<div class="sorting">
    <?php foreach($fields as $field_name => $field_display): ?>

        <?php if (($sort_by == $field_name)) : ?>

            <?php
            $sort = $sort_order == 'asc' && $sort_by == $field_name ? 'desc' : 'asc';

            $temp = "event/$$call_by/%s/$sort/$limit";
            $desc_url = site_url( sprintf( $temp, "Description" ) );
            $price_url = site_url( sprintf( $temp, "Price" ) );

            $desc_ent = $sort_order == 'asc' && $sort_by == 'Description' ? '&#9650;' : '&#9660;';
            $price_ent = $sort_order == 'asc' && $sort_by == "Price" ? '&#9650;' : '&#9660;';
            ?>

    <a href="<?php echo $desc_url; ?>">Desc <?php echo $desc_entity; ?></a>
    <a href="<?php echo $price_url; ?>">Price  <?php echo $price_ent; ?></a>

        <?php endif; ?>

    <?php endforeach; ?>
</div>

Hope this helps

share|improve this answer
    
Thank you very much for taking time to review and pointing out all the errors.I will definitely apply all the suggested programming techniques.(P.S: I cannot vote up due to lack of reputation) –  Christopher Dec 27 '12 at 16:20
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.