Small PHP Viewer/Controller template

I have written a small template Viewer/Controller esque library that I would like some critiques on.

The library is located here

If you are looking for where to start, check out App.php and AppController.php in the classes folder

I would really love to hear your critiques on:

• Code quality

• Code clarity

• How to improve

• Anything else that needs clarification expansion etc

• I have also heard that my static methods (like my get and set methods in App) should be replaced with something more dynamic, How would I go about doing that?

I'm more interested in what I'm doing wrong than right.

Any opinions on the actual usefulness of the library are welcome.

App.php:

```
_connection = Database::getConnection();
}

private function render_template(){
$rPath = $this->read_path();
foreach($rPath as $key=>$value){
$$key = $value;
}
unset($rPath);

ob_start();

App::set('page_title',App::get('DEFAULT_TITLE'));
App::set('template',App::get('DEFAULT_TEMPLATE'));
App::set('page',$page);

//LOGIN
if(!isset($_SESSION['LOGIN']) || $_SESSION['LOGIN'] == false){
Login::check_login();
}
else {
$modFolders = array('images', 'js', 'css');

//load controller
if(strlen($controller) == 0) $controller = App::get('DEFAULT_CONTROLLER');

if(count(array_intersect($path_info, $modFolders)) == 0){ //load it only if it is not in one of those folders
$controllerName = "{$controller}Controller";
$app_controller = $this->create_controller($controllerName, $args);
}
else { //fake mod-rewrite
$this->rewrite($path_info);
}
}

$main = ob_get_clean();
App::set('main', $main);
//LOAD VIEW
ob_start();
$this->load_view($app_controller, 0);
//END LOAD VIEW

//LO

I've spent about 20 minutes reading your code and I've identified several issues.

Relative paths

private function load_template($controllerName, $jQuery = null){
    $page_title = $controllerName->get('title') ? $controllerName->get('title') : App::get('DEFAULT_TITLE');

    //display output
    $cwd = dirname(__FILE__);
    $template_file = $cwd.'/../view/'.App::get('template').'.stp';
    if(is_file($template_file)){
        include $template_file;
    }
    else {
        include $cwd.'/../view/missingfile.stp'; //no such file error
    }
}

What happens if at a later point you decide to change your directory structure? Instead of using dirname(__FILE__) you could just pass that base directory as a function parameter.

Function names inconsistencies

You prefix some private / protected variables and functions with an underscore:

protected $_mysql;

Two points:

• Either do it for every private / protected variable and function or don't do it at all

• Don't do it at all

Some justification on the second point:

• Every decent IDE out there allows for sorting of variables and functions based on their visibillity / scope. Eclipse goes a step further and uses green for public, and red for private / public in the Outline view, so everything is easily identifiable.

• In PHP functions that are prefixed with an underscore might be mistaken for magic methods (which are prefixed by two underscores).

Note that while almost all php IDEs will treat:

function foo()

as:

public function foo()

you could help the IDE by adding the public keyword. That's also a readibillity point and a PHP4 style (function foo()) vs PHP5 style (public function foo()).

Inline HTML

protected function error_page($msg = null)    {
    $err = '%s';
    return sprintf($err, $msg);
}

protected function include_jQuery(){
    $ret = ''.PHP_EOL;
    $ret .= '        '.PHP_EOL;
    return $ret;
}

function __get($var_name){
    // echo $var_name."";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?>\n";
        ?><?php
        exit;
    }
}

Don't do that, you must separate presentation from logic.

Sessions

In AppController.php you utilize sessions:

if(!isset($_SESSION[App::get('APP_NAME')][strtolower($this->name)])){
    $_SESSION[App::get('APP_NAME')][strtolower($this->name)] = array();
}

But you don't have session_start() at the beginning of the script. Granted, you probably start the session in the caller script, but that means that your class will throw notices ($_SESSION array not initialized) if called from anywhere where session_start() hasn't been called. You can do something like:

if( !isset($_SESSION) ) session_start();

at the top of each script that uses sessions.

Comments

You have some phpDoc comments, you need one for every function. But please note that I've been accused of overcommenting :)

PHP4 style class members

You are using some PHP4 style class members:

var $name = __CLASS__;
var $helpers = array();
var $validate = array();
var $posts = array();

You should be using PHP5 access control (public / private / protected) members everywhere.

Ternary operator readability

This:

return isset(self::$app_vars[$v])?self::$app_vars[$v]:false;

is not very readable. Consider something like:

return
    isset(self::$app_vars[$v])
    ? self::$app_vars[$v]
    : false;

or at least something like this:

return isset(self::$app_vars[$v]) ? self::$app_vars[$v] : false;

Similarly, please add spaces before and after concatenation points:

$cwd.'/../view/'.App::get('template').'.stp'; // somewhat unreadable
$cwd . '/../view/'.App::get('template') . '.stp'; // friendlier to the eyes

Minor stuff

private function load_template($controllerName, $jQuery = null) {}

You don't utilize the $jQuery variable in the function. Is your code complete? If not, you shouldn't have posted it up for review.

function __get($var_name){
    // echo $var_name."";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?>\n";
        ?><?php
        exit;
    }
}

else is not needed (as if block returns, which means that execution of the function stops there):

function __get($var_name){
    // echo $var_name."";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }

    ?>\n";
    ?><?php
    exit;        
}

Here:

private function create_controller($controllerName, $args = array()){
    if (class_exists($controllerName)) {

You treat $controllerName as a parameter that holds a class name, but in the very next function:

private function load_template($controllerName, $jQuery = null){
    $page_title = $controllerName->get('title')?$controllerName->get('title'):App::get('DEFAULT_TITLE');

you also have a $controllerName parameter that holds a

StackExchange Code Review Q#5039, answer score: 19