1

私はCakeとMVC全般に不慣れです。最初から良い習慣を身につけたい。良い習慣には、コントローラーをスリムに保ち、モデルをファットにすることがあります。しかし、私のような初心者にとっては、少し動くターゲットです。あるモデルから別のモデルに情報を渡す必要がある場合、そのすべてをコントローラーにダンプするだけですか? モデルで機能させてみませんか？

これは、私が整理しようとしている種類の混乱の典型的な例であるアクションです。

すべてがコントローラーにあるように見えますが、おそらく間違っています。このアクションはメンバーのリストを取得し、それをビューに送信します。ビューで、アカウントを「アクティブ化」したいメンバーにチェックマークを付けることができます。ACL はなく、単純な認証のみです。「サブ管理者」には、db フィールド client_id を使用して管理を許可されているユーザーのみが表示されるようにしています。私が使用している 2 つのモデルは、ユーザーとクライアントです。

public function activate() {
    if ($this->request->is('get')) {
        $id = $this->Auth->user('id');
        $this->User->id = $id; //make sure current User is the logged in user
        $currentClient = $this->User->field('client_id'); // get client_id based on logged in user
        $members = $this->User->Client->find('first', array( // find users that have the same client_id
            'conditions' => array('id' => $currentClient),
            'recursive' => 1
        ));
        $this->set('clients', $members); // send the users to the view                  
    } else if ($this->request->is('post') || $this->request->is('put')) {
        $members = $this->request->data['Members']; // grab players submitted from push form
        $memberIds = array(); // this will hold the selected users
        foreach($members as $a){
            $memberIds[$a['id']] = $a['id']; // loop over user's that were selected
        }
        $usersToActivate = $this->User->find('all', array( //find user records, based on the array of id's
            'conditions' => array(
                "User.id" => $memberIds
            )
        ));
        $this->Ticket->bulkActivate($usersToActivate); // send array of members into model for processing
        $this->Session->setFlash('Activations sent.', 'default', array('class' => 'success'));
        $this->redirect(array('action' => 'index'));
    }
}

私の目には、大幅に間違っているようには見えません...そして、モデルですでにいくつかの処理を行っています（実際にユーザーレコードを取得し、アクティベーションチケットを生成するbulkActivateで見られるように）。

でも、まだ100%じゃない感じが否めません。

4

1 に答える 1

2

クライアントを 1 つだけ取得したいとは思いませんか?

$members = $this->User->Client->find('first', array

私はこれがすべてを見つけるはずだと思います。多くのユーザーがいる場合にページネーションを使用するようにこれを改善しました。私はこれに間違っているかもしれませんが、このコードを見ると、あなたの関連付けも本当の目標も明確ではありません. たとえば、チケットモデルが他のデータにどのように関連付けられているかわかりません。しかし、Controller::uses を使用していると思いますが、それを行うべきではなく、関連付けを通じて関連するモデルにアクセスする必要があります。

$a のようなひどい変数名を使用しないでください。これは最悪であり、より大きなコード ブロックやアプリケーションでそれが何を意味するのか誰も知りません。また、クライアント データを含む配列に $members という名前を付けましたが、なぜ $clients ではないのでしょうか? これを読んでください: Clean Codeと CakePHP については、 CakePHP コーディング標準に従うことをお勧めします。

目標を説明すると、これはさらにうまくリファクタリングできると思います。クライアントをアクティベートしてチケットにアクセスできるようにしたい場合 (それがどのように見えるか)、チケットまたはクライアント コントローラー/モデルでそれを行わなかったのはなぜですか?

また、この膨大な量のインライン コメントは、役に立たないどころか混乱を引き起こしています。きれいで読みやすいコードを書くと、そのコードはそれ自体で語ります。そこでは、超複雑なコードや超複雑な計算を行っていません。繰り返しになりますが、「Clean Code」を読むことをお勧めします。私の意見では、すべての開発者にとって「必読」です。

<?php
    // UsersController.php
    public function activate() {
        if ($this->request->is('post') || $this->request->is('put')) {
            $this->User->activate($this->request->data);
            $this->Session->setFlash('Activations sent.', 'default', array('class' => 'success'));
            $this->redirect(array('action' => 'index'));
        }

        this->Paginator->settings['Client'] = array(
            'conditions' => array('id' => $this->Auth->('current_id')),
            'contain' => array(
                'OnlyModelsYouNeedHere'));
        $this->set('clients', $this->Paginator->paginate($this->User->Client)); 
    }
?>

<?php
    // User.php - the model
    public function activate($data) {
        $memberIds = array();
        foreach($data['Members']members as $member) {
            $memberIds[$member['id']] = $member['id'];
        }
        $usersToActivate = $this->find('all', array(
            'conditions' => array(
                'User.id' => $memberIds)));
        return $this->Ticket->bulkActivate($usersToActivate);
    }
?>

ID を渡すことでさらに削減することもできますが、ここでは遅くなりました。:) これをあなたのコードの大まかなリファクタリングとして取り、私が何を変更したか、そしてより重要な理由について考えてください。

スキニー コントローラーとファット モデルの適切な例を見たい場合は、プラグインをチェックしてください。ユーザー プラグインの UsersController と Model を使用すると、より大きな画像が得られる場合があります。

于 2012-04-27T00:49:49.690 に答える