patternphpMinor
Small MVC for PHP
Viewed 0 times
forphpsmallmvc
Problem
The following is a new question based on answers from here: Small PHP MVC 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.
The README is not complete and there is a lot more functionality than what is documented, if you have any questions, please ask me.
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):
```
CONFIG = $CONFIG;
$this->CONFIG->set('page_title', $this->CONFIG->get('DEFAULT_TITLE'));
$this->CONFIG->set('template', $this->CONFIG->get('DEFAULT_TEMPLATE'));
$this->_session = new SessionManager($this->CONFIG->get('APP_NAME'));
$this->_path = $this->readPath();
$this->_controller = $this->loadController();
$this->_template = new Template($this->_path, $this->_session, $this->_controller, $this->CONFIG); //has destructor that controls it
$this->_controller->displayPage($this->_path->args); //run the page for the controller
$this->_template->renderTemplate($this->_path->args); //only render template after all is said and done
}
/**
* @return \stdClass
*/
private function readPath(){
$default_controller = $this->CONFIG->get('DEFAULT_CONTROLLER');
$index = str_replace("/", "", \SmallFry\Config\INDEX);
//use strtok to remove the query string from the end fo the request
$request_uri = !isset($_SERVER["REQUEST_URI"]) ? "/" : strtok($_SERVER["REQUEST_URI"],'?');;
$request_uri = str_replace("/" . $index , "/", $request_uri);
$path = $re
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
lib 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.
The README is not complete and there is a lot more functionality than what is documented, if you have any questions, please ask me.
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:```
CONFIG = $CONFIG;
$this->CONFIG->set('page_title', $this->CONFIG->get('DEFAULT_TITLE'));
$this->CONFIG->set('template', $this->CONFIG->get('DEFAULT_TEMPLATE'));
$this->_session = new SessionManager($this->CONFIG->get('APP_NAME'));
$this->_path = $this->readPath();
$this->_controller = $this->loadController();
$this->_template = new Template($this->_path, $this->_session, $this->_controller, $this->CONFIG); //has destructor that controls it
$this->_controller->displayPage($this->_path->args); //run the page for the controller
$this->_template->renderTemplate($this->_path->args); //only render template after all is said and done
}
/**
* @return \stdClass
*/
private function readPath(){
$default_controller = $this->CONFIG->get('DEFAULT_CONTROLLER');
$index = str_replace("/", "", \SmallFry\Config\INDEX);
//use strtok to remove the query string from the end fo the request
$request_uri = !isset($_SERVER["REQUEST_URI"]) ? "/" : strtok($_SERVER["REQUEST_URI"],'?');;
$request_uri = str_replace("/" . $index , "/", $request_uri);
$path = $re
Solution
A few things:
Firstly, It's not MVC. In MVC, controllers do not feed the view data. This is a common misconception but the architecture you're using here is closer to PAC. See http://www.garfieldtech.com/blog/mvc-vs-pac and http://r.je/views-are-not-templates.html for more info (Full disclosure: I wrote the second article.)
Secondly, your models aren't models they're simple data access and barely even that. They should at least abstract SQL away. You have domain logic, presentation logic and application logic all in the controller. In MVC the model should store application state (and have access to domain state), the controller should respond to user actions and the view should fetch the data from the model. See http://blog.astrumfutura.com/2008/12/the-m-in-mvc-why-models-are-misunderstood-and-unappreciated/ for a description of the model in MVC.
Thirdly, forcing models, views and controllers to extend from specific base classes heavily limits flexibility. Your controllers, views and models should implement interfaces instead of extending base classes. Although from experience, I have found that controllers and models don't even need to do that.
Finally, you should always favour Dependency injection rather than constructing objects arbitrarily throughout the code. See http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ (and the rest of his very excellent site) for more information.
Sorry to sound very critical, but it's important to get the fundamental architecture correct before asking people to critique code clarity and quality.
Firstly, It's not MVC. In MVC, controllers do not feed the view data. This is a common misconception but the architecture you're using here is closer to PAC. See http://www.garfieldtech.com/blog/mvc-vs-pac and http://r.je/views-are-not-templates.html for more info (Full disclosure: I wrote the second article.)
Secondly, your models aren't models they're simple data access and barely even that. They should at least abstract SQL away. You have domain logic, presentation logic and application logic all in the controller. In MVC the model should store application state (and have access to domain state), the controller should respond to user actions and the view should fetch the data from the model. See http://blog.astrumfutura.com/2008/12/the-m-in-mvc-why-models-are-misunderstood-and-unappreciated/ for a description of the model in MVC.
Thirdly, forcing models, views and controllers to extend from specific base classes heavily limits flexibility. Your controllers, views and models should implement interfaces instead of extending base classes. Although from experience, I have found that controllers and models don't even need to do that.
Finally, you should always favour Dependency injection rather than constructing objects arbitrarily throughout the code. See http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ (and the rest of his very excellent site) for more information.
Sorry to sound very critical, but it's important to get the fundamental architecture correct before asking people to critique code clarity and quality.
Context
StackExchange Code Review Q#19592, answer score: 8
Revisions (0)
No revisions yet.