Refactoring my controllers with a BaseController to remove repetitive code? [closed]

Question

Take this controller for instance. Is there a way I can rip out the constructor and property, extend a base controller class and still have $twig? I feel like this repeitive behaviour can be removed.

I understand that I need to extend a base class, but I'm not 100% sure where to go from there.

namespace App\Controllers;

class Welcome extends {
    private $twig;

    public function __construct(\Twig_Environment $twig) {
        $this->twig = $twig;
    }

    public function getTemplate() {
        return $this->twig->render('/templates/welcome.html', array());
    }
}

?>

When I try and create a base class like this, removing the constructor and property from the Welcome class, I get Undefined property: App\Controllers\Welcome::$twig

<?php declare(strict_types = 1);

namespace App\Controllers;

class Base {
    private $twig;

    public function __construct(\Twig_Environment $twig) {
        $this->twig = $twig;
    }

}

?>

If I have 100 controllers, that's 100 classes to refactor if I ever change my template engine, surely there must be a way to add some sort of base class to do this? And then extend it in all classes.

Answer 1

Lets look at your code

namespace App\Controllers;

class Welcome extends {
    private $twig;

    public function __construct(\Twig_Environment $twig) {
        $this->twig = $twig;
    }

    public function getTemplate() {
        return $this->twig->render('/templates/welcome.html', array());
    }
}
//------------------------
namespace App\Controllers;

class Base {
    private $twig;

    public function __construct(\Twig_Environment $twig) {
        $this->twig = $twig;
    }

}

You have 3 Big issues here:

1. class Welcome extends { - extends what
2. private $twig; this property is private, which means only the class it's defined in can access it
3. public function __construct

Lets fix these (this simplest way):

namespace App\Controllers;
class Welcome extends Base {
    protected $twig; //use protected - even though private would work this way

    public function __construct(\Twig_Environment $twig) {
        $this->twig = $twig;
    }

    public function getTemplate() {
        return $this->twig->render('/templates/welcome.html', array());
    }
}
//------------------------
namespace App\Controllers;

class Base {

    /*public function __construct(\Twig_Environment $twig) {
        parent::__construct($twig); //or omit the constructor all togather
    }*/

}

In this simple example, you can just Nuke the guts of the child class as there is nothing specific to that class in the code shown. This happens to be the whole point of inheritance.

I left the constructor in comments, in this minimal example its not needed. But you may want it in the future, so you can use parent to call the parent classes version of any protected/public methods. Regardless if $twig is private we will want to try to re-use as much of the parent code as we can. If it's private you have no choice but to use the parent's version, or set it after the fact.

If you want to keep $twig private for some reason, keep in mind the property is only visible within the Base class. So any work you do directly on it must also be in that same class. This means you can setup a method to return it, or set it in the parent class and call it from the child class. This is rather trivial so I don't think it warrants an example, but if you want one, just let me know.

To be honest I use private about 10% of the time. Only when I have some value only the parent class should "know" about. And example would be say you have a base class that connects to the DB. Well the act of connecting and the data needed for that is largely irrelevant to the child classes. They don't (nor should they) care how the DB connection happened as the parent can take care of that without any need for the child to be aware of it. All the child needs is whatever connection resource you get from the act of connecting.

When thinking about OOP, one very good thing to learn is called S.O.L.I.D

https://en.wikipedia.org/wiki/SOLID

Single responsibility principle[6] A class should have only a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.

Open architecture "Software entities ... should be open for extension, but closed for modification."

Liskov substitution principle "Objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program." See also design by contract.

Interface segregation principle "Many client-specific interfaces are better than one general-purpose interface."

Dependency inversion principle One should "depend upon abstractions, [not] concretions."

By having the template object private, your breaking rule Number2 or the Open architecture. You could make Base an Abstract class (if it doesn't extend a concrete class), this way your not tempted to use it as a concrete Object. That's mostly 5. If the parent is "responsible" for creating the template, there is no need for the child to take on that responsibility 1 and some of 3 (by not duplicating the code, we insure that Welcome can seamlessly replace Base etc... Because this is not 100% code you control, a Controller must meet certain specifications for example, there is only so much you can do. A controller is also generally the end result of a Request so you won't have much need of interfaces etc. Because you can't just load a controller at will and use it. Interfaces are for type hinting your inputs to insure they posses the methods the interface demands. That is the contract for the object.

These are "general" guide lines, you should try to use. But that doesn't mean you have to use all of them. For example don't make abstract classes and interfaces just because they are part of SOLID principals, due it because it's the correct thing to do for that object.

Cheers!

Answer 2

You can make a 'getter' method for $twig in your base controller, like this:

class Base {
    private $twig;

    public function __construct(\Twig_Environment $twig) 
    {
        $this->twig = $twig;
    }

    public function getTwig()
    {
      return $this->twig;
    }

}

This would make the getTemplate() method look like this:

public function getTemplate() {
    return $this->getTwig()->render('/templates/welcome.html', array());
}

Or you could make $twig protected instead of private. See:

https://www.php.net/manual/en/language.oop5.visibility.php

Your code is far from complete, and so my answer is too. There may be other ways to do things. Always keep in might that ownership of classes should guide your decisions. For instance, if you don't own the twig class you could use the base controller to interface with it. In that case you would not make a getter for $twig, but implement a method for render(), inside the base controllor, which can be used by extended classes. That way you will only ever need to change the base controller if the twig class changes.