1
\$\begingroup\$

I am making an eCommerce web application using the Laravel 5 framework.

I have created a Categories controller for the administrator and with the following actions for the categories:

Add / Delete / Update / Delete Permanently

I want to know whether the created controller matches the standards. If it doesn't, then how can it be improved?

AdminController.php

<?php namespace App\Http\Controllers;

use App\Category;
use App\Http\Requests;
use App\Http\Controllers\Controller;

use Illuminate\Http\Request;
use Illuminate\Support\Facades\DB;

/**
 * The AdminCategoriesController will handle all the actions
 * that are related to the categories. In this, addition, updation,
 * temporary deletion, permanent deletion of category are performed
 * using AJAX.
 *
 */
class AdminCategoriesController extends Controller {

    /**
     * Middleware to check if the admin is logged in
     *
     */
    public function __construct() {
        $this->middleware('admin');
    }

    /**
     * Show all the categories
     *
     * @return \Illuminate\View\View
     */
    public function index() {
        $cat_parent = Category::where('parent_id', '=', 0)->first();
        $categories = DB::table('categories as c')
                        ->join( 'categories as cp', 'c.parent_id', '=', 'cp.id' )
                        ->where('c.deleted_at', '=', NULL)
                        ->select(
                            'cp.name AS cat_parent_name',
                            'c.name AS cat_name',
                            'c.parent_id AS cat_parent_id',
                            'c.id AS cat_id',
                            'c.display AS cat_display_status',
                            'c.created_at AS cat_created_at'
                        )->get();
        return view( 'admin.categories.index', compact( 'categories', 'cat_parent' ) );
    }

    /**
     * Show the create form to the user
     *
     * @return \Illuminate\View\View
     */
    public function create() {
        $catList = Category::where('display', 'Enabled')->lists('name', 'id');
        return view( 'admin.categories.create', compact('catList') );
    }

    /**
     * Store the newly created category in the database if
     * no errors were detected.
     *
     * @param Request $request
     * @return \Illuminate\Contracts\Routing\ResponseFactory|\Symfony\Component\HttpFoundation\Response
     */
    public function store( Request $request ) {
        $this->validate($request, [
            'name'              => 'required',
            'parent_id'         => 'required|integer',
            'sort'              => 'required|integer',
            'display'           => 'required|in:Enabled,Disabled',
            'meta_title'        => 'required',
            'meta_keywords'     => 'required',
            'meta_description'  => 'required'
        ]);

        if ( $request->ajax() ) {
            Category::create( $request->all() );
            return response(['msg' => 'The category has been inserted successfully.', 'status' => 'success']);
        }
        return response(['msg' => 'Failed inserting the category.', 'status' => 'failed']);
    }

    /**
     * Show the edit form of the particular category id to edit
     *
     * @param $id
     * @return \Illuminate\View\View
     */
    public function edit( $id ) {
        $category = Category::findOrFail( $id );
        $catList = Category::where('display', 'Enabled')->lists('name', 'id');

        return view( 'admin.categories.edit', compact( 'category', 'catList' ) );
    }

    /**
     * Update the category of the particular id if the
     * validation fails.
     *
     * @param $id
     * @param Request $request
     * @return \Illuminate\Contracts\Routing\ResponseFactory|\Symfony\Component\HttpFoundation\Response
     */
    public function update( $id, Request $request ) {
        $category = Category::findOrFail( $id );

        $this->validate($request, [
            'name'              => 'required',
            'parent_id'         => 'required|integer',
            'sort'              => 'required|integer',
            'display'           => 'required|in:Enabled,Disabled',
            'meta_title'        => 'required',
            'meta_keywords'     => 'required',
            'meta_description'  => 'required'
        ]);

        if ( $request->ajax() ) {
            $category->update( $request->all() );

            return response(['msg' => 'Category updated', 'status' => 'success']);
        }
        return response(['msg' => 'Failed updating the category', 'status' => 'failed']);
    }

    /**
     * Temporarily delete the category of the particular id
     *
     * @param $id
     * @param Request $request
     * @return \Illuminate\Contracts\Routing\ResponseFactory|\Symfony\Component\HttpFoundation\Response
     */
    public function destroy( $id, Request $request ) {
        $category = Category::findOrFail( $id );

        if ( $request->ajax() ) {
            $category->delete( $request->all() );

            return response(['msg' => 'Category has been successfully deleted.', 'status' => 'success']);
        }
        return response(['msg' => 'Failed deleting the category', 'status' => 'failed']);
    }

    /**
     * Show all the temporarily deleted categories.
     *
     * @return \Illuminate\View\View
     */
    public function allDeleted() {
        $categories = Category::onlyTrashed()->get();
        return view( 'admin.categories.deleted', compact( 'categories' ) );
    }

    /**
     * Restore the category of the particular id
     *
     * @param $id
     * @param Request $request
     * @return \Illuminate\Contracts\Routing\ResponseFactory|\Symfony\Component\HttpFoundation\Response
     */
    public function restoreCategory( $id, Request $request ) {
        $category = Category::onlyTrashed()->where('id', $id)->first();

        if ( $request->ajax() ) {
            $category->restore();

            return response(['status' => 'success', 'message' => 'The category has been successfully restored.']);
        }
        return response(['status' => 'failed', 'message' => 'The category could not be restored.']);
    }

    /**
     * Delete the category permanently of the id provided
     *
     * @param $id
     * @param Request $request
     * @return \Illuminate\Contracts\Routing\ResponseFactory|\Symfony\Component\HttpFoundation\Response
     */
    public function deletePermanently( $id, Request $request ) {
        $category = Category::onlyTrashed()->where('id', $id)->first();

        if ( $request->ajax() ) {
            $category->forceDelete();

            return response(['status' => 'success', 'message' => 'The category has been successfully deleted.']);
        }
        return response(['status' => 'failed', 'message' => 'The category could not be deleted.']);
    }
}

routes.php

Route::get('/admin/categories/deleted', 'AdminCategoriesController@allDeleted');
Route::get('/admin/categories/{id}/restore', 'AdminCategoriesController@restoreCategory');
Route::get('/admin/categories/{id}/delete-permanently', 'AdminCategoriesController@deletePermanently');
Route::resource( '/admin/categories', 'AdminCategoriesController' );
\$\endgroup\$
3
\$\begingroup\$

"I want to know whether the created controller matches the standards."

I'm afraid it doesn't. Laravel's website states (on its home page):

Love beautiful code? We do too.

Your controller is really quite messy, to be honest.

What is a controller?

As the name suggests, a controller is the component that controls how a request is handled, what components do what tasks, and what response is sent back. That's its job. Querying the DB, processing raw resultsets and deleting data from certain tables is not part of a controllers' job description.

Interacting with the database is the job of the Model (note: capital M), as you can see here:

Laravel architecture

The routing component determines what controller should handle the request, the controller then pours the request data into model (note: lower case m) objects, initializes services and other components that belong to the Model layer, calls a couple of methods and passes the results to the View, which takes care of the end-user representation of the requested data.

That's how you write clean MVC code, regardless of the framework you use: Stick to the SOLID principles, the most important of all, IMHO, being the Single Responsibility principle: a class should have only one task. The controller should process the request. End of. Accessing the DB is beyond its responsibilities, it's that simple.

That's all nice in theory

Yes, I realize that Laravel's docs contain some code snippets where DB::table is used inside a controller. And it would seem that it's a rather common thing to do in the laravel community. But seeing as you're building an eCommerce application, you'll soon find yourself scrolling through endless action methods. All eCommerce platforms already out there offer some degree of modularity (some more than others), enabling/disabling certain components, the possibility to add custom components and what have you. If you start putting DB-related code in your controllers, you'll end up with something that is, by definition a lot less modular.

If it doesn't, then how can it be improved?

Even though, at this stage, it may seem tedious and silly, I'd strongly recommend you start working on a Model layer: create data models that can be extended (depending on the needs of the user), add services that perform certain queries, and make it so that these services are portable and can be configured as much as possible (using config, service container, middleware or whatever...).
The aim should be to end up with controller actions that are as small/elegant as possible. My rule of thumb is simple: If I have to scroll to read the full method (any method, not just controller actions), it's too big. If I can't see the docblock and the code of an action in a single screen, I'm doing something wrong.

After looking at your code, I suggest you create a Category model, and a Category service, and refactor your controller to look something like this:

public function index()
{
    $service = $this->getCategoryService();//<-- how you implement this is up to you
    return view(
        'admin.categories.index',
        $service->getCategories(true)//assume public function getCategories($withParant = false) returning an array
    );
}

public function edit($id)
{
    $model = new CategoryModel();
    //set the data the service needs to query
    $model->setName($id)
        ->setDisplay('Enabled');
    $service = $this->getCategoryService();
    return view(
        'admin.categories.edit',
         [
             'category' => $service->getCategory($model),
             'catList'  => $service->getCategoryList($model),
         ]
    );
}

Do the same for the other actions, and you're there. A clean controller, and DB interactions in a service that can be used by other controllers, too (making the platform a hell of a lot more flexible, and yielding a codebase with a lot less copy-pasting of queries).

Other comments:

When you Ask about your code matching the standards, it's important to note that there are standards (more like conventions) that you're recommended to follow. In case of Laravel, You can see what the coding style standards are here: PSR-0 and PSR-1, with 1 notable exception, you seem to be folling them:

  • Functions and control structures must use Allman style braces

Allman-style means your methods and if's are not written according to the Laravel standards:

public function destroy( $id, Request $request ) {
    $category = Category::findOrFail( $id );

    if ( $request->ajax() ) {
        $category->delete( $request->all() );

        return response(['msg' => 'Category has been successfully deleted.', 'status' => 'success']);
    }
    return response(['msg' => 'Failed deleting the category', 'status' => 'failed']);
}

Should be written like this:

public function destroy( $id, Request $request )
{//<--next line
    $category = Category::findOrFail( $id );

    if ( $request->ajax() )
    {//<--new line, too
        $category->delete( $request->all() );

        return response(['msg' => 'Category has been successfully deleted.', 'status' => 'success']);
    }
    return response(['msg' => 'Failed deleting the category', 'status' => 'failed']);
}

Lastly: I'm not a huge fan of compact, but that may be a personal thing. However, getting back to reusability and modularity: if you pass all local variables using compact to the view, you should be extremely careful: maintaining, debugging and refactoring your code is a potential minefield (rename a variable, and you're in trouble)... I'd strongly advise you to write the arrays manually. It's not that much work anyway...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Well, thanks a lot for the improvements in my code. I was not knowing this at all. But since I am at the learning stage, I would like to learn more about services (creating and handling, etc). Where would I find it other than the documentation. You said that I need to create a Model Category, I have created that already. \$\endgroup\$ – Saiyan Prince Jun 11 '15 at 10:21
  • \$\begingroup\$ @user3514160: Though perhaps a bit dry and academic, this seems like a decent enough explanation of the MVC pattern, I've posted a couple of reviews like this one giving a more in-depth expose of the MVC pattern/concepts, have a look at some of them here, that should help you on your way. If you do post a follow-up, leave a comment here and I'll try to look into it (if I find the time) \$\endgroup\$ – Elias Van Ootegem Jun 11 '15 at 11:55
  • \$\begingroup\$ This is another post you might want to look at, it's another walkthrough of the MVC pattern I gave. Of course, it's not 100% comprehensive, but if you want some more background, I think it's worth a look \$\endgroup\$ – Elias Van Ootegem Jun 11 '15 at 12:01

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.