HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Small PHP framework template library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
templatephpsmalllibraryframework

Problem

The following is a new question based on answers from here: Small MVC for PHP

I have written a small template library that I would like some critiques on, 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, and anything else that needs clarification expansion.

I'm more interested in what I'm doing wrong than right. The README is incomplete and there is a lot more functionality than what is documented.

Code examples (these might not have changed much since last year, but I was asked to add some code, so I am including the same two files as on my previous post):

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->CONFIG);
$this->_template->setController($this->_controller);
$this->_controller->setPage($this->_path->page);
$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
}
catch (\Exception $e) {
header("HTTP/1.1 404 Not Found");
exit;
}
}

/**
* @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($_S

Solution

You have asked for critiques, so here it goes.

  • A readability remark:


The comments for private variables in Bootstrap take too much space yet tell very little to unfamiliar reader. Maybe give more descriptive names to your variables and drop comments or replace them by more details. Also one line per comment. Quoting Uncle Bob's fantastic book:


Explain yourself in code

-
__construct is generally recommended to be small, yours seems to be doing too much.

-
It is generally considered bad practice to have new Object inside your constructor. This makes it very hard to test and makes your code tightly coupled with dependencies. A better way is to make them explicit dependencies (Dependency Injection):

function __construct(Config $CONFIG, SessionManager $SManager, Template $Template)


-
Your function loadController is huge! Quoting Uncle Bob:


The first rule of functions is that they should be small. The second rule is that they should be smaller than that.

-
I can see many hard-coded strings inside your functions, you may want to put them separately in a dedicated config class.

-
Writing style: You are using $app_controller and $modelName. Maybe keep the same convention - either always underscore of camel-case.

-
It is a matter of personal preference, but I like to start private/protected function with underscore, to easily distinguish them from public ones.

Code Snippets

function __construct(Config $CONFIG, SessionManager $SManager, Template $Template)

Context

StackExchange Code Review Q#30304, answer score: 7

Revisions (0)

No revisions yet.