Is this a good controller?

Question

<?php
session_start();
include_once("model/Model.php");
class Controller {
 public $model;
 public function __construct() 
 { 
 $this->model = new Model();
 } 
 public function invoke()
 {
 if (isset($_GET['page']) && $_GET['page'] == "registration")
 {
 if(isset($_POST['username']) && isset($_POST['password']))
 {
 // data send for registration
 $user = $this->model->userRegistration($_POST['username'], $_POST['password'], $_POST['name'], $_POST['familyname'], $_POST['country'], $_POST['age'], $_POST['degree']); 
 if($user->IsRegistered){
 include 'view/registered.php';
 }
 else
 {
 include 'view/registration.php';
 }
 }
 else
 {
 include 'view/registration.php';
 }
 } // if end for registration page
 elseif (isset($_GET['page']) && $_GET['page'] == "login")
 {
 if(isset($_POST['username']) && isset($_POST['password']))
 {
 // data send for login
 $user = $this->model->userLogin($_POST['username'], $_POST['password']);
 if($user->IsLogin){
 $_SESSION["username"] = $user->Username;
 include 'view/home.php';
 }
 else
 {
 include 'view/login.php';
 }
 }
 else
 {
 include 'view/login.php';
 }
 } // elseif end for login page
 elseif (isset($_GET['page']) && $_GET['page'] == "edit_registered")
 {
 if(isset($_REQUEST['eid']))
 {
 // data send for edit registration for a particular user
 $user = $this->model->userEditRegistration($_POST['username'], $_POST['password'], $_POST['name'], $_POST['familyname'], $_POST['country'], $_POST['age'], $_POST['degree'], $_REQUEST['eid']); 
 if($user->IsRegistered){
 include 'view/registered.php';
 }
 else
 {
 include 'view/edit_registered.php';
 }
 }
 else
 {
 include 'view/edit_registered.php';
 }
 } // elseif end for edit registration page
 elseif (isset($_GET['page']) && $_GET['page'] == "delete_registered")
 {
 if(isset($_REQUEST['id']))
 {
 // id send for delete registration for a particular user
 $user = $this->model->userDeleteRegistration($_REQUEST['id']); 
 if($user->IsRegistered){
 include 'view/registration.php';
 }
 else
 {
 include 'view/edit_registered.php';
 }
 }
 else
 {
 include 'view/edit_registered.php';
 }
 } // elseif end for edit registration page since after delete it is redirecting to this page 
 elseif (isset($_GET['page']) && $_GET['page'] == "logout")
 {
 // session is destroyed
 unset($_SESSION["username"]);
 include 'view/home.php';
 } // elseif end for logout
 elseif (isset($_GET['page']) && $_GET['page'] == "intro")
 {
 include 'view/intro.php';
 } // elseif end for content page intro
 elseif (isset($_GET['page']) && $_GET['page'] == "leedsu")
 {
 include 'view/leedsu.php';
 } // elseif end for content page leedsu
 elseif (isset($_GET['page']) && $_GET['page'] == "leedsuleedsmet")
 {
 include 'view/leedsuleedsmet.php';
 } // elseif end for content page leedsuleedsmet
 elseif (isset($_GET['page']) && $_GET['page'] == "trav")
 {
 include 'view/trav.php';
 } // elseif end for content page leedsuleedsmet
 else
 {
 include 'view/home.php';
 } // elseif end for content page home
 }
}
?>

Answer 1

If you want an honest opinion, I'd say no, this is not a good controller. I won't bother with code styles, but as far as logic goes:

1. Conditional includes? Not a good idea. You should be writing code that could be included anywhere if necessary (but possibly not used). Using includes like this can quickly turn your project into a complete mess, especially if the other files are written without the knowledge that they're being included in such a manner.
2. A separate elseif and logic for each URL parameter? Why? There are so many better ways to route users to the correct page. Search Google and you'll find at least a dozen. (Also, you check to make sure $_GET['page'] is set as many as 9 times...)
3. Why is this in a class? In it's current state, with only one function and one instance variable, it definitely doesn't need to be. (Although I might not be seeing everything.)

• mheinzerling: 1) Based on the path I guess the includes are plain templates, so it is not as bad as it seems on the first glance.

Answer 2

No

I'm not sure why this question is tagged CakePHP but since it is: This is not CakePHP code =).

Some specific points to elaborate:

session_start, $_SESSION

This should not be in any CakePHP application code - The session is started on demand whenever it is first accessed via the Session component, session helper or the CakeSession class.

include_once

Files shouldn't need to be included explicitly at all in CakePHP, there's a standard way to load everything. For classes that is using App::uses, for template files it's Controller::render and View::element.

Model

By defining a class called Model, even if it were loaded correctly - CakePHP's base model class cannot be used.

new Model

This is not the right way to construct a model in CakePHP - the normal way is:

$instance = ClassRegistry::init('Name');

As per the previous point - a model class cannot be named "Model".

Controller

By defining a class called Controller - CakePHP's base controller class cannot be used.

$_POST $_REQUEST $_GET

There should basically be no super-global references in app-land code - all of the same data is available via $this->data, $this->data, $this->params. None of that will work with the code in the question though, because it's not based on CakePHP's controller class which populates those variables.

if ($_GET['page'] == "registration")

The function invoke handles everything - there are 10 "actions" in it. Each of these if statements would ordinarily be a separate action (function) in a controller.

include view

That's not how views are rendered in CakePHP. Like that there are:

• No helpers
• No layouts
• No two-pass rendering

Conclusion

There's no CakePHP code here.

If you're interested in writing/using something similar to the code in the question, it may be useful to look for a lightweight framework such as, for example, slim - either use it as is or pick through the source to make your own framework.

Answer 3

A common pattern for your scenario is to create a action...()-method for every page and extract the routing (your outer if) to a separate class. Rename your Model to UserModel and create new Models for every other entity which exists in your system. In a similar way you could group your pages/actions to different controller.

In general you should also step back and think about your code, if you discover that you are copying some part of you code do create new behavior (new pages). Even if we assume your idea would be a suitable solution, then there is still a lot of duplicated code. You can either try to extract a method or create a loop to handle the common parts.

From a security point of view, there might be an issue if there is no other validation in you models and you directly forward the POST values to you database (without prepared statements).