eCommerce project using the Repository pattern

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

```
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, $fromDelet

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.

StackExchange Code Review Q#105342, answer score: 4