Is this how an MVC controller supposed to be? This is the controller I created for my MVC framework, and I think I finally got it right.

Anything I can do to make the code more efficient?

Is this how a MVC controller is supposed to look like?

abstract class Controller
{
    protected $requestHandler;

    public function __construct(RequestHandler $rh, $action)
    {
        $this-&gt;requestHandler = $rh;

        $this-&gt;$action();
    }

    protected abstract function index();
}

class SignUpController extends Controller
{
    protected function index()
    {
        $headerData = array(
            'title' =&gt; 'Sign up',
            'stylesheets' =&gt; ['/stylesheets/signup.css'],
            'scripts' =&gt; array(
                '//ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js',
                '/javascripts/misc.js'
            )
        );

        View::render('Header', $headerData);

        if ($this-&gt;requestHandler-&gt;getMethod() === 'POST') {
            $v = new Validator();

            $v-&gt;addRule(1, ['required', 'max' =&gt; 35]);
            $v-&gt;addRule(2, ['required', 'between' =&gt; [5, 255], 'email']);
            $v-&gt;addRule(3, ['required', 'min' =&gt; 6]);
            $v-&gt;addRule(4, ['required']);

            $firstname    = $v-&gt;validate(1, (isset($_POST['firstname']))    ? trim($_POST['firstname'])    : false);
            $lastname     = $v-&gt;validate(1, (isset($_POST['lastname']))     ? trim($_POST['lastname'])     : false);
            $email        = $v-&gt;validate(2, (isset($_POST['email']))        ? trim($_POST['email'])        : false);
            $emailConfirm = $v-&gt;validate(2, (isset($_POST['emailConfirm'])) ? trim($_POST['emailConfirm']) : false);
            $password     = $v-&gt;validate(3, (isset($_POST['password']))     ?      $_POST['password']      : false);
            $terms        = $v-&gt;validate(4,  isset($_POST['terms']));

            if ($emailConfirm === true &amp;&amp; trim($_POST['email']) !== trim($_POST['emailConfirm'])) {
                $emailConfirm = $v-&gt;setError('match');
            }

            if (!$v-&gt;errors()) {
                //user sign up logic here

                View::render('SignUpSuccess', ['email' =&gt; trim($_POST['email'])]);
            } else {
                $signUpData = array(
                    'firstname'    =&gt; $firstname,
                    'fnVal'        =&gt; (isset($_POST['firstname'])) ? $_POST['firstname'] : false,
                    'lastname'     =&gt; $lastname,
                    'lnVal'        =&gt; (isset($_POST['lastname'])) ? $_POST['lastname'] : false,
                    'email'        =&gt; $email,
                    'emVal'        =&gt; (isset($_POST['email'])) ? $_POST['email'] : false,
                    'emailConfirm' =&gt; $emailConfirm,
                    'emConfVal'    =&gt; (isset($_POST['emailConfirm'])) ? $_POST['emailConfirm'] : false,
                    'password'     =&gt; $password,
                    'pwVal'        =&gt; (isset($_POST['password'])) ? $_POST['password'] : false,
                    'terms'        =&gt; $terms,
                    'termsVal'     =&gt; isset($_POST['terms'])
                );

                View::render('SignUp', $signUpData);
            }
        } else {
            View::render('SignUp');
        }

        View::render('Footer');
    }
}
MVC has a strict definition. However, frameworks don't actually follow MVC (yet they call themselves such). It's like a free-for-all on framework design, approaches can differ.

Enough of that theory, let's do code.

One issue here is the constructor. If the class is subclassed, but doesn't call the parent when the constructor is overridden, then nothing will run. I suggest you separate a router logic from the controller. The order should be like

Also, I think it's best you place all request data (parameters, headers) into an object, like a $request parameter handed into the function that's called in the controller by the router (pre-cleaned and pre-formatted). Also, throw in getters to normalize the data, so that you won't be doing 'isset` all the time. Something like:

$pass = $req-&gt;post('password'); // Gets the password param from a post request, else false


Don't render by parts, like header, content, footer in the controller. It's best you invest in having a templating system. The template has the fully formed HTML, and all your render does is fill in the blanks. See mustache templates, you might like it.

So the order is:


Receive request (in CodeIgniter, that's the index.php)
index.php loads core libraries and dependencies (like routers, url parsers, db driver, XSS prevention, stuff like that).
Parse the request and simplify where needed. What you could do is 


Sanitize all inputs from XSS and SQL injections.
Normalize input by placing encapsulating them in an object. Throw in a getter, so that you only need to call something like $request-&gt;post('password') to get the password from a POST request.

Then the router determines the route from the request, includes that class, creates an instance, and executes the appropriate method. Something like $classInstance-&gt;theAction($normalizedRequestData)
Now we are in the controller. Controller then talks to the model, do something here and there, gets the data, and renders the view with the data from the DB.

$this-&gt;render('VIEW_NAME',$dataFromDB);

Template parser kicks in, renders the data and generates the HTML which then gets send back to browser.


If you're a visual guy like I am, I'd prefer to look at a diagram rather than a wall of text. Luckily, CodeIgniter drew their data flow, which got me sold. It's pretty handy diagram, which can be used as basis for framework design.