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 currently writing Report's system. I have about 20 different users with different access. Am I doing it correctly? Can I improve my style of writing this code? All my reports based on the same structure.

In my Report's Controller:

function my_report_name() 
    {
        //current week by default
        $dates['from'] = $this->uri->segment(3,date('Y-m-d',strtotime(date('o-\\WW'))-60*60*24*7)); 
        $dates['to'] = $this->uri->segment(4,date("Y-m-d", strtotime($dates['from']) + 6 * 24* 60 * 60));

        $data['report_name']=$this->Reports_model->getMyReportDescriptionSummary($dates);

        $data['title'] = "My Report Name Description";
        //view in 'view/report' folder to use
        //name of view always the same as name of function
        $data['main'] = "report/my_report_name";
        $data['menuReport'] = "class='current'";
        $data['panel'] = "panel/panel";

        $this->load->vars($data);
        //load template
        $this->load->view('template/t_main_full', $data);

    }

In my Model:

function getMyReportDescriptionSummary($dates)
    {
        $this->db->select("some_data,
                           some_data_2,
                           some_data_3"
                          );
        $this->db->join("table1");
        $this->db->join("table2");
        $this->db->where("some_params");
        $Q = $this->db->get();

        if($Q->num_rows() > 0)
        {
            return $Q->result_array();
        }
        else
        {
            return false;
        }
    }

In my repots layout page:

//function to check permissions
<? if($this->Model_Users->accessVerify('my_report_name')) {?>
<tr>My report</tr>
<?}?>
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

From a CodeIgniter point of view, this is fine but CI isn't really MVC. in MVC proper, the controller would be doing very little (See: http://stackoverflow.com/questions/13813046/how-is-mvc-supposed-to-work-in-codeigniter ). However in CodeIgniter's implementation you still want to strive for "Fat model, thin controller", which you're doing for the most part. This will give you far greater reusability as the logic isn't tied to a specific controller. I'd argue that most of the things in the $data array are actually stateful or display logic (e.g. which panel to display) so don't belong in the controller.

MVC strives for a high separation of concerns with reusable components and although CodeIgniter goes against this philosophy slightly, you can still work around it. Keep your views and models reusable.

share|improve this answer
    
would you like to take a look at codereview.stackexchange.com/questions/41959/… –  DanielTheRocketMan Feb 18 at 16:14
add comment

The code is fine, but why are you passing from date and to date to model ? Its not being used at model. May be you are trying to add a condition to filter the data set under specified dates. Anyway, your starting is good. Good luck.

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.