current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm quite new to Laravel, and I'm not sure what am I doing is the best practice.

I'd like to return JSON if the request is Ajax, or return a view otherwise. This is the way I made it, and it works fine, but I'm not sure if this is the best way(it seems long winded). I'd really like to hear your suggestions.

public function store()
{
    $input = Input::all();

    if(!$this->settings->fill($input)->isValid())
    {
        if ( Request::ajax() ){
            return $this->jsonFailure(array(
                'errors' => $this->settings->errors
            ));
        }
        else{
            return Redirect::route('admin.settings.create')
                ->withInput()
                ->withErrors($this->settings->errors);
        }
    }

    $this->settings->save();

    if ( Request::ajax() )
        return $this->jsonSuccess('success');
    else
        Redirect::route('admin.settings.index');
}
share|improve this question

3 Answers 3

up vote 4 down vote accepted

The only problem with this approach is that as you add more methods, you are going to end up with an awful lot of repeated if (Request::ajax()) ... blocks all over your code.

Here's what I do for my sites:

<?php
class ApiController extends Controller {
    protected function makeResponse($data, $isError = false, $redirectToIfNotAjax = '/') {
        if (Request::ajax()) {
            if ($isError) {
                return $this->jsonFailure(array(
                    'errors' => $data
                ));
            } else {
                return $this->jsonSuccess($data);
            }
        } else {
            if ($isError) {
                return Redirect::route($redirectToIfNotAjax)
                    ->withInput()
                    ->withErrors($data);
            } else {
                Redirect::route($redirectToIfNotAjax);
            }
        }
    }
}

(This is not exactly the same as what I am using, but it's pretty close.)

Then, inherit your controllers from ApiController instead of just Controller.

Here is what your example would look like:

public function store()
{
    $input = Input::all();

    if(!$this->settings->fill($input)->isValid())
    {
        return makeResponse($this->settings->errors, true, 'admin.settings.create')
    }

    $this->settings->save();

    return makeResponse('success', false, 'admin.settings.index')
}

Much cleaner!

share|improve this answer
    
thank you, really great solution –  Angel M. Jul 24 '14 at 5:14
up vote 4 down vote

If you follow Single Responsibility principle than your controller should not know how view is presented. I would rather create view presenter class in you app/Projectname/Presenters folder and than inject it thru constructor. That way your code would be cleaner and more testable.

share|improve this answer
1  
Great, this is somewhat I had in mind. To put it in a dedicated class and use it everywhere you need –  Filip Zelic Jul 24 '14 at 7:44
up vote 1 down vote

make two different methods and in the routes.php change one for get and another for post and do the ajax request with post, that way you can use the same URL and separate functionality.

share|improve this answer
1  
That sounds like significant unnecessary code duplication. –  Moshe Katz Jul 24 '14 at 4:41

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.