Ggthjy

I am working on a basic blog application in Codeigniter 3.1.8 and Bootstrap 4.

Several entities are present in all controllers (except Login.php and Register.php): static data, categories and pages.

$data = $this->Static_model->get_static_data();

$data['pages'] = $this->Pages_model->get_pages();

$data['categories'] = $this->Categories_model->get_categories();

Further more, in most controller, the code above appears more then one time.

I am afraid this is mot the only case of repetitive code in the application. (See the entire application, at its current state, on my Github account).

I am looking for specific and/or general advice from experienced PHP developers that would help me reduce code redundancy and make it more efficient.

What is the best way to avoid the repeating of the code above in my controllers?

I had a very short look at lightblog/application/controllers/Pages.php and I noticed that these two values were not used in the page() method:

$data['pages']

$data['posts']

Then I realized that you hand over data to other objects and they might be using these two values. Who knows? This makes your code very hard to understand and to debug.

Basically you're treating the $data array as a sort of object you hand over to other classes.

But wait, did I say 'object'? But what if it was? Clearly you have a need to keep data accessible, but why use an array that you need to recreate every time? Why not use a class?

Now I don't have any idea how your code is structure, and which class would be the best to use for this. But it would make sense to extend the basic CodeIgniter controller class to hold this data, like so:

class Data_Controller extends CI_Controller 

{

    private $data;



    public function __construct()

    {

        parent::__construct();

        $this->data = $this->Static_model->get_static_data();

        $this->data['pages'] = $this->Pages_model->get_pages();

        $this->data['categories'] = $this->Categories_model->get_categories();

        $this->data['posts'] = $this->Posts_model->sidebar_posts($limit=5, $offset=0);

        $this->data['page'] = $this->Pages_model->get_page($page_id);

    }





    public function getData()

    {

        return $this->data;

    }



}

or something to the same effect and then extend this for your pages class:

class Pages extends Data_Controller {



    public function __construct()

    {

        // currently this whole method doesn't do anything, it can be removed

        parent::__construct();

    }



    public function page($page_id) {

        $data = $this->getData();

        ..........................



    }



}

And, of course, those controller classes that don't need all that data can just extend CI_Controller.

I'm sure there are other ways of doing this. I don't like the fact that now you still collect all that information without actually knowning if it is needed. I would make the retrieval of the data dependent on the fact that it is actually used. In other words use getter methods, not a data array. I'll give an example for 'pages' and 'posts', but you have to create the other ones yourself:

class Data_Controller extends CI_Controller 

{



    public function getPages()

    {

        return $this->Pages_model->get_pages();

    }



    public function getCatagories()

    {

        return $this->Categories_model->get_categories();



    }



}

If you need to buffer the data you can do:

class Data_Controller extends CI_Controller 

{



    public function getCatagories()

    {

        static $buffer = NULL;

        if (is_null($buffer)) $buffer = $this->Categories_model->get_categories();

        return $buffer;

    }



}

Keep in mind that this buffer will work for all instances of Data_Controller. That can be an advantage, or a disadvantage.

OK, that was a bit long. I hope you got some new ideas from my ramblings.

In this case I would probably convert this $this->Static_model into a Config file that I can use through CI (that is what they are there for). You can even make CI load this file automatically, with hook. And you can even load different version based on your ENVIORMENT setting (for testing and what not).

For the dynamic data, now depending how dynamic this is:

 $data['pages'] = $this->Pages_model->get_pages();

I would probably use the Caching Driver. Chances are this doesn't update that much, and even when it does you can reset the cache. For example how often will you really add a new category. Certainly not every couple minutes, or even hours.

One other thing not mentioned

Avoid changing data types when returning from a method. You may not even realize you are doing it. In some cases its perfectly fine to return mixed data types, but as a general rule you should avoid it as it complicates your downstream code by having to check the return data all the time for it's type.

I found this quick example (in your code):

class Categories_model extends CI_Model {



    public function get_category($category_id){

        $query = $this->db->get_where('categories', array('id' => $category_id));

        if ($query->num_rows() > 0) {

            return $query->row();

        }



    } //<-- returns null || array



}



class Pages extends CI_Controller {

    public function page($page_id) {



        //...

        $data['categories'] = $this->Categories_model->get_categories(); //returns an array || null

        //...

        //requires a check (null is false, so is [])

        if ($data['categories']) {

            foreach ($data['categories'] as &$category) {

                $category->posts_count = $this->Posts_model->count_posts_in_category($category->id);

            }

        }

        //...

    }

}

Instead consider this:

class Categories_model extends CI_Model {



    public function get_category($category_id){

        $query = $this->db->get_where('categories', array('id' => $category_id));

        if ($query->num_rows() > 0) {

            return $query->row();

        }

        return [];

    } 



}



class Pages extends CI_Controller {

    public function page($page_id) {

        //...

        $data['categories'] = $this->Categories_model->get_categories(); //always returns an array

        //...

        //requires no type check, as an empty array simply skips the loop

        foreach ($data['categories'] as &$category) {

            $category->posts_count = $this->Posts_model->count_posts_in_category($category->id);

        }

        //...

    }

}

This may seem trivial, but it can add up to a lot of code. In PHP7 you can even set return type hints eg. public function get_category($category_id) : array. To insure the return type is consistent etc.

Really when it comes to programing one of the most important things is consistency.

Hope it helps!