Level 61
It would be so much easier to review if it was posted on GitHub or something. Looks super clean but I do not like it. I think that you should watch this video https://www.youtube.com/watch?v=MF0jFKvS4SI
Hope it helps.
1 like
Jun 13, 2018
5
Level 1
I have made a base controller to DRY my API controllers, i would like some feedback
I'm still new at Laravel, i don't really know if what i coded is bad. I studied a little about repository pattern, however i felt like doing a base controller would be simple and would solve my problem.
My controller receives json and returns json response. Heres my base controller:
abstract class ApiController extends Controller
{
use ApiResponser;
protected $model;
protected $storeRules, $updateRules, $deleteRules;
public function store(Request $request)
{
$this->validate($request, $this->storeRules);
$requestData = $request->get($this->model::table());
if (!$this->model::insertAll($requestData))
return $this->errorResponse('Could not insert data', 500);
return $this->showAll($requestData, 201);
}
public function index()
{
$items = $this->model::all();
return $this->showAll($items);
}
public function show($id)
{
$item = $this->model::findOrFail($id);
return $this->showOne($item);
}
public function update(Request $request)
{
$this->validate($request, $this->updateRules);
$requestData = $request->get($this->model::table());
$updated = [];
foreach ($requestData as $data) {
$item = $this->model::findOrFail($data['id']);
$item->fill($data);
if ($item->isDirty()) {
$item->save();
array_push($updated, $item);
}
}
return $this->showAll($updated, 200);
}
public function destroy(Request $request)
{
$this->validate($request, $this->deleteRules);
$requestData = $request->input($this->model::table().'.*.id');
$deleted = $this->model::destroy($requestData);
return $this->showAll($deleted, 201);
}
Here is how a child controller looks like:
class CourseController extends ApiController
{
public function __construct()
{
$this->model = Course::class;
$this->storeRules = Course::STORE_RULES;
$this->updateRules = Course::UPDATE_RULES;
$this->deleteRules = Course::DELETE_RULES;
}
Please if anyone could review it or at least give me some advice i would appreciate. First time posting here in laracasts discussion, sorry if my post is poorly structured.
Level 1
Thank you a bunch for that video, i will rethink my routes and controllers. I wish i could post the API on GitHub but i'm afraid i can't post it, i'm building it to the company i'm currently working, i can only share some parts of it.
Looks super clean but I do not like it.
What do you dislike about it?
Level 61
Too much abstraction, magic, and automation.
I like implicit model binding, request objects validation. Takes controllers cleanliness to the extreme.
Dedicated controllers are a must in real-world apps since every model has different relationships and it's better to have all the logic in the app than to outsource for example cascade delete to the database. What if you switch to some database that does not support it? It would get very dirty to put that logic in the controller you wrote. Also overwriting base controller methods is not a common pattern and it can be overlooked easily especially by new team members.
This is just an excerpt but there is so much that would bring complexity and headache if you go with this snippet of code.
I think I've heard @JeffreyWay say that programmers (especially juniors) are hesitant to create new classes and they tend to put everything in existing structure/files. This might not apply to you but it gave me that impression.
Sorry if my thoughts are confusing. I lack some serious amount of sleep.
1 like
Level 1
I'm really thankful for your reply @bugsysha , i was afraid my post was going to be ignored haha. I really have little experience when it comes to programming but i'm working on it !
It would get very dirty to put that logic in the controller you wrote.
It gets kind of cloudy when i try to imagine it, however i think i know where you're getting at. There are some polymorphic relationships that need some cascading delete i have to think of and it's probably the case you've mentioned.
I think I've heard @JeffreyWay say that programmers (especially juniors) are hesitant to create new classes and they tend to put everything in existing structure/files. This might not apply to you but it gave me that impression.
Hmmm, it is my case i'm pretty sure. After watching the video you suggested i found out that my controllers are pretty much the same as it was at the start of that video, i will have to create new controllers for relationship actions. Now i know why Route::resource is used.
Sorry if my thoughts are confusing. I lack some serious amount of sleep.
It was a good reply ! Thank you.
Level 61
i was afraid my post was going to be ignored
That happened to me too many times. It is time to give something back :)
It gets kind of cloudy when i try to imagine it
I know exactly what you are trying to say. Only experience can remedy that. I always thought that some people are at level I can not reach, ever, not in one life time. But then I've realized that it boils down to experience.
i found out that my controllers are pretty much the same as it was at the start of that video
We've all wrote them so don't worry. Follow Adam he is great.
Now i know why Route::resource is used.
I always used only resource controller boilerplate and it never failed me. Get that into your workflow and in no time you will see progress.
It was a good reply ! Thank you.
My pleasure.
1 like
Please or to participate in this conversation.