1

コントロールアーキテクチャスクリプト(以下に含まれています)に関するフィードバックを探しています。具体的には、スクリプトのデザイン、構成、コメント、およびフォーマットに関するフィードバックを探しています。私は趣味としてPHPプログラミングを楽しんでおり、コードをどこで改善できるかを学びたいと思っています。

前もって感謝します!

class FrontController extends ActionController {

//Declaring variable(s)
private static $instance;
protected $controller;

//Class construct method
public function __construct() {}

//Starts new instance of this class with a singleton pattern
public static function getInstance() {
    if(!self::$instance) {
        self::$instance = new self();
    }
    return self::$instance;
}

public function dispatch($throwExceptions = false) {

    /* Checks for the GET variables $module and $action, and, if present,
     * strips them down with a regular expression function with a white
     * list of allowed characters, removing anything that is not a letter,
     * number, underscore or hyphen.
     */
    $regex  = '/[^-_A-z0-9]+/';
    $module = isset($_GET['module']) ? preg_replace($regex, '', $_GET['module']) : 'home';
    $action = isset($_GET['action']) ? preg_replace($regex, '', $_GET['action']) : 'frontpage';

    /* Generates Actions class filename (example: HomeActions) and path to
     * that class (example: home/HomeActions.php), checks if $file is a
     * valid file, and then, if so, requires that file.
     */
    $class = ucfirst($module) . 'Actions';
    $file  = $this->pageDir . '/' . $module . '/' . $class . '.php';

    try {

        //Checks for existance of file
        if (!is_file($file)) {
            throw new Exception('File not found!');
        }

        //Includes file
        require_once $file;

        /* Creates a new instance of the Actions class (example: $controller
         * = new HomeActions();), and passes the registry variable to the
         * ActionController class.
         */
        $controller = new $class();
        $controller->setRegistry($this->registry);

        //Trys the setModule method in the ActionController class
        $controller->setModule($module);

        /* The ActionController dispatchAction method checks if the method
         * exists, then runs the displayView function in the
         * ActionController class.
         */    
        $controller->dispatchAction($action);

    } catch(Exception $error) {

        /* An exception has occurred, and will be displayed if
         * $throwExceptions is set to true.
         */
        if($throwExceptions) {
            echo $error;
        }
    }
}
}

abstract class ActionController {

//Declaring variable(s)
protected $registry;
protected $module;
protected $registryItems = array();

//Class construct method
public function __construct(){}

public function setRegistry($registry) {

    //Sets the registry object
    $this->registry = $registry;

    /* Once the registry is loaded, the controller root directory path is
     * set from the registry.  This path is needed for the controller
     * classes to work properly.
     */
    $this->setPageDir();
}

//Sets the controller root directory from the value stored in the registry
public function setPageDir() {
    $this->pageDir = $this->registry->get('pageDir');
}

//Sets the module
public function setModule($module) {
    $this->module = $module;
}

//Gets the module
public function getModule() {
    return $this->module;
}

/* Checks for actionMethod in the Actions class (example: doFrontpage()
 * within home/HomeActions.php) with the method_exists function and, if
 * present, the actionMethod and displayView functions are executed.
 */  
public function dispatchAction($action) {
    $actionMethod = 'do' . ucfirst($action);
    if (!method_exists($this, $actionMethod)) {
        throw new Exception('Action not found!');
    }
    $this->$actionMethod();
    $this->displayView($action);
}

public function displayView($action) {
    if (!is_file($this->pageDir . '/' . $this->getModule() . '/' . $action . 'View.php')) {
        throw new Exception('View not found!');
    }

    //Sets $this->actionView to the path of the action View file
    $this->actionView = $this->pageDir . '/' . $this->getModule() . '/' . $action . 'View.php';

    //Sets path of the action View file into the registry
    $this->registry->set('actionView', $this->actionView);

    //Includes template file within which the action View file is included
    require_once $this->pageDir . '/default.tpl';
}
}

class Registry {

//Declaring variables
private $store;

//Class constructor
public function __construct() {}

//Sets registry variable
public function set($label, $object) {
    $this->store[$label] = $object;
}

//Gets registry variable    
public function get($label) {
    if(isset($this->store[$label])) {
        return $this->store[$label];
    } else {
        return false;          
    }
}

//Adds outside array of registry values to $this->store array
public function addRegistryArray($registryItems) {
    foreach ($registryItems as $key => $value) {
        $this->set($key, $value);
    }
}

//Returns registry array
public function getRegistryArray() {
    return $this->store;
}
}

4

3 に答える 3

1

コードを詳しく調べずに:

意味のある関数名と変数名を使用して、わかりやすいコードを記述してみてください。コードの目的や機能が明確でない場合にのみコメントを使用してください。例えば

//Declaring variable(s)
//Class construct method
//Checks for existance of file
//Includes file

コード自体はすでに十分に明確であるため、役に立たないコメントです。

読む価値のある本:クリーンコード

于 2010-09-24T14:33:50.937 に答える
1

ローカライズされすぎているためにクローズ投票を行うことと、コードにコメントしたいこととの間で私は引き裂かれています。また、コードが多すぎて今は通り抜けられないので、いくつかコメントします。

1)ドキュメントスタイル

PHPDocのような確立されたドキュメント形式を使用してみませんか?

2)フォーマット

私が見る限り一貫性がありますが、自分で行うのではなく、 PEARやZF(PEARに基づく)のような広く使用されているコーディング規約を使用することをお勧めします(とにかくあなたはPEARに近いので)あなたはそれを完全に採用したほうがよいでしょう)。

3)シングルトンパターン

シングルトンが機能するためには、プライベート__contructおよび__cloneメソッドが必要です。しかし、私はそれをまったく使用しないことをお勧めします。多くの人々は、シングルトンがアンチパターンであると信じています。シングルトンパターンの欠点を議論するSOに関する質問がたくさんあるので、周りを見てください。インスタンスが1つしかない場合は、2番目のインスタンスをインスタンス化しないでください。他のクラスのFrontControllerにアクセスする必要がある場合は、それを挿入します。

4)メソッドの長さ

dispatch私はおそらくメソッドを短くしようとします。基本的に、メソッドが何をするかを説明し、それを使用する必要がある場合、その部分はそれ自体のメソッドに入る必要があります。メソッドを小さな個別のユニットにしようとします。これにより、UnitTestingも簡単になります。

5)関心の分離

FrontControllerがActionControllerから拡張されている理由がわかりません。それを拡張するクラスは他にありませんが、FrontControllerがインスタンス化するクラスもActionControllerのサブクラスであると思います。しかし、FrontControllerはコントローラーと名付けられていますが、 PageControllersとは異なることを行うので、おそらくそれらを分離したままにしておきます。

On a sidenote, if you are interested in increasing the quality of your code, have a look at the slides and tools given at http://phpqatools.org/

于 2010-09-24T14:44:55.560 に答える
0

あなたのコードはCakePhpを非常に思い出さます。私はそれをチェックすることをお勧めします、それはApp :: import()とファイルシステムをラップするFileクラスでこのすべての仕事をします。さまざまなバージョンとOSでコミュニティによってテストされており、実際に時間を節約できます。

于 2010-09-24T14:44:37.277 に答える