patternphpMinor
Small PHP MVC Template
Viewed 0 times
phptemplatesmallmvc
Problem
The following is a new question based on answers from here: Small PHP Viewer/Controller template
I have written a small MVC template library that I would like some critiques on.
The library is located here
If you are looking for where to start, check out the files in the
I would really love to hear your critiques on:
I'm more interested in what I'm doing wrong than right.
Any opinions on the actual usefulness of the library are welcome.
Code examples (all of the code is equal, just on my last question I was asked for some snippets):
```
_session = new SessionManager(Config::get('APP_NAME'));
$this->_path = $this->readPath();
$this->_controller = $this->loadController();
$this->_template = new Template($this->_path, $this->_session, $this->_controller); //has destructor that controls it
$this->_controller->displayPage($this->_path->args); //run the page for the controller
$this->_template->renderTemplate(); //only render template after all is said and done
}
/**
*
* @return stdClass
*/
private function readPath(){
$path = isset($_SERVER["PATH_INFO"])?$_SERVER["PATH_INFO"]:'/'.Config::get('DEFAULT_CONTROLLER');
$path_info = explode("/",$path);
$page = (isset($path_info[2]) && strlen($path_info[2]) > 0)?$path_info[2]:'index';
list($page, $temp) = explode('.', $page) + array('index', null);
$args = array_slice($path_info, 3);
$controller = $path_info[1] ?: Config::get('DEFAULT_CONTROLLER');
return (object) array(
'path_info'=>$path_info,
'page'=>$page,
'args'=>$args,
'controller'=>$controller
);
}
/**
* @return AppController
*/
private function loadController(){
Config::set('page', $this->_pat
I have written a small MVC template library that I would like some critiques on.
The library is located here
If you are looking for where to start, check out the files in the
smallFry/application directory.I would really love to hear your critiques on:
- Code quality
- Code clarity
- How to improve
- Anything else that needs clarification expansion etc
I'm more interested in what I'm doing wrong than right.
Any opinions on the actual usefulness of the library are welcome.
Code examples (all of the code is equal, just on my last question I was asked for some snippets):
Bootstrap.php:```
_session = new SessionManager(Config::get('APP_NAME'));
$this->_path = $this->readPath();
$this->_controller = $this->loadController();
$this->_template = new Template($this->_path, $this->_session, $this->_controller); //has destructor that controls it
$this->_controller->displayPage($this->_path->args); //run the page for the controller
$this->_template->renderTemplate(); //only render template after all is said and done
}
/**
*
* @return stdClass
*/
private function readPath(){
$path = isset($_SERVER["PATH_INFO"])?$_SERVER["PATH_INFO"]:'/'.Config::get('DEFAULT_CONTROLLER');
$path_info = explode("/",$path);
$page = (isset($path_info[2]) && strlen($path_info[2]) > 0)?$path_info[2]:'index';
list($page, $temp) = explode('.', $page) + array('index', null);
$args = array_slice($path_info, 3);
$controller = $path_info[1] ?: Config::get('DEFAULT_CONTROLLER');
return (object) array(
'path_info'=>$path_info,
'page'=>$page,
'args'=>$args,
'controller'=>$controller
);
}
/**
* @return AppController
*/
private function loadController(){
Config::set('page', $this->_pat
Solution
I think your code is at a point where you need to stop worrying about it. It's not perfect, but it's way past the point of anything obvious.
The one thing I really don't like is your
-
Consider turning it into a Singleton, if you are so inclined in static state. Singletons are evil, but an improvement upon your code. All
-
Consider getting the configuration files from a configuration file, and not requiring the developer to hardcode them.
The simplest way would be
And, you seem to have ignored my advice on prefixing private members with an underscore. That hurts my feelings :)
Further suggestions (that are outside the scope of a code review):
I honestly can't find anything else worth mentioning.
The one thing I really don't like is your
Config class. I get it that static is heplful, but you are abusing it a bit. You could: -
Consider turning it into a Singleton, if you are so inclined in static state. Singletons are evil, but an improvement upon your code. All
static classes are virtually un-testable (in the context of unit tests) and I can only describe them as a procedural code masquerading as an object. It's a style you don't want to adopt.-
Consider getting the configuration files from a configuration file, and not requiring the developer to hardcode them.
The simplest way would be
ini files, PHP's support via parse_ini_file() is excellent. That way your configuration class could be used autonomously. And, you seem to have ignored my advice on prefixing private members with an underscore. That hurts my feelings :)
Further suggestions (that are outside the scope of a code review):
- PHPUnit is your best friend! Learn how to write unit tests, framework code especially should be 100% covered by unit tests.
- PHP has support for interfaces and you should always program to an interface.
I honestly can't find anything else worth mentioning.
Context
StackExchange Code Review Q#7531, answer score: 4
Revisions (0)
No revisions yet.