0
\$\begingroup\$

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?

\$\endgroup\$

closed as off-topic by 200_success, esote, Toby Speight, Vogel612 Apr 10 at 11:05

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Authorship of code: Since Code Review is a community where programmers improve their skills through peer review, we require that the code be posted by an author or maintainer of the code, that the code be embedded directly, and that the poster know why the code is written the way it is." – 200_success, Toby Speight, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Instead of this $this->Static_model why not make a new config for this data, it seems like a better way. Because you can then create a development copy etc. codeigniter.com/user_guide/general/… \$\endgroup\$ – ArtisticPhoenix Apr 8 at 17:07
  • \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The exerpted code that you have posted is not reviewable in this form because the overall code you want reviewed is still behind a link. Please include all the code that you want reviewed in your question to make it available to reviewers. Thanks! \$\endgroup\$ – Vogel612 Apr 10 at 11:04
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Where is it best to place Data_Controller, in the controller directory or someware else? \$\endgroup\$ – Razvan Zamfir Jun 12 at 7:33
3
\$\begingroup\$

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!

\$\endgroup\$
0
\$\begingroup\$

Personally I recommend you to create a Controller in core folder like MY_Controller And each controller will extend this controller.

Sample here : https://avenir.ro/codeigniter-tutorials/creating-working-with-my_controller-codeigniter/

\$\endgroup\$

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