3
\$\begingroup\$

I have a VenuesController and an EventsController. My venues controller has an admin_add method that, well, allows an administrator to add a venue to the database. It looks like this:

public function admin_add() {
    if ($this->request->is('post')) {
        if ($this->Venue->save($this->request->data)) {
            $message = 'Venue has been created.';
            $this->response->statusCode(201);
            if ($this->request->params['ext'] == 'json') {
                $this->set('id', $this->Venue->id);
                $this->set('_serialize', array('id'));
            }
            else {
                $this->Session->setFlash($message, 'flash', array('class' => 'success'));
                $this->redirect(array('action' => 'index'));
            }
        }
        else {
            if ($this->request->params['ext'] == 'json') {
                $this->set('validationErrors', $this->Venue->validationErrors);
                $this->set('_serialize', array('validationErrors'));
                $this->response->statusCode(400);
            }
        }
    }
}

As you can see, there are a lot of if () statements which seem like code smells to me. But they’re there because in my CMS’s events section, you can add a venue inline via a modal, which sents the request to this function via AJAX.

So my question is: can I improve the above VenuesController::admin_add() method, make it more intelligent without the need for the if () statements, and just generally make it more CakePHP-ier?

\$\endgroup\$
  • \$\begingroup\$ Sometimes you can't avoid having lots of if statements in your code. In your case the only thing I notice that I personally don't like is that you have sections of code inside if statements which would be prevented by calling if(!$some_condition) return false; then continuing with your logic. \$\endgroup\$ – Robbie Averill May 12 '14 at 3:18
2
\$\begingroup\$

Why do you have the following lines repeating:

    if ($this->Venue->save($this->request->data)) {
        if ($this->Venue->save($this->request->data)) {

I think you may try to merge them to one and the associated logic too.

\$\endgroup\$
  • \$\begingroup\$ I didn’t actually know I had, that’s a bug! There should only be one save() call. That’s late night coding for you! \$\endgroup\$ – Martin Bean Sep 2 '13 at 10:38
0
\$\begingroup\$

For Aditional security i would definitely include the session or the cookie component

public $components = array('Session', 'Cookie');

and inside the function this if

if ($this->Session->read('Auth.User.role') == 'admin' || $this->Cookie->read('User.role') == 'admin') {

for more details you could check these

\$\endgroup\$
  • 1
    \$\begingroup\$ This is redundant, as I check the user role in a beforeAction() method in my AppController to keep things DRY. \$\endgroup\$ – Martin Bean Jan 7 '14 at 10:00

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.