8
\$\begingroup\$

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 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:

<?php
/**
 * Description of Bootstrap
 *
 * @author nlubin
 */
Class Bootstrap {

    /**
     *
     * @var SessionManager
     */
    private $_session;
    /**
     *
     * @var stdClass
     */
    private $_path;
    /**
     *
     * @var AppController
     */
    private $_controller;
    /**
     *
     * @var Template
     */
    private $_template;

    function __construct() {
        Config::set('page_title', Config::get('DEFAULT_TITLE'));
        Config::set('template', Config::get('DEFAULT_TEMPLATE'));
        $this->_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->_path->page);

        //LOAD CONTROLLER
        $modFolders = array('images', 'js', 'css');

        //load controller
        if(strlen($this->_path->controller) == 0) $this->_path->controller = Config::get('DEFAULT_CONTROLLER');

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

    /**
     * @return AppController
     */
    private function create_controller($controllerName) {
        if (class_exists($controllerName) && is_subclass_of($controllerName, 'AppController')) {  
            $app_controller  = new $controllerName($this->_session); 
        } else {
            //show nothing 
            header("HTTP/1.1 404 Not Found");
            exit;
        }
        return $app_controller;
    }

    /**
     *
     * @param array $path_info 
     */
    private function rewrite($path_info){
        $rewrite = $path_info[count($path_info) - 2];
        $file_name = $path_info[count($path_info) - 1];

        $file = DOCROOT."webroot/".$rewrite."/".$file_name;
        include DOCROOT.'/smallFry/functions/mime_type.php'; // needed for setups without `mime_content_type`
        header('Content-type: '.mime_content_type($file));
        readfile($file);

        exit;
    }
}

AppController.php:

<?php
/**
 * Description of AppController
 *
 * @author nlubin
 */
class AppController {

    private $pageOn;
    protected $name = __CLASS__;
    protected $helpers = array();
    protected $validate = array();
    protected $posts = array();
    protected $session;
    protected $validator;
    protected $template;

    /**
     *
     * @param SessionManager $SESSION
     */
    public function __construct(SessionManager $SESSION) {

        $this->pageOn = Config::get('page');
        $this->session = $SESSION;
        $model_name = $this->name;
        if(class_exists($model_name) && is_subclass_of($model_name, 'AppModel')){
            /**
             * @var AppModel $model_name
             */
            $this->$model_name = new $model_name();
        }
        else {
            //default model (no database table chosen)
            $this->$model_name = new AppModel();
        }
        /* Get all posts */
        $this->posts = $this->$model_name->getPosts();

        Config::set('view', strtolower($model_name));

        if(!$this->session->get(strtolower($model_name))){
            $this->session->set(strtolower($model_name), array());
        }

    }

    private function getPublicMethods(){
        $methods = array();
        $r = new ReflectionObject($this);
        $r_methods = $r->getMethods(ReflectionMethod::IS_PUBLIC);
        foreach($r_methods as $method){
            if($method->class !== 'AppController'){ //get only public methods from extended class
                $methods[] = $method->name;
            }
        }
        return $methods;
    }

    /**
     *
     * @param Template $TEMPLATE 
     */
    public function setTemplate(Template $TEMPLATE){
        $this->template = $TEMPLATE;
        $model_name = $this->name;
        $this->setHelpers();
    }

    /**
     * Function to run before the constructor's view function
     */
    public function init(){} //function to run right after constructor

    /**
     * Show the current page in the browser
     *
     * @param array $args
     * @return string 
     */
    public function displayPage($args)  {
        Config::set('method', $this->pageOn);
        $public_methods = $this->getPublicMethods();
        if(in_array($this->pageOn, $public_methods))    {  
            call_user_func_array(array($this, $this->pageOn), $args);
        }
        else    {
            if(Config::get('view') == strtolower(__CLASS__) || 
                    !in_array($this->pageOn, $public_methods)){
                header("HTTP/1.1 404 Not Found");
            }
            else {
                Config::set('method', '../missingfunction'); //don't even allow trying the page
                return($this->getErrorPage(Config::get('view')."/{$this->pageOn} does not exist."));
            }
            exit;
        }
    }

    /**
     *
     * @return string 
     */
    function index() {}

    /**
     *
     * @param string $msg
     * @return string 
     */
    protected function getErrorPage($msg = null)    {
        $err = '<span class="error">%s</span>';
        return sprintf($err, $msg);
    }

    protected function setHelpers(){
        $helpers = array();
        foreach($this->helpers as $helper){
            $help = "{$helper}Helper";
            if(class_exists($help) && is_subclass_of($help, 'Helper')){
                $this->$helper = new $help();
                $helpers[$helper] = $this->$helper;
            }
        }
        $this->template->set('helpers', (object) $helpers);
    }

    protected function logout(){
        session_destroy();
        header('Location: '.WEBROOT.'index.php');
        exit;
    }

    /**
     *
     * @param array $validate
     * @param array $values
     * @param boolean $exit
     * @return boolean 
     */
    protected function validateForm($validate = null, $values = null, $exit = true){

        $this->validator = new FormValidator(); //create new validator

        if($validate == null){
            $validate = $this->validate;
        }

        foreach($validate as $field => $rules){
            foreach($rules as $validate=>$message){
                $this->validator->addValidation($field, $validate, $message);
            }
        }

        return $this->doValidate($values, $exit);
    }

    protected function doValidate($values = null, $exit = true){
        if(!(!isset($_POST) || count($_POST) == 0)){
            //some form was submitted
            if(!$this->validator->ValidateForm($values)){
                $error = '';
                $error_hash = $this->validator->GetErrors();
                foreach($error_hash as $inpname => $inp_err)
                {
                  $error .= $inp_err.PHP_EOL;
                }
                return $this->makeError($error, $exit);                
            }
        }
        return true;
    }

    protected function makeError($str, $exit = true){
        $return = $this->getErrorPage(nl2br($str));
        if($exit) exit($return);
        return $return;
    }
}
\$\endgroup\$
  • \$\begingroup\$ @ZaphodBeeblebrox The Config is just a holder of global Configuration variables. I do not feel the need to include it here since it is not really important. I did not include all of my code in the OP because it would make it waaaay too long. but I want most (to all) of the classes in the smallFry/application directory to be looked at (if possible) \$\endgroup\$ – Naftali aka Neal Jan 13 '12 at 14:38
  • \$\begingroup\$ @ZaphodBeeblebrox here is the Config code if you want to see it. \$\endgroup\$ – Naftali aka Neal Jan 13 '12 at 14:39
  • \$\begingroup\$ @ZaphodBeeblebrox hehe. What is there to criticize about it? :-P Yes, I know it is basically a global class, but it detached it from anything meaningful (as it was in the previous question) \$\endgroup\$ – Naftali aka Neal Jan 13 '12 at 14:42
  • \$\begingroup\$ @ZaphodBeeblebrox haha the only reason I even added a snippet here is because of a comment on my last post. Anyways -- thank you for the help! :-D \$\endgroup\$ – Naftali aka Neal Jan 13 '12 at 14:43
4
\$\begingroup\$

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 Config class. I get it that static is heplful, but you are abusing it a bit. You could:

  1. 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.

  2. 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):

  1. PHPUnit is your best friend! Learn how to write unit tests, framework code especially should be 100% covered by unit tests.
  2. PHP has support for interfaces and you should always program to an interface.

I honestly can't find anything else worth mentioning.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I had a lot of my parameters prefixed with an underscore. The PHP chatroom suggested that that was an old tactic... :-( I do not rly want to use ini files because I want the users to be able to change the Config easily on the fly.. How would a singleton help? \$\endgroup\$ – Naftali aka Neal Jan 17 '12 at 15:13
  • \$\begingroup\$ @Neal It would help in the sense that once initialized, you could treat it as a regular object. It could follow an interface, for example. It's a bit more object oriented, if you will, what you have now is procedural code masquerading as a class. \$\endgroup\$ – yannis Jan 17 '12 at 15:15
  • \$\begingroup\$ Hmmmmm Could you give some example code? \$\endgroup\$ – Naftali aka Neal Jan 17 '12 at 15:19
  • \$\begingroup\$ The manual link under singleton has a very good example of a Singleton... \$\endgroup\$ – yannis Jan 17 '12 at 15:19
  • \$\begingroup\$ @Neal But the most important thing is that your code is good enough. You should consider unit tests and programming to an interface. Skip the singleton if it doesn't make sense, it's not a really good practice anyway - it's just a more object-y way of writing your Config class. Instead you should consider forgetting about static all together, mention "singleton" in the PHP chat room and prepare for people shouting at you :P \$\endgroup\$ – yannis Jan 17 '12 at 15:33

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.