1
\$\begingroup\$

Please could you review and critic my code and logic? Is this is along the right lines of an MVC application class or not?

namespace dh_mvc2;

    use dh_mvc2\dispatchers\Base_Dispatcher;
    use dh_mvc2\routers\Base_Router;
    use dh_mvc2\autoloaders\Basic_Autoloader;
    use dh_mvc2\classes\Config;

    class Application {
    protected $_config;
    protected $_autoloader;
    protected $_app_path;
    protected $_framework_path;
    protected $_router;
    protected $_dispatcher;

    /**
     *
     * @param string $app_path
     *          "path/to/application/directory";
     * @throws \DH_MVC2_Application_Exeption
     */
    public function __construct($app_path, $run = TRUE) {
        if (! is_dir ( $app_path )) {
            require_once 'exeptions/DH_MVC2_Application_Exeption.php';
            throw new \DH_MVC2_Application_Exeption ( "Must supply a path to an application directory" );
        }

        $this->_framework_path = realpath ( dirname ( __DIR__ ) );
        $this->_app_path = $app_path;
        $this->set_ini_default_paths ();
        $this->init ();

        if ($run) {
            $this->run ();
        }
    }

    /**
     * add the framework dir and application dir to include path
     */
    private function set_ini_default_paths() {
        $paths = explode ( PATH_SEPARATOR, get_include_path () );
        if (array_search ( $this->_app_path, $paths ) === false) {
            array_push ( $paths, $this->_app_path );
        }
        if (array_search ( $this->_framework_path, $paths ) === false) {
            array_push ( $paths, $this->_framework_path );
        }
        set_include_path ( implode ( PATH_SEPARATOR, $paths ) );
        spl_autoload_register ();
    }

    /**
     * links to the app and framework default config files and inits them
     * registers autoloader
     */
    private function init() {
        $config_file = '/config/config.ini.php';
        $config_files ['DH_MVC'] = __DIR__ . $config_file; //framework config
        $config_files ['APP'] = $this->_app_path . $config_file; //app config

        $this->_config = new Config ( $config_files );
        $config_paths = $this->_config->get_paths ();
        $this->_autoloader = new Basic_Autoloader ( $config_paths );
    }

    /**
     * creates new base router instance which reads the url request and routes
     * application onto Action Controllers
     */
    public function run() {
        $this->_router = new Base_router ( $this->_config );
        $route = $this->_router->run ();

        if ($route) {
            $this->_dispatcher = new Base_Dispatcher ( $route );
            $this->_dispatcher->run ();
        }
    }
}

Not moved the initialization of new router to construct in order to remove protected $_config as I might use this attribute depending on how I develop the class. If not used I will remove.

$config_files ['DH_MVC'] = __DIR__ . $config_file;

It uses __DIR__ as $_framework_path is parent of DIR. $_framework_path takes into account my namespace dh_mvc2 thats used as the first directory, by the spl_autoload, from the framework basepath

namespace dh_mvc2\application;

    use dh_mvc2\classes\Config;
    use dh_mvc2\dispatchers\Base_Dispatcher;
    use dh_mvc2\routers\Base_Router;
    use dh_mvc2\autoloaders\Basic_Autoloader;

    class Application {
    protected $_config;
    protected $_autoloader;
    protected $_app_path;
    protected $_framework_path;
    protected $_router;
    protected $_dispatcher;

    /**
     *
     * @param string $app_path
     *          "path/to/application/directory";
     * @throws \DH_MVC2_Application_Exeption
     */
    public function __construct($app_path, $run = TRUE) {
        if (! is_dir ( $app_path )) {
            require_once 'exeptions/DH_MVC2_Application_Exeption.php';
            throw new \DH_MVC2_Application_Exeption ( "Must supply a path to an application directory" );
        }

        $this->_framework_path = realpath ( dirname ( __DIR__ ) );
        $this->_app_path = $app_path;
        $this->set_ini_default_paths ();
        $this->init ();
        $d = new Config ();
        if ($run) {
            $this->run ();
        }
    }

    /**
     * add the framework dir and application dir to include path
     */
    private function set_ini_default_paths() {
        spl_autoload_register ();
        $paths = explode ( PATH_SEPARATOR, get_include_path () );
        array_push ( $paths, $this->_app_path );
        array_push ( $paths, $this->_framework_path );
        $paths = array_unique ( $paths );
        set_include_path ( implode ( PATH_SEPARATOR, $paths ) );
    }

    /**
     * links to the app and framework default config files and inits them
     * registers autoloader
     */
    private function init() {
        $config_file = '/config/config.ini.php';
        $config_files ['DH_MVC'] = __DIR__ . $config_file; // framework config
        $config_files ['APP'] = $this->_app_path . $config_file; // app config
        $this->_config = new Config ( $config_files );
        $config_paths = $this->_config->get_paths ();
        $this->_autoloader = new Basic_Autoloader ( $config_paths );
    }

    /**
     * creates new base router instance which reads the url request and routes
     * application onto Action Controllers
     */
    public function run() {
        $this->_router = new Base_router ( $this->_config );
        $route = $this->_router->run ();

        if ($route) {
            $this->_dispatcher = new Base_Dispatcher ( $route );
            $this->_dispatcher->run ();
        } else {
            require_once 'exeptions/DH_MVC2_Router_Exeption.php';
            throw new \DH_MVC2_Router_Exeption ( "Failed to discover route" );
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

this looks pretty good to me.

Some thoughts for entertainment;

  • If you created the router in init(), then _config would not be required any more as a protected property, one less private property

  • There seems to be no error handling when $this->_router->run () fails.

  • set_ini_default_paths violates DRY, but only once, have you considered just adding the paths and then calling array_unique. Not sure that is the right way to go, but it could be considered.

  • _autoloader is nowhere used, and it is private. Seems pointless?

  • Should $config_files ['DH_MVC'] = __DIR__ . $config_file; not use $this->_framework_path ?

\$\endgroup\$
  • \$\begingroup\$ Thanks Tom. The autoloader class registers an spl_autoload_register callback function that uses the conifg paths passed to its constructor. The call back function is one of the autoloaders own methods. Hopefully allowing me to inherit and override the autoload callback for other 'types' of autoloaders. If that all made sense? \$\endgroup\$ – Dave Hale Mar 19 '13 at 18:20
  • \$\begingroup\$ your other points will be implemented now though. Well after looking at that array function to try fix the DRY issue \$\endgroup\$ – Dave Hale Mar 19 '13 at 18:22
1
\$\begingroup\$

I have some criticism on details you didn't ask, but which should be addressed:

Autoloading

Make use of it! There is no need to make manual calls to require_once().

There even is a standard: PSR-0, which defines how you should structure your class and namespace names together with the directory and file structure. When I look at your code, I see some issues: Because the underscore was used as a separator before PHP got namespaces, it is considered a separation character in class names.

Namespaces

Make use of it! I cannot see a reason why the exception \DH_MVC2_Application_Exeption is located in the root namespace, and not called \dh_mvc2\application\Exception. It would be so much easier to simply throw new Exception(); inside the "\dh_mvc2\application" namespace.

Whitespace and coding style

Endless debates might occur, but I really don't like yours. Especially the inconsistent placement of parentheses. Personally, I'd rather prefer not to use that much spaces, but if you really have to, use them everywhere. For example, if you want to find the function "set_ini_default_paths", and want to make sure not to find "set_ini_default_paths_directory", you'd search for "set_ini_default_paths(" - which will find only function definitions, but not usage. To find these, you'd have to search "set_ini_default_paths (".

Dependency injection

Doesn't take place. Objects are created inside your class. There is no way I would be able to change for example the Config object if I'd use your class, I must use yours. I even cannot change the config filename!

Include path

Be careful what you add here. If a PSR-0 autoloader is used, there is no need to add anything to the include path. In fact, you'll get a good amount of performance if you include as few directories as possible, preferably only ".", to be able to include files with a relative path.

\$\endgroup\$
  • \$\begingroup\$ Thanks Sven. I'm kind of confused with the DI thing people keep mentioning. \$\endgroup\$ – Dave Hale Mar 21 '13 at 10:22
  • \$\begingroup\$ Zend framework for example (ignoring the code) needs its config and application configs defined a certain way right? ok admittedly you can define the config zend uses in one of several ways but still define to their framework standard? And also i may be mistaken but Zend also requires the config to be in a set place within dir structure and without checking, named to a set convention also? \$\endgroup\$ – Dave Hale Mar 21 '13 at 10:25
  • \$\begingroup\$ Your comments about Namespaces and auto-loading though I understand. And currently trying to get both to work correctly without require/includes. My class would still be called DH_MVC2_Application_Exeption but located in the dir dh_mvc2\application\Exception right? \$\endgroup\$ – Dave Hale Mar 21 '13 at 10:33
  • \$\begingroup\$ Looking at zend again, with respect to configs. You can specify the path to config file, and can have one of several formats. So maybe i should allow for my config to be located anywhere. But i'm still confused as to why requiring-'coupling' my application class to use a 'known' config (and autoloader) is wrong. Am i missing something big here? Or could i write a config file/autoloader how i wanted and throw it at zend and other frameworks and they would happily use them? \$\endgroup\$ – Dave Hale Mar 21 '13 at 11:25
  • \$\begingroup\$ I think i figured out why I was struggling with auto-loading thanks to your link to the PSR-0. DH_MVC2_Application_Exception would work if my directory was DH/MVC2/Application/Exception but my root dir was DH_MVC2 which auto-loader resolves as DH/MVC2 (if i understand it right?). Thank you for that \$\endgroup\$ – Dave Hale Mar 21 '13 at 12:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.