Extending the Controller to avoid code duplication

[eluser]Barber of Padua[/eluser]
To avoid repeating code I want the site navigation defined just once, not in all methods of a controller. Is this solution ok, or ... suggestions? (newbie)

first in application/libraries the Controller gets extended and gets a property $navigation
Model gets autoloaded

Code:

class MY_Controller extends Controller {
    
    var $navigation;
    
    function MY_Controller(){
        parent::Controller();
        $this->getMainNav('globalmenu');
    }
    
    function getMainNav($menu){
        $this->navigation = $this->Pages_model->getNav($menu);
    }
}

then in the 'regular' controller

Code:

class Page extends MY_Controller {

    function Page()
    {
        parent::MY_Controller();
    }
    
    function index()
    {
        $data = array();
        
        $data['main_nav'] = $this->navigation;
        $data['content']  = $this->Pages_model->getPageData('home');
        $this->load->vars($data);
        $this->load->view('main_layout');    
    }

    function contact()
    {
        $data = array();
        
        $data['main_nav'] = $this->navigation;
        $data['content']  = $this->Pages_model->getPageData('contact');
        //etc
    }
}

Still got some code repeated, though

[eluser]TheFuzzy0ne[/eluser]
DRY is not always the best route to take. Personally, I choose simplicity and clarity. But at the end of the day, it's up to you to do whatever you feel is easiest for you, and anyone else who may need to work on your application. DRY is used to prevent duplicate data, and to prevent breakages when something changes. I don't see anything in your code that would require any serious changes if another component was modified. In my humble opinion, I think you might be taking DRY a bit too literally.

[eluser]Colin Williams[/eluser]
When DRY is overused or misused, you end up with inflexible code. If it is the case that the application uses this navigation element for nearly all requests (which it likely does) then it makes perfect sense to me to implement it in the way you have. The key is to make sure it's done in such a way that any of the controllers down the line can alter or disable the functionality.

You could even abstract the $data['main_nav'] part by using $this->load->vars() from MY_Controller. Then that content is just available and ready in all your views. The other controllers don't have to worry about it, but they can override it if needed.

[eluser]Barber of Padua[/eluser]
Nice you think the code is ok, FuzzyOne :-)
Since this must be a really common problem, I am wondering how others solved it, and whether this solution is ok in CI.

[eluser]Barber of Padua[/eluser]
Thanks Colin, great answer. I'm going to use your suggestion!

[eluser]Colin Williams[/eluser]
EllisLab designed CI in a way that all core classes can be overloaded or replaced without touching the core classes. It is by design, so the solution is 110% okay.

[eluser]TheFuzzy0ne[/eluser]
[quote author="Barber of Padua" date="1234679018"]Nice you think the code is ok, FuzzyOne :-) [/quote]

I am in agreement with Colin. Your solution is fine (sorry for not making it clearer). I just felt that you were taking DRY a bit too literally. So long as repeated code doesn't leave your code open to breakages when a component that's relied on by any dependencies is change, there's no problem. I think Colin said it better. He seems to be much better at creating legible sentences than I am.

[eluser]Xeoncross[/eluser]
Looks fine to me, the only thing I would suggest is that if there is ANYCHACE that you might have a controller that DOESN'T use the getMainNav() function - that you don't call the function in the base controller. Instead move it to the controller constructor of the classes you need it.

Personally, I almost always have an API or AJAX controller that would only waste it's time doing any kind of theme/layout/menu stuff as it is just for sending and receiving JSON/XML data. So my system would just waste it's time with that function when the ajax/api controller was called.

[eluser]Barber of Padua[/eluser]
I'll keep your point of view in mind, Fuzzy0ne. Personally, I immediately get very suspicious about repeated code. That's mostly because I've had some nasty trouble with my own code in the past, so not out of dogmatism. I agree that's something else to avoid. ;-)

[eluser]Barber of Padua[/eluser]
XeonCross, the getMainNav() method could be moved to the Page class, provided there won't be much other Controller classes which need it. And certainly should be there, if the Page class is one of the few classes which use it. Right?