4
\$\begingroup\$

I am currently working on an eCommerce project in Laravel 5.1. I have implemented Repository Pattern by learning from this site only for 1 model, namely, Carousel just to test myself as I am still at the learning stage.

Now I want my code to be reviewed, before moving on to the next part of my project.

CarouselRepositoryInterface.php

<?php

namespace App\Repositories\Contracts;

use App\Http\Requests\CarouselRequest;

interface CarouselRepositoryInterface
{
    public function getAllCarousels($fromDeleted = false, $fromLatest = true);

    public function storeNew(CarouselRequest $request);

    public function find($id, $fromDeleted = false, $fromLatest = true);

    public function update($id, CarouselRequest $request);

    public function deleteTemporarily($id);

    public function restore($id);

    public function deletePermanently($id);
}

CarouselRepository.php

<?php

namespace App\Repositories\Eloquent;

use App\Carousel;
use App\Http\Requests\CarouselRequest;
use App\Repositories\Contracts\CarouselRepositoryInterface;

class CarouselRepository implements CarouselRepositoryInterface
{
    /**
     * Get the list of all the carousels.
     * 
     * @param  bool $fromDeleted Whether the result should include deleted models as well.
     * @param  bool $fromLatest  Whether the results should be sort in descending order.
     * @return mixed
     */
    public function getAllCarousels($fromDeleted = false, $fromLatest = true)
    {
        if($fromDeleted === true && $fromLatest === true) {
            return Carousel::onlyTrashed()->latest()->get();
        } elseif($fromDeleted === true && $fromLatest ===  false) {
            return Carousel::onlyTrashed()->get();
        } elseif($fromDeleted === false && $fromLatest === true) {
            return Carousel::latest()->get();
        } else {
            return Carousel::get();
        }
    }

    /**
     * Store the model in the database after validation passes.
     * 
     * @param  \App\Http\Requests\CarouselRequest $request The validation for adding the model in the database.
     * @return bool|int
     */
    public function storeNew(CarouselRequest $request)
    {
        $carousel = Carousel::create($request->all());

        $carousel->products()->attach($request->input('product_list'));

        return $carousel;
    }

    /**
     * Find the carousel of the given id.
     * 
     * @param  int  $id          The id of the carousel.
     * @param  bool $fromDeleted Find in deleted model(s).
     * @param  bool $fromLatest  Find in latest model(s).
     * @return \Illuminate\Database\Eloquent\Model
     */
    public function find($id, $fromDeleted = false, $fromLatest = true)
    {
        if($fromDeleted === true && $fromLatest === true) {
            return Carousel::onlyTrashed()->latest()->find($id);
        } elseif($fromDeleted === false && $fromLatest === true) {
            return Carousel::latest()->find($id);
        } elseif($fromDeleted === true && $fromLatest === false) {
            return Carousel::onlyTrashed()->find($id);
        } else {
            return Carousel::find($id);
        }
    }

    /**
     * Update the carousel model of the given id.
     * 
     * @param  int                                $id      The id of the carousel that needs to be updated.
     * @param  \App\Http\Requests\CarouselRequest $request The validation for updating the model in the database.
     * @return bool|int
     */
    public function update($id, CarouselRequest $request)
    {
        $carousel = $this->find($id);

        $carousel->update($request->all());

        $carousel->products()->detach($carousel->product_list->toArray());

        $carousel->products()->attach($request->input('product_list'));

        return $carousel;
    }

     /**
     * Temporarily delete the model from the database.
     * 
     * @param  int      $id The id of the carousel that needs to be temporarily deleted.
     * @return bool|int
     */
    public function deleteTemporarily($id)
    {
        $carousel = $this->find($id);

        $carousel->delete();

        return $carousel;
    }

    /**
     * Restore the deleted carousel model from the database.
     * 
     * @param  int      $id The id of the carousel that needs to be restored.
     * @return bool|int
     */
    public function restore($id)
    {
        $carousel = $this->find($id, true);

        $carousel->restore();

        return $carousel;
    }

    /**
     * Permanently delete carousel model from the database.
     * 
     * @param  int      $id The id of the carousel that needs to be permanently deleted.
     * @return bool|int
     */
    public function deletePermanently($id)
    {
        $carousel = $this->find($id, true);

        $carousel->forceDelete();

        return $carousel;
    }
}

I have following questions on my mind:

  1. What are the things that I have missed / not followed according to the docs or according to the convention?
  2. Is my code optimized for the production level?
  3. What is/are thing(s) that I need to improve upon, in respect to the coding standards / convention, etc.?
\$\endgroup\$
4
\$\begingroup\$

First I'd like to start off by saying that if that code suits you, then it's not a bad idea to stick with it. It's fairly clean and pretty neat. I'm just going to point out a few things that may diverge from different types of implementation.


Let's talk about the store and update methods. You are injecting a Reqeust class in it, which means your repository class is not just communicating with your data storage, but also validating your input. In one hand you have reusable code with implicit validation, which is kind of a nice idea, you're assuming that whenever you want to save a new entity, you want it to be validated. On the other hand, I have a little problem with it regarding different types of validation for the same entity. Here is an example:

Suppose you have a tickets table where a customer creates a new ticket. When a customer creates a new ticket, the validation requires a title, body and attachments (optional). Now, the software also allows your staff to open tickets. When they're opening a ticket, it's required they add title, body, attachments (optional), subject and department that defines which department is responsible for that ticket. In this scenario, we're going to be using ticketsRepository in both cases, but they require different validation. That is the main problem of having your Repository Pattern validating your data. In order to achieve this, you'll have more trouble.


Secondly, You have too much complexity on your find and getAll method. You're checking for deleted records and/or ascending order and you're ending up with 4 blocks of code. I'd suggest you to keep those methods simple. If you want to find deleted records, call a different method. If you think about it, when calling those methods you're already deciding what you want based on the method's input. Instead of sending an input to decide it for you, change the getAll() to getDeleteds().


The last thing I'd comment are your method names. It's really not a big deal and it's perfectly fine the way you're doing it, but I would choose a convention and stick with it. In your case, if you want to get all carousels, you'd call getAllCarousels(); and if you want all users you'd call getAllUsers(); . Personally, I prefer if all loading methods stick with a simple name, such as getAll();. If you want to know what entity you're getting all records, you just have to check which object you're holding.

\$\endgroup\$
  • \$\begingroup\$ I would say the issue with injecting a Request object into a Repository method is that it is the Controller's job to handle requests. Controller's allow you to decouple data and business logic from the work of serving HTTP requests, allowing your Model/Repository code to be useful if the application someday imports data from spreadsheets or has to communicate with a reporting system. \$\endgroup\$ – Josh Rumbut Nov 5 '15 at 14:39
  • \$\begingroup\$ @JoshRumbut Regarding data import I'd have to disagree. I've handled those cases and you can't trust the data being imported. I'm not sure how I would go about implementing validation for data import in this case, but I'm sure I wouldn't let the data go without being validated. \$\endgroup\$ – Marco Aurélio Deleu Nov 5 '15 at 14:53
  • 1
    \$\begingroup\$ I agree with that, and I'm not sure how I gave the impression that I didn't think validation wasn't required. My comment was that Requests were the concern of the Controller, and was suggesting a few practical situations where you might want to use your Repository code without an HTTP request occurring (via tinker, job queues, etc). Those situations will require their own validation which is beyond the scope of my comment. \$\endgroup\$ – Josh Rumbut Nov 5 '15 at 15:02

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.