我编写了一个小的Viewer / Controller模板式esque库,希望对其进行评论。

该库位于此处

如果您正在寻找从哪里开始,请检查App.php文件夹中的AppController.phpclasses

我真的很想听听您对以下问题的批评:


代码质量
代码清晰度
如何改进
需要澄清扩展等的其他内容
我也听说我的静态方法(例如get中的setApp方法)应替换为更具动态性的东西,我该怎么做那是什么?

我对做错的事情比对的更感兴趣。

欢迎对库的实际用途发表任何看法。

App.php:

<?php
/**
 * Description of App
 *
 * @author nlubin
 */
class App {
    /**
     * Holds all of the app variables
     * @var array
     */
    private static $app_vars = array();
    /**
     * Will be an App object
     * @var App
     */
    private static $app = null;

    /**
     * Get a single app_vars variable
     * @param string $v
     * @return mixed 
     */
    public static function get($v){
        return isset(self::$app_vars[$v])?self::$app_vars[$v]:false;
    }

    /**
     * Get all app_vars variables
     * @return array app_vars
     */
    public static function getAll(){
        return self::$app_vars;
    }

    /**
     * Set an app_vars variable
     * 
     * @param string $v
     * @param mixed $va
     * @return mixed
     */
    public static function set($v, $va){
        if(self::$app == null){ //create App on first set. if not, the app does not exist
            self::$app = new self();
        }
        return self::$app_vars[$v] = $va;
    }

    /**
     * Clean up the app_vars variable
     */
    public static function clean(){
        self::$app_vars = array();
    }

    public function __construct() {
        $this->_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

        //LOAD TEMPLATE
        $main = ob_get_clean();
        App::set('main', $main);
        $this->load_template($app_controller, $app_controller->get('jQuery'));
        //END LOAD TEMPLATE
    }


    private function read_path(){
        $path = isset($_SERVER["PATH_INFO"])?$_SERVER["PATH_INFO"]:'/'.App::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 = isset($path_info[1])?$path_info[1]:App::get('DEFAULT_CONTROLLER');
        return array(
            'path_info'=>$path_info,
            'page'=>$page,
            'args'=>$args,
            'controller'=>$controller
        );
    }

    private function create_controller($controllerName, $args = array()){
        if (class_exists($controllerName)) {  
            $app_controller  = new $controllerName(); 
        } else {
            //show nothing 
            header("HTTP/1.1 404 Not Found");
            exit;
        }
        echo $app_controller->display_page($args);
        return $app_controller;
    }

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

    private function load_view($controllerName, $saveIndex){

        //Bring the variables to the global scope
        $vars = $controllerName->getAll();
        foreach($vars as $key=>$variable){
            $$key = $variable;
        }
        $cwd = dirname(__FILE__);
        if(App::get('view')){
            $template_file = $cwd.'/../view/'.App::get('view').'/'.App::get('method').'.stp';
            if(is_file($template_file)){
                include $template_file;
            }
            else {
                include $cwd.'/../view/missingview.stp'; //no such view error
            }
        }
        else {
            App::set('template', 'blank');
            include $cwd.'/../view/missingfunction.stp'; //no such function error
        }
    }


    private function rewrite($path_info){
        $rewrite = $path_info[count($path_info) - 2];
        $file_name = $path_info[count($path_info) - 1];

        $file = WEBROOT.$rewrite."/".$file_name;
//                echo $file; 
        header('Location: '.$file);
        exit;
    }

    public function __destruct() {
        $this->render_template();
    }
}

?>


AppController .php

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

    /**
     *
     * @var mySQL
     */
    protected $_mysql;
    protected $_page_on,
            $_allowed_pages = array(),
            $_not_allowed_pages = array(
                '__construct', 'get', 'set', 
                'getAll', 'display_page', 'error_page',
                'include_jQuery', 'include_js', '_setHelpers',
                '_validate_posts', '_doValidate', '_make_error'
            );
    protected $app_vars = array();
    var $name = __CLASS__;
    var $helpers = array();
    var $validate = array();
    var $posts = array();
    protected $validator;

    public function __construct()   {
        $this->_mysql = Database::getConnection();
        $this->_page_on = App::get('page');
        App::set('view', strtolower($this->name));
        $this->_allowed_pages = get_class_methods($this);
        $this->set('jQuery', $this->include_jQuery());
        $this->setHelpers();
        $this->validator = new FormValidator();
        $this->_validate_posts();
        $this->posts = (object) $this->posts;
        if(!isset($_SESSION[App::get('APP_NAME')][strtolower($this->name)])){
            $_SESSION[App::get('APP_NAME')][strtolower($this->name)] = array();
        }
        return;
    }

    public function init(){

    }

    public function get($v){
        return isset($this->app_vars[$v])?$this->app_vars[$v]:false;
    }

    protected function set($v, $va){
        return $this->app_vars[$v] = $va;
    }

    public function getAll(){
        return $this->app_vars;
    }
    /**
     * Show the current page in the browser
     * @return string 
     */
    public function display_page($args)  {
        App::set('method', $this->_page_on);
        $private_fn = (strpos($this->_page_on, '__') === 0);
        if(in_array($this->_page_on, $this->_allowed_pages) 
                && !in_array($this->_page_on, $this->_not_allowed_pages)
                        && !$private_fn)    {  
            $home = $this->include_jQuery();
            call_user_func_array(array($this, $this->_page_on), $args);
        }
        else    {
            if(App::get('view') == strtolower(__CLASS__) || $private_fn ||
                    in_array($this->_page_on, $this->_not_allowed_pages)){
                header("HTTP/1.1 404 Not Found");
            }
            else {
                App::set('method', '../missingfunction'); //don't even allow trying the page
                return($this->error_page(App::get('view')."/{$this->_page_on} does not exist."));
            }
            exit;
        }
    }

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

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

    /**
     *
     * @return string 
     */
    protected function include_jQuery(){
        $ret = '<script type="text/javascript" src="js/jquery-1.6.2.min.js"></script>'.PHP_EOL;
        $ret .= '        <script type="text/javascript" src="js/jquery-ui-1.8.9.custom.min.js"></script>'.PHP_EOL;
        return $ret;
    }

    /**
     *
     * @param string $src
     * @return string 
     */
    protected function include_js($src){
        $script = '<script type="text/javascript" src="js/%s.js"></script>'.PHP_EOL;
        return sprintf($script, $src);
    }

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

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

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

    protected function _doValidate(){
        if(!(!isset($_POST) || count($_POST) == 0)){
            //some form was submitted
            if(!$this->validator->ValidateForm()){
                $error = '';
                $error_hash = $this->validator->GetErrors();
                foreach($error_hash as $inpname => $inp_err)
                {
                  $error .= "$inp_err<br/>\n";
                }
                $this->_make_error($error);                
            }

            foreach($_POST as $key=>$post){
                $this->posts[$key] = $post;
            }
        }
    }

    function __get($var_name){
//        echo $var_name."<br>";
        if(isset($this->posts->$var_name)){
            return $this->posts->$var_name;
        }
        else{
            ?><div class="errors"><?php
               echo "$var_name is not set<br/>\n";
            ?></div><?php
            exit;
        }
    }

    function __call($name, $arguments){
        if($name == 'mysql'){
            return (strlen($this->$arguments[0])==0?"NULL":"'{$this->$arguments[0]}'");
        }
    }

    function _make_error($str){
        ?><div class="errors"><?php
        echo $str;
        ?></div><?php
        exit;
    }
}

?>


#1 楼

我已经花了大约20分钟的时间阅读您的代码,并且发现了几个问题。

相对路径

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


稍后您决定更改目录结构?代替使用dirname(__FILE__),您可以仅将该基本目录作为函数参数传递。

函数名称不一致

在某些私有/受保护的变量和函数前加下划线:

protected $_mysql;


两点:


要么对每个私有/受保护的变量和函数都这样做,要么根本不做
根本不做

第二点有些道理:


每一个像样的IDE都可以基于变量进行排序他们的可见度/范围。 Eclipse更进一步,在“大纲”视图中将绿色用于公共,将红色用于私有/公共,因此所有内容都易于识别。
在PHP函数中加下划线的前缀可能会误认为魔术方法(以两个下划线为前缀)。

请注意,尽管几乎所有的php IDE都将对待:

function foo() 


as:

public function foo()


您可以通过添加public关键字来帮助IDE。这也是一个易读性点,并且是PHP4样式(function foo())和PHP5样式(public function foo())。

内联HTML

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

protected function include_jQuery(){
    $ret = '<script type="text/javascript" src="js/jquery-1.6.2.min.js"></script>'.PHP_EOL;
    $ret .= '        <script type="text/javascript" src="js/jquery-ui-1.8.9.custom.min.js"></script>'.PHP_EOL;
    return $ret;
}

function __get($var_name){
    // echo $var_name."<br>";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?><div class="errors"><?php
           echo "$var_name is not set<br/>\n";
        ?></div><?php
        exit;
    }
}   


不这样做,必须将表示与逻辑分开。

会话

在AppController.php中,您利用了会话:

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


但是脚本开头没有session_start()。当然,您可能会在调用者脚本中启动会话,但这意味着如果从尚未调用$_SESSION的任何地方调用该类,则该类将引发通知(session_start()数组未初始化)。您可以在每个使用会话的脚本的顶部执行以下操作:

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




评论

您有一些phpDoc注释,每个功能都需要一个注释。但是请注意,我一直被指控过分评论:)

PHP4样式类成员

您正在使用一些PHP4样式类成员:

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


您应该在各处使用PHP5访问控制(公共/私有/受保护)成员。

三元运算符的可读性

此:

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


可读性很差。请考虑以下内容:

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


,或至少类似以下内容:

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


同样,请添加空格串联点之前和之后:

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


小东西

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


您不使用$jQuery变量在功能上。您的代码是否完整?如果不是,您不应该将其发布以进行审查。

function __get($var_name){
    // echo $var_name."<br>";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?><div class="errors"><?php
           echo "$var_name is not set<br/>\n";
        ?></div><?php
        exit;
    }
}


else(因为if block返回,这意味着该函数的执行在那里停止) :

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

    ?><div class="errors"><?php
       echo "$var_name is not set<br/>\n";
    ?></div><?php
    exit;        
}


这里:

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


您将$controllerName视为持有类名的参数,但是在下一个函数中:

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


您还具有一个保存对象的$controllerName参数。将第二个重命名为$controller

结论

您的代码存在许多小不一致,几个可读性问题,几个主要问题(尤其是html whaty),并且显然仍在进行中(尤其是AppController.php)。您需要尝试并至少以更一致的方式重写它。

PS。正如leo建议的那样,您应该从每个类脚本的末尾删除?>

评论


\ $ \ begingroup \ $
哇。太彻底了!看看这个相关的SO问题:stackoverflow.com/questions/7676515/…
\ $ \ endgroup \ $
– Naftali又名Neal
2011年11月9日14:01

\ $ \ begingroup \ $
我已经更新了一些图书馆^ _ ^
\ $ \ endgroup \ $
– Naftali又名Neal
2011年12月8日在21:57

\ $ \ begingroup \ $
看到我更新的代码,您认为我应该提出一个新的问题吗?
\ $ \ endgroup \ $
– Naftali又名Neal
2012年1月3日在22:53



\ $ \ begingroup \ $
我只是希望它不会作为重复项而关闭(我正在解决即将在我的更新中出现的下划线问题,大部分情况下切换到所有camelCase)
\ $ \ endgroup \ $
– Naftali又名Neal
2012年1月5日15:43

\ $ \ begingroup \ $
@Neal我不认为这是重复的,您不只是复制粘贴相同的代码,而且还做了很多修改。关于在Code Review中构成欺骗的讨论仍在进行中,所以我不能确定地说...但是无论如何都可以这样做。
\ $ \ endgroup \ $
– yannis
2012年1月5日15:46

#2 楼

您的代码看起来非常不错,并且也具有不错的PHP-Doc,但是我认为方法内的注释有点令人困惑,因为它们始终具有不同的书写风格(有时大写,有时小写,有时有时只是简单的单词等) 。),因此您应该清理它们并设置约定,以便在代码中记录这些点。

此外,您还应该对load_view中的foreach循环有点谨慎,因为在那里设置了$$key = $variable;。如果您知道要为控制器设置什么,那还不错,但是如果尝试设置$this会怎样呢?我没有尝试过,但是它实际上会产生一个错误,因此我建议您只创建一个保留变量数组,在加载视图之前将对其进行检查。在那里,您还应该添加$cwd$template_file,也许还有更多这样的vars。

我实际上在您的代码中看到的是一些封闭的PHP标记,这只是一些编码约定在文件末尾(?>)。当然,我看到许多开发人员现在正在使用该功能,但是我建议您不要使用它,您可以添加诸如/* End of File */之类的注释。问题是,如果在关闭PHP标记后有一个空格,然后尝试修改标头,它将无法正常工作,只会出现错误。

并替换您的get可以使用PHP魔术方法完成set__get($var)方法。应使用__set($var)$this->newTemplateVar = "Hello World!";,而不是像普通对象属性一样访问它们。例如。 q4312079q。

希望我的帖子对您​​有所帮助;如果没有,就说出来。

#3 楼

我同意扬尼斯。但是最后一件事。

构造函数只是应该为属性分配值。您的构造函数似乎要做的还不止这些。将其切换为接受数据库连接。请记住,静电是有害的,因为它们会使测试变得困难。

$Settings   = new FileSettings(new File('Config.php'));
$MFactory   = new ModelFactory;
$TFactory   = new TemplateFactory($Settings['UI']['Template']);

$Database   = $MFactory->createDB($Settings['DB']['Driver'], ....);
$Request    = $MFactory->createRequest($_SERVER['PHP_SELF'], $_GET, $_POST);
$View       = $TFactory->createView($Request->getView());

$Controller = new Controller($Database, $TFactory, $MFactory);
$Action     = $Request->getAction()?: 'index';

if (is_callable(array($Controller, $Action))) {
    call_user_function(array($Controller, $Action));
}

else $Controller->error($Request);