Looking for constructive criticism of my simple MVC classes..

[cgm225]

I am looking to get some constructive criticism of my simple MVC.  Some things I would like feedback on are:

 

1. In each class, am I initially declaring all the variables I need to?

2. Is the way I pass the $pageDir from class to class ok?  (I feel like there is a better way to do this?)

3. Is the way I plan to include default variables in my View class ok?  (I need to provide those default values somewhere, but want to make sure this location is a good one)

4. How is my overall syntax and commenting?

5. Do I have good flow of logic?

6. How does my simple exception handling look to you?

 

Overall, I was trying to create a good BUT simple MCV.

 

Thank you all in advance!  You have all been so helpful!

 

 

<?php

class FrontController {

    //Declaring variable
    private static $pageDir;
    
    //Setting page directory and starting new instance of this class
    public static function createInstance($pageDir){
        self::$pageDir = $pageDir;
        
        $instance = new self();
        return $instance;
    }

    public function dispatch($throwExceptions = false) {

        /*  Checking for the GET variables $module and $action, and, if present, will strip them down
            with a regex function with a white list of allowed characters, removing anything that is 
            not a letter, number, underscore or hyphen.
        */

        $regex = '/[^-_A-z0-9]++/';
        $module= isset($_GET["module"]) ? preg_replace($regex, '', $_GET['module']) : "home";
        $action = isset($_GET["action"]) ? preg_replace($regex, '', $_GET['action']) : "frontpage";

        //Generating class name (example: HomeActions)
        $class = ucfirst($module) . "Actions";

        //Generating path to Actions class (example: home/HomeActions.php5)
        $file = self::$pageDir . "/" . $module . "/" . $class . ".php5";
        if (!is_file($file)) {
            throw new FrontControllerException("Page not found!");
        }

        //Requiring the Actions class (example: home/HomeActions.php5)
        require_once $file;

        //Creating a new instance of the Actions class (example: $controller = new HomeActions()
        $controller = new $class();

        //Passing page directory variable to the ActionController class
        $controller->setPageDir(self::$pageDir);

        try {
            //Using the setName function in the ActionController class
            $controller->setName($module);

            //Checks if the method exists, then runs the displayView function in the ActionController class
            $controller->dispatchAction($action);
        }
        catch(Exception $e){
            //An exception has occurred, and an error message will be displayed if $throwExceptions is set to TRUE
            if($throwExceptions){
                //$throwExceptions is true, therefore, a detailed exception message is echoed
                echo $e;
            } else {
                //$throwExceptions is false, therefore, a simple error message is echoed
                echo $e->errorMessage();
            }
        }
    }
}

abstract class ActionController {

    //Declaring variable(s)
    private static $pageDir;
    protected $viewData = array();
    
    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setName($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }

    //Placing a value in the $viewData array at the key position
    public function setVar($key, $value) {
        $this->viewData[$key] = $value;
    }

    //Returns a value from the $viewData array located at the key position
    public function getVar($key) {
        if (array_key_exists($key, $this->viewData)) {
            return $this->viewData[$key];
        }
    }
    
    /*
        Checking for actionMethod in the Actions class (example: doFrontpage() within home/HomeActions.php5)
        with the method_exists function and, if present, the actionMethod and displayView functions are
        executed.
    */
    
    public function dispatchAction($action) {
        $actionMethod = "do" . ucfirst($action);
        if (!method_exists($this, $actionMethod)) {
            throw new FrontControllerException("Page not found!");
        }

        $this->$actionMethod();
        $this->displayView($action);
    }

    public function displayView($action) {
        if (!is_file(self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5")) {
            throw new FrontControllerException("Page not found!");
        }

        //Setting content variable to the path of the action View file
        $this->actionView = self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5";

        //Creating a new instance of the View class and passing the $pageDir, $viewData, and actionView variables
        $view = new View();
        $view->setPageDir(self::$pageDir);
        $view->setViewData($this->viewData);
        $view->setVar("actionView","$this->actionView");
        $view->render();
    }
}

class View extends ActionController {

    //Declaring variable(s)
    private static $pageDir;

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setViewData($viewData){
        $this->viewData = $viewData;
    }

    public function render() {

        //Default variables, to be placed in an outside/included file
        $title = "Example.com";

        //This foreach function goes over all of the elements in $this->viewData array, creates
        //a variable for every value, and the name of the value (the variable name) is the key's value.
        foreach ($this->viewData as $key => $value) {
            $$key = $value;
        }

        //Including a template file within which the action View file is included
        require_once self::$pageDir . "/default.tpl";
    }

}

class FrontControllerException extends Exception {
    public function errorMessage(){
        //Simple error message returned if $throwExceptions is false
        return $this->getMessage();
    }
}

/*
    Notes:
        Abstract Class:
            An abstract class is a class that cannot (or should not) be instantiated.

        PHP 5 allows you to declare properties and methods as public, private or protected.
        These are defined as:
            Public: anyone either inside the class or outside can access them
            Private: only the specified class can access them. Even subclasses will be denied access.
            Protected: only the specified class and subclasses can access them
        
        The PHP Static Statement:
            The PHP static statement is that it allows a function to "remember" the value of a local
            variable for the next time the function is called.

    Reference(s):
        "A lightweight and flexible front controller for PHP 5" by Chris Corbyn
        http://www.w3style.co.uk/a-lightweight-and-flexible-front-controller-for-php-5

*/

?>

[KevinM1]

I believe you need to declare $name in your ActionController class.  You can't reference $this->name without declaring it first.

 

You also have a small redundancy in your View class.  If you define something in an abstract base/super/parent class that you want a child class to have, you don't need to copy it into the child class.  As long as those properties and methods are public or protected (recommended), the child class will automatically have them available.  That's the whole point to inheritance.  So, having another setPageDir() method in your View class is redundant as it's public in the abstract base class.

[KevinM1]

Looking at it further, your FrontController class should be a Singleton, with setter/getter functions to handle the directory path stuff.  I doubt you want to have more than one Front Controller active at the same time.  If you're not familiar with that pattern, it's basically:

 

<?php
   class SingletonExample{
      private static $instance; //static, so the class 'remembers' whether or not there's an instance floating around

      private function __construct(){} //PRIVATE constructor, so it can't be instantiated by the outside world

      public static function getInstance(){ //checks to see if it's been instantiated yet.  If not, make a new one.  If so, return the current one
         if(!self::$instance){
            self::$instance = new self();
         }

         return self::$instance;
      }
   }
?>

[cgm225]

I am going to try to implement everything you have discussed, and now declare the name variable in my action controller.  I think I have also fixed the class to be more singleton in pattern.

 

So moving to your other point, with regard to that setPageDir redundancy in the View class, my question is then, how do you get the value of the $pageDir variable from the ActionController to the View class?

 

(I will repost my code after I hear back from you and work on it a bit more)

 

Thanks so much for the help!

[KevinM1]

For the page directory, why not try:

 

<?php
   abstract class ActionController{
      private static $instance;
      private static $pageDir = 'something'; //default value

      private function __construct(){}

      public static function getInstance(){
         if(!self::$instance){
            self::$instance = new self();
         }

         return self::$instance;
      }

      public function setDir($pageDir){
         self::$pageDir = $pageDir;
      }

      public function getDir(){
         return self::$pageDir;
      }

      .
      .
      .
   }

   class View extends ActionController{}

   $view = View::getInstance();

   $view->setDir('/home/views/');
   echo "{$view->getDir()}";
?>

 

As far as getting $pageDir from ActionController to View goes, remember that ActionController is abstract.  You're not going to be instantiating it, unless you decide to not make it abstract.  That said, I put the mechanisms for handling it in the base ActionController class because that's the most logical place to put it.  Unless the View class requires a different implementation than its parent -- in which case there's little use in making it extend ActionController -- there's no need to put it there.

 

In any event, ideally, the page directory will either be hardwired into your script, which is why I assigned it a default (albeit non-legit) path within the class itself, or it will be obtained from another source, such as an external configuration file, which is why I added the setDir() method.

 

Something to keep in mind: singletons are essentially global variables.  So, you need to be sure that for important things, like the value of the directory that holds your views, you're not overwriting/changing that value in other areas.  You may want to add a static flag to the class which is triggered when setDir() is called, so the directory can only be set once.

[cgm225]

First, thank you so much for your help! 

 

First, I am still having some conceptual or possibly syntax problems I need help with.  First, should I be including my getInstance() and constructor functions within my ActionController or FrontController class?  Right now, I have them in my front controller class, and then instantiate the entire MVC like so:

 

FrontController::setPageDir(PAGE_DIR);
FrontController::getInstance()->dispatch(false);

 

Is this the best way to be doing this?

 

Second, I am still having a hard time fixing the redundancy of the setPageDir() function, which is found within all three of my classes (ActionController, FrontController, and View).  Could you more explicitly should me how to use the setPageDir() in the View class by extending the ActionController?  Or how I can best/appropriately provide the value of the $pageDir variable to all these classes without being redundant?

 

Here is what my classes look like now:

 

<?php

class FrontController {

    //Declaring variable
    private static $instance;
    private static $pageDir;

    private function __construct(){}
    
    //Setting page directory and starting new instance of this class with a Singleton pattern
    public static function getInstance(){
        if(!self::$instance){
            self::$instance = new self();
        }
        return self::$instance;
    }

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function dispatch($throwExceptions = false) {

        /*  
            Checking for the GET variables $module and $action, and, if present, will strip them down
            with a regular expression function with a white list of allowed characters, removing anything 
            that is not a letter, number, underscore or hyphen.
        */

        $regex = '/[^-_A-z0-9]++/';
        $module= isset($_GET["module"]) ? preg_replace($regex, '', $_GET['module']) : "home";
        $action = isset($_GET["action"]) ? preg_replace($regex, '', $_GET['action']) : "frontpage";

        //Generating class name (example: HomeActions)
        $class = ucfirst($module) . "Actions";

        //Generating path to Actions class (example: home/HomeActions.php5)
        $file = self::$pageDir . "/" . $module . "/" . $class . ".php5";
        if (!is_file($file)) {
            throw new FrontControllerException("Page not found!");
        }

        //Requiring the Actions class (example: home/HomeActions.php5)
        require_once $file;

        /*  
            Creating a new instance of the Actions class (example: $controller = new HomeActions()
            and passing page directory variable to the ActionController class
        */
            
        $controller = new $class();
        $controller->setPageDir(self::$pageDir);

        try {
            //Using the setName function in the ActionController class
            $controller->setName($module);

            //Checks if the method exists, then runs the displayView function in the ActionController class
            $controller->dispatchAction($action);
        }
        catch(Exception $e){
            //An exception has occurred, and an error message will be displayed if $throwExceptions is set to TRUE
            if($throwExceptions){
                echo $e;
            } else {
                echo $e->errorMessage();
            }
        }
    }
}

abstract class ActionController {

    //Declaring variable(s)
    private static $pageDir;
    protected $name;
    protected $viewData = array();

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setName($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }

    //Placing a value in the $viewData array at the key position
    public function setVar($key, $value) {
        $this->viewData[$key] = $value;
    }

    //Returns a value from the $viewData array located at the key position
    public function getVar($key) {
        if (array_key_exists($key, $this->viewData)) {
            return $this->viewData[$key];
        }
    }
    
    /*
        Checking for actionMethod in the Actions class (example: doFrontpage() within home/HomeActions.php5)
        with the method_exists function and, if present, the actionMethod and displayView functions are
        executed.
    */
    
    public function dispatchAction($action) {
        $actionMethod = "do" . ucfirst($action);
        if (!method_exists($this, $actionMethod)) {
            throw new FrontControllerException("Page not found!");
        }
        $this->$actionMethod();
        $this->displayView($action);
    }

    public function displayView($action) {
        if (!is_file(self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5")) {
            throw new FrontControllerException("Page not found!");
        }

        //Setting content variable to the path of the action View file
        $this->actionView = self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5";

        //Creating a new instance of the View class and passing the $pageDir, $viewData, and actionView variables
        $view = new View();
        $view->setPageDir(self::$pageDir);
        $view->setViewData($this->viewData);
        $view->setVar("actionView","$this->actionView");
        $view->render();
    }
}

class View extends ActionController {

    //Declaring variable(s)
    private static $pageDir;

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setViewData($viewData){
        $this->viewData = $viewData;
    }

    public function render() {

        //Default variables, (To be placed in an outside/included file!!)
        $title = "Example.com";

        //This foreach function goes over all of the elements in $this->viewData array, creates
        //a variable for every value, and the name of the value (the variable name) is the key's value.
        foreach ($this->viewData as $key => $value) {
            $$key = $value;
        }

        //Including a template file within which the action View file is included
        require_once self::$pageDir . "/default.tpl";
    }
}

class FrontControllerException extends Exception {
    public function errorMessage(){
        //Error message
        return $this->getMessage();
    }
}

/*
    Notes:
        Abstract Class:
            An abstract class is a class that cannot (or should not) be instantiated.

        PHP 5 allows you to declare properties and methods as public, private or protected.
        These are defined as:
            Public: anyone either inside the class or outside can access them
            Private: only the specified class can access them. Even subclasses will be denied access.
            Protected: only the specified class and subclasses can access them
        
        The PHP Static Statement:
            The PHP static statement is that it allows a function to "remember" the value of a local
            variable for the next time the function is called.

    Reference(s):
        "A lightweight and flexible front controller for PHP 5" by Chris Corbyn
        http://www.w3style.co.uk/a-lightweight-and-flexible-front-controller-for-php-5

*/

?>

 

 

 

Thanks again for all your help!

[KevinM1]

First, thank you so much for your help! 

 

First, I am still having some conceptual or possibly syntax problems I need help with.  First, should I be including my getInstance() and constructor functions within my ActionController or FrontController class?  Right now, I have them in my front controller class, and then instantiate the entire MVC like so:

 

FrontController::setPageDir(PAGE_DIR);
FrontController::getInstance()->dispatch(false);

 

Is this the best way to be doing this?

 

You need to reverse that, so:

 

$controller = FrontController::getInstance(); //you need an instance before you can play with it
$controller->setPageDir(PAGE_DIR); //note how it's not a static function
$controller->dispatch(false);

 

Second, I am still having a hard time fixing the redundancy of the setPageDir() function, which is found within all three of my classes (ActionController, FrontController, and View).  Could you more explicitly should me how to use the setPageDir() in the View class by extending the ActionController?  Or how I can best/appropriately provide the value of the $pageDir variable to all these classes without being redundant?

 

Here is what my classes look like now:

 

<?php

class FrontController {

    //Declaring variable
    private static $instance;
    private static $pageDir;

    private function __construct(){}
    
    //Setting page directory and starting new instance of this class with a Singleton pattern
    public static function getInstance(){
        if(!self::$instance){
            self::$instance = new self();
        }
        return self::$instance;
    }

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function dispatch($throwExceptions = false) {

        /*  
            Checking for the GET variables $module and $action, and, if present, will strip them down
            with a regular expression function with a white list of allowed characters, removing anything 
            that is not a letter, number, underscore or hyphen.
        */

        $regex = '/[^-_A-z0-9]++/';
        $module= isset($_GET["module"]) ? preg_replace($regex, '', $_GET['module']) : "home";
        $action = isset($_GET["action"]) ? preg_replace($regex, '', $_GET['action']) : "frontpage";

        //Generating class name (example: HomeActions)
        $class = ucfirst($module) . "Actions";

        //Generating path to Actions class (example: home/HomeActions.php5)
        $file = self::$pageDir . "/" . $module . "/" . $class . ".php5";
        if (!is_file($file)) {
            throw new FrontControllerException("Page not found!");
        }

        //Requiring the Actions class (example: home/HomeActions.php5)
        require_once $file;

        /*  
            Creating a new instance of the Actions class (example: $controller = new HomeActions()
            and passing page directory variable to the ActionController class
        */
            
        $controller = new $class();
        $controller->setPageDir(self::$pageDir);

        try {
            //Using the setName function in the ActionController class
            $controller->setName($module);

            //Checks if the method exists, then runs the displayView function in the ActionController class
            $controller->dispatchAction($action);
        }
        catch(Exception $e){
            //An exception has occurred, and an error message will be displayed if $throwExceptions is set to TRUE
            if($throwExceptions){
                echo $e;
            } else {
                echo $e->errorMessage();
            }
        }
    }
}

abstract class ActionController {

    //Declaring variable(s)
    private static $pageDir;
    protected $name;
    protected $viewData = array();

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setName($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }

    //Placing a value in the $viewData array at the key position
    public function setVar($key, $value) {
        $this->viewData[$key] = $value;
    }

    //Returns a value from the $viewData array located at the key position
    public function getVar($key) {
        if (array_key_exists($key, $this->viewData)) {
            return $this->viewData[$key];
        }
    }
    
    /*
        Checking for actionMethod in the Actions class (example: doFrontpage() within home/HomeActions.php5)
        with the method_exists function and, if present, the actionMethod and displayView functions are
        executed.
    */
    
    public function dispatchAction($action) {
        $actionMethod = "do" . ucfirst($action);
        if (!method_exists($this, $actionMethod)) {
            throw new FrontControllerException("Page not found!");
        }
        $this->$actionMethod();
        $this->displayView($action);
    }

    public function displayView($action) {
        if (!is_file(self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5")) {
            throw new FrontControllerException("Page not found!");
        }

        //Setting content variable to the path of the action View file
        $this->actionView = self::$pageDir . "/" . $this->getName() . "/" . $action . "View.php5";

        //Creating a new instance of the View class and passing the $pageDir, $viewData, and actionView variables
        $view = new View();
        $view->setPageDir(self::$pageDir);
        $view->setViewData($this->viewData);
        $view->setVar("actionView","$this->actionView");
        $view->render();
    }
}

class View extends ActionController {

    //Declaring variable(s)
    private static $pageDir;

    //Setting the page directory, which is passed from the FrontController
    public static function setPageDir($pageDir){
        self::$pageDir = $pageDir;
    }

    public function setViewData($viewData){
        $this->viewData = $viewData;
    }

    public function render() {

        //Default variables, (To be placed in an outside/included file!!)
        $title = "Example.com";

        //This foreach function goes over all of the elements in $this->viewData array, creates
        //a variable for every value, and the name of the value (the variable name) is the key's value.
        foreach ($this->viewData as $key => $value) {
            $$key = $value;
        }

        //Including a template file within which the action View file is included
        require_once self::$pageDir . "/default.tpl";
    }
}

class FrontControllerException extends Exception {
    public function errorMessage(){
        //Error message
        return $this->getMessage();
    }
}

/*
    Notes:
        Abstract Class:
            An abstract class is a class that cannot (or should not) be instantiated.

        PHP 5 allows you to declare properties and methods as public, private or protected.
        These are defined as:
            Public: anyone either inside the class or outside can access them
            Private: only the specified class can access them. Even subclasses will be denied access.
            Protected: only the specified class and subclasses can access them
        
        The PHP Static Statement:
            The PHP static statement is that it allows a function to "remember" the value of a local
            variable for the next time the function is called.

    Reference(s):
        "A lightweight and flexible front controller for PHP 5" by Chris Corbyn
        http://www.w3style.co.uk/a-lightweight-and-flexible-front-controller-for-php-5

*/

?>

 

 

 

Thanks again for all your help!

 

With your setup, I think the easiest thing to do is simply ensure that whenever you set the $pageDir variable in one object, you do the same for the other.  That option leaves something to be desired if you really want to have that info available across the board.  You could always refactor and create another class that holds/sets configuration data like this so your FrontController and View classes can retrieve it from there.  These objects are typically refered to as registry objects.  You register application info with them and any other class that needs that info can retrieve it.  I have some sample code you can look at if you're interested.

 

With ActionController, View, and $pageDir, it's a simple matter of declaring it as a protected static variable inside ActionController.  I think I messed up previously and had it private, but it really should be protected.  Like I said before, you're not instantiating an ActionController because it's abstract.  But, since it's the base/parent class, you should be putting whatever functionality you want all its child classes to have within it.  So, anything that's not specific to View should be put in ActionController.

 

Just a note about the singleton(s).  Remember to assign getInstance() to a variable like I showed above.  This gets your hands on the object itself.  Then you can play with it.  Or, if you don't want direct access to it, and just want it to run, you can do something like (my test front controller):

 

<?php
   class MP_controller_Controller{
      private function __construct(){} //private constructor because it shouldn't be instantiated directly

      static function run(){
         $instance = new MP_controller_Controller(); //$instance IS NOT a property because we don't need the client code to have direct access.  We just want it to run once per request.  The constructor works because we're calling it from WITHIN the object itself.  It's not private to itself.
         $instance->init();
         $instance->handleRequest();
         $instance->display();
      }

      private function init(){} //nothing to initialize yet, but there for the future

      private function handleRequest(){
         $request = new MP_controller_Request(); //either $_POST or $_GET...the rest is separate OOP stuff
         $commandFactory = new MP_command_CommandFactory();
         $command = $commandFactory->getCommand($request);
         $command->execute($request);
      }

      private function display(){ //display username if they logged in
         $sessReg = MP_base_SessionRegistry::getInstance(); //another singleton, this is a registry that deals with session info

         if(!$sessReg->isEmpty()){
            $user = $sessReg->getUser();

            if($user){
               echo "Welcome back, {$user->getName()}<br /><br />\n\n";
            }
         }
      }
   }
?>

<?php
   /* client code */

   MP_controller_Controller::run();
?>

[cgm225]

Thanks again for your help.  I have made the changes you discussed, having now (1) my getInstance() assigned to a variable and (2) a protected $pageDir (which allowed me to remove its repeated declaration and the setPageDir() function from my view class).

 

As far as "registry objects"...

 

These objects are typically referred to as registry objects.  You register application info with them and any other class that needs that info can retrieve it.  I have some sample code you can look at if you're interested.

 

...if you have some simple code I can look at (simple because I am still very new to OOP), I would love to see it.  I would like to apply this idea of a registry object to (1) my $pageDir variable information and (2) my default variables that I intend to include in my View class (of which there is currently an example of a default $title variable in the code I have previously posted).  Also, how are these objects "included" in Classes where I need that information?  Do I actual include them, or extend, or something else?

 

Thanks again for everything.  This discussion has been so helpful!

 

 

(I will repost my code again after I hear back and work on it some more.)

[cgm225]

Follow-up: I found this Registry class online.. what do you think of it?

 

class Registry {

    private $store = array();

    public function __construct() {}

    public function __set($label, $object) {
        if(!isset($this->store[$label]))
        {
            $this->store[$label] = $object;
        }
    }

    public function __unset($label) {
        if(isset($this->store[$label]))
        {
            unset($this->store[$label]);
        }
    }

    public function __get($label) {
        if(isset($this->store[$label]))
        {
            return $this->store[$label];
        }
        return false;
    }

    public function __isset($label) {
        if(isset($this->store[$label]))
        {
            return true;
        }
        return false;
    }
}

[aschk]

Yup, that registry object should work ok for you.

 

I would like to commend you on your excellent work. I've read through it and i'm very impressed with what you've come up with.

 

I think you're trying to create a templating engine (view) and i'm not sure you should be confusing a controller with a view. Logically they should be rather different; the view consisting of html+templating engine (i.e smarty or your own rolled engine), and your controller should be performing the actions on your business model (backend logic).

You appear to be merging the 2 together at the minute. I'll keep an eye on this topic and see how you get on.

[cgm225]

Thank you all for your help.  My View definitely needs some work, which I will take on next.  However, I just finished implementing a registry class which also has an import method using the parse_ini_file() function so a config.ini file can be used.  My questions are:

 

1. Is my initial instantiation of the registry and front controller ok?  Is there a better way to do this?

2. Do I need to declare the registry variable somewhere?

3. Is it ok how I am passing the $this->registry variable with the getRegistry method from class to class, or is there a better way to do this?

 

Any other feedback is greatly appreciated!

 

My index.php5 where I instantiate the registry and front controller:

$registry = new Registry();
$registry->import(PAGE_DIR. '/config.ini');

$controller = FrontController::getInstance();
$controller->getRegistry($registry);
$controller->dispatch(false);

 

My config file:

[mcvSettings]
rootPath = "/web/example.com/includes/php"

 

My classes:

<?php

class FrontController extends ActionController {

    //Declaring variable
    private static $instance;
    protected $controller;
    
    private function __construct(){}
    
    //Setting page directory and starting new instance of this class with a Singleton pattern
    public static function getInstance() {
        if(!self::$instance) {
            self::$instance = new self();
        }
        return self::$instance;
    }

    public function dispatch($throwExceptions = false) {

        /*  
            Checking for the GET variables $module and $action, and, if present, will strip them down
            with a regular expression function with a white list of allowed characters, removing anything 
            that is not a letter, number, underscore or hyphen.
        */

        $regex  = '/[^-_A-z0-9]++/';
        $module = isset($_GET["module"]) ? preg_replace($regex, '', $_GET['module']) : "home";
        $action = isset($_GET["action"]) ? preg_replace($regex, '', $_GET['action']) : "frontpage";

        //Generating class name (example: HomeActions)
        $class = ucfirst($module) . "Actions";

        //Generating path to Actions class (example: home/HomeActions.php5)
        $file = $this->registry->mcvSettings['rootPath'] . "/" . $module . "/" . $class . ".php5";

        if (!is_file($file)) {
            throw new FrontControllerException("Page not found!");
        }

        //Requiring the Actions class (example: home/HomeActions.php5)
        require_once $file;

        /*  
            Creating a new instance of the Actions class (example: $controller = new HomeActions()
            and passing page directory variable to the ActionController class
        */
            
        $controller = new $class();
        $controller->getRegistry($this->registry);

        try {
            //Using the setName function in the ActionController class
            $controller->setName($module);

            //Checks if the method exists, then runs the displayView function in the ActionController class
            $controller->dispatchAction($action);
        }
        catch(Exception $e) {
            //An exception has occurred, and an error message will be displayed if $throwExceptions is set to TRUE
            if($throwExceptions) {
                echo $e; //Full exception echoed
            } else {
                echo $e->errorMessage(); //Simple error messaged echoed
            }
        }
    }
}

abstract class ActionController {

    //Declaring variable(s)
    protected $name;
    protected $viewData = array();
    
    public function getRegistry($registry) {
        $this->registry = $registry;
    }

    public function setName($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }

    //Placing a value in the $viewData array at the key position
    public function setVar($key, $value) {
        $this->viewData[$key] = $value;
    }

    //Returns a value from the $viewData array located at the key position
    public function getVar($key) {
        if (array_key_exists($key, $this->viewData)) {
            return $this->viewData[$key];
        }
    }
    
    /*
        Checking for actionMethod in the Actions class (example: doFrontpage() within home/HomeActions.php5)
        with the method_exists function and, if present, the actionMethod and displayView functions are
        executed.
    */
    
    public function dispatchAction($action) {
        $actionMethod = "do" . ucfirst($action);
        if (!method_exists($this, $actionMethod)) {
            throw new FrontControllerException("Page not found!");
        }
        $this->$actionMethod();
        $this->displayView($action);
    }

    public function displayView($action) {
        if (!is_file($this->registry->mcvSettings['rootPath'] . "/" . $this->getName() . "/" . $action . "View.php5")) {
            throw new FrontControllerException("Page not found!");
        }

        //Setting content variable to the path of the action View file
        $this->actionView = $this->registry->mcvSettings['rootPath'] . "/" . $this->getName() . "/" . $action . "View.php5";

        //Creating a new instance of the View class and passing the $pageDir, $viewData, and actionView variables
        $view = new View();
        $view->getRegistry($this->registry);
        $view->setVar("actionView","$this->actionView");
        $view->render();
    }
}

class View extends ActionController {

    public function render() {

        //The following TWO blocks of code can go in a registry + the pageDir
        //Default variables, (To be placed in an outside/included file!!)
        $title = "Example.com";

        //This foreach function goes over all of the elements in $this->viewData array, creates
        //a variable for every value, and the name of the value (the variable name) is the key's value.
        foreach ($this->viewData as $key => $value) {
            $$key = $value;
        }

        //Including a template file within which the action View file is included
        require_once $this->registry->mcvSettings['rootPath'] . "/default.tpl";
    }
}

class Registry {

    private $store = array();

    public function __set($label, $object) {
        if(!isset($this->store[$label]))
        {
            $this->store[$label] = $object;
        }
    }

    public function __get($label) {
        if(isset($this->store[$label]))
        {
            return $this->store[$label];
        }
        return false;
    }
    
    public function import($filename) {
        if(is_file($filename)) {
            $parsed = parse_ini_file($filename, true);
            foreach($parsed as $key => $value) {
                $this->$key = $value;
            }
        } else {
            throw new Exception("$filename does not exist!");
        }
    }
}

class FrontControllerException extends Exception {
    public function errorMessage() {
        //Error message
        return $this->getMessage();
    }
}

/*
    Notes:
        Constructor(s)
            A constructor is a function which is called right after you create a
            new object. This makes constructor suitable for any object
            initialization you need.
    
        Abstract Class:
            An abstract class is a class that cannot (or should not) be
            instantiated.

        PHP 5 allows you to declare properties and methods as public, private or
        protected. These are defined as:
            Public: anyone either inside the class or outside can access them
            Private: only the specified class can access them. Even subclasses
                will be denied access.
            Protected: only the specified class and subclasses can access them
        
        The PHP Static Statement:
            The PHP static statement is that it allows a function to "remember"
            the value of a local variable for the next time the function is
            called.
        
        The Singleton Pattern
             The Singleton is one of the simplest Patterns to understand. It's
             common usage is to ensure that only one instance of a class is ever
             instantiated. The reason for wanting such behaviour varies but
             typically it is because only one object instantiated from the
             source class is required and you want the resulting object to be
             available throughout an application, i.e. globally available.

    Reference(s):
        "A lightweight and flexible front controller for PHP 5" by Chris Corbyn
        http://www.w3style.co.uk/a-lightweight-and-flexible-front-controller-for-php-5
*/

?>

[Cobby]

I like your code, its very clean, commented and strict.

Have you considered using a static registry class?

 

With regards to a template engine, no matter what you do, you can't really get anything better than smarty. But if you just want to create a very basic template engine that can really only do outputting variables, then making you own would be better to save in file size, efficiency, etc.

 

If you are going to create a separate config file, it could be worth while creating it in an XML format and creating a class to handle it. This way you can update settings from within PHP where as you can't do that as easily with a .ini type file.

[cgm225]

I actually tried a static registry class, but felt like, for what I needed, and my goal of keeping my MVC simple, it was too much.

 

However, since my last post, I have worked my View class a bit more, and have included my most recent MVC code below in case anyone else has any additional feedback.

 

Thanks again for all your help!

 

<?php

class FrontController extends ActionController {

    //Declaring variable(s)
    private static $instance;
    protected $controller;

    private function __construct(){}

    //Starting new instance of this class with a singleton pattern
    public static function getInstance() {
        if(!self::$instance) {
            self::$instance = new self();
        }
        return self::$instance;
    }

    public function dispatch($throwExceptions = false) {

        /*  
            Checking for the GET variables $module and $action, and, if present,
            will strip them down with a regular expression function with a white
            list of allowed characters, removing anything that is not a letter,
            number, underscore or hyphen.
        */
        $regex  = '/[^-_A-z0-9]++/';
        $module = isset($_GET['module']) ? preg_replace($regex, '', $_GET['module']) : 'home';
        $action = isset($_GET['action']) ? preg_replace($regex, '', $_GET['action']) : 'frontpage';

        /*
            Generating Actions class filename (example: HomeActions) and path to
            that class (example: home/HomeActions.php5), checking if $file is a
            valid file, and then, if so, requiring that file.
        */
        $class = ucfirst($module) . 'Actions';
        $file  = $this->pageDir . '/' . $module . '/' . $class . '.php5';

        if (!is_file($file)) {
            throw new FrontControllerException('Page not found!');
        }

        require_once $file;

        /*  
            Creating a new instance of the Actions class (example: $controller
            = new HomeActions(), and passing page directory variable to the
            ActionController class.
        */
        $controller = new $class();
        $controller->setPageDir($this->pageDir);

        try {
            //Using the setModule method in the ActionController class
            $controller->setModule($module);

            /*
                The ActionController dispatchAction method Checks if the method
                exists, then runs the displayView function in the
                ActionController class.
            */    
            $controller->dispatchAction($action);
        }
        catch(Exception $e) {

            /*
                An exception has occurred, and an error message will be
                displayed if $throwExceptions is set to TRUE.
            */
            if($throwExceptions) {
                echo $e; //Full exception echoed
            } else {
                echo $e->errorMessage(); //Simple error messaged echoed
            }
        }
    }
}

abstract class ActionController {

    //Declaring variable(s)
    protected $pageDir;
    protected $module;
    protected $viewData = array();

    public function setPageDir($pageDir){
        $this->pageDir = $pageDir;
    }

    public function setModule($module) {
        $this->module = $module;
    }

    public function getModule() {
        return $this->module;
    }

    //Placing a value in the $viewData array at the key position
    public function setVar($key, $value) {
        $this->viewData[$key] = $value;
    }

    //Returns a value from the $viewData array located at the key position
    public function getVar($key) {
        if (array_key_exists($key, $this->viewData)) {
            return $this->viewData[$key];
        }
    }

    public function getViewData($viewData) {
        $this->viewData = $viewData;
    }

    /*
        Checking for actionMethod in the Actions class (example: doFrontpage()
        within home/HomeActions.php5) with the method_exists function and, if
        present, the actionMethod and displayView functions are executed.
    */  
    public function dispatchAction($action) {
        $actionMethod = 'do' . ucfirst($action);
        if (!method_exists($this, $actionMethod)) {
            throw new FrontControllerException('Page not found!');
        }
        $this->$actionMethod();
        $this->displayView($action);
    }

    public function displayView($action) {
        if (!is_file($this->pageDir . '/' . $this->getModule() . '/' . $action . 'View.php5')) {
            throw new FrontControllerException('Page not found!');
        }

        //Setting $this->actionView to the path of the action View file
        $this->actionView = $this->pageDir . '/' . $this->getModule() . '/' . $action . 'View.php5';

        /*
            Creating a new instance of the View class and passing the $pageDir,
            $viewData, and actionView variables
        */  
        $view = new View();
        $view->setPageDir($this->pageDir);
        $view->getViewData($this->viewData);
        $view->setVar('actionView',$this->actionView);
        $view->render();
    }
}

class View extends ActionController {

    public function render() {

        //Including default template settings
        require_once $this->pageDir . '/defaultTplSettings.php5';

        /*
            Merging the default template variables with variables provided in
            the $this->viewData array, taken from the Actions class, giving
            priority to those provided by the action class, then extracting the
            array with the extract() function, which creates a variable for
            every array value, and the name of the value (the variable name) is
            the key's value.
        */
        $templateSettings = array_merge($defaultTemplateSettings, $this->viewData);
        extract($templateSettings, EXTR_OVERWRITE);

        //Including template file within which the action View file is included
        require_once $this->pageDir . '/default.tpl';
    }
}

class FrontControllerException extends Exception {
    public function errorMessage() {
        //Error message
        return $this->getMessage();
    }
}

?>

[Tim Fletcher]

Firstly, sorry for bringing this post back up. Maybe I should have started a new topic but I felt my comments were most relevant in this thread. I'll repost if necessary.

 

Looking at it further, your FrontController class should be a Singleton, with setter/getter functions to handle the directory path stuff.  I doubt you want to have more than one Front Controller active at the same time.

 

I'm a little confused as to why it is useful to have the Front Controller implemented as a singleton. Surely as a 'core' class and one of the first pieces of code to be executed it would be impossible to have a situation where you could instantiate it more than once. There wold be absolutely no need. I understand that in the case of a database class you might need to use a singleton to ensure only one database object but what does it achieve here? Is it merely good coding practice to use a singleton if you know you only need a single instance regardless of the situation?

 

Thanks!

[aschk]

A good question Tim.

 

I believe that a singleton should only be used in the event that you DON'T want another of it's type in your application. There's nothing wrong with having 2 of the same class so long as they don't cause problems.

 

In the case of the FrontController, you only really want your application working with 1 of it's type. And singletons in web applications work very much like global variables. You're using the same object in all places of your application, and you don't need to pass it by reference.

[Tim Fletcher]

Thanks for your answer aschk.

 

So if I understand correctly - in this instance the purpose of making the front controller a singleton is to ensure its availability throughout the application AND to ensure that each time you access the object it is same object. If you needed access, you would simple 're-instantiate' the object using the FrontController::getInstance() method which would return either a new object (first time) or a reference (subsequent times).

 

That sounds reasonable enough to me. Thanks!

[aschk]

Perfect understanding.