-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tip 1: Give nested resources a dedicated controller #1
Conversation
Thanks @adamwathan, this is really useful, beautiful and mind-blowing for me! |
@adamwathan thank you. very useful |
Thanks :) |
A very useful post @adamwathan ! Thanks. |
This is great, thank you @adamwathan! |
@adamwathan, I was wondering if there was a particular reason for you to use
instead of
with |
@ptournet I don't really use route model binding very often so just habit. Don't like that it forces me to add a type-hint unless I set up the binding manually 😉 |
What if I want to remove all episodes for a given podcast? |
@nikolaynizruhin Yep I think that's the right move! |
Thanks! |
Hi Adam! |
Personally I don't like overly nested folder structures or files with the same name in different directories. I find things easier to navigate with flatter project structures. |
Even if there are 100 files in a single directory?
|
Yep doesn't bother me! |
@pbgneff Makes it easier to import too without renaming |
How is it recommended to go for the update route? Usually you have Following this approach I end up with But this breaks the rule of the controller being concerned only about the ID of one resource type. Is it ok to just use the non-nested resource controller here (as in the first case)? I don't think I care about the Podcast ID if I have the Episode ID, is that correct? |
@Nartub600 I would keep the update action on the EpisodesController, we still have an EpisodesController for these actions even after this refactoring: |
Hi @adamwathan Is it considered wrong to display the episodes of a specific podcast using a show method
|
@Orest-Divintari, it's okay, you can even see it in the examples: laracon2017/app/Http/Controllers/PodcastsController.php Lines 43 to 53 in 796c788
laracon2017/app/Http/Controllers/EpisodesController.php Lines 19 to 28 in 796c788
|
When dealing with nested resources in an application, you often end up with routes like this:
When you have an endpoint like this, which controller do you use and what do you call the action?
❌ Option 1: PodcastsController
Since
/podcasts
is the top level resource, it's common to use thePodcastsController
for these sorts of nested index actions.This can work but it forces you to define a "custom action"; something that isn't part of the 7 standard REST/CRUD actions that exist on a resource controller.
In this example, we started with an action on
PodcastsController
calledlistEpisodes
. If we want to stick to only the 7 standard controller actions, what else could we do?❌ Option 2: EpisodesController
Since this action is a list of episodes, you might think "hey, let's make this
EpisodesController@index
!"But there's a problem: we already have an
EpisodesController@index
action! In our case, it lists all episodes across all podcasts.Antipattern: Optional route parameters
One thing I've seen folks do in this case is re-use
EpisodesController@index
for two different use cases: listing all episodes or listing an individual podcast's episodes.That usually looks like this in a routes file:
...and like this in the controller:
The problem with this approach is that you are taking two completely different actions from the user's point of view and stuffing them all into one action that has to reverse-engineer the intent of the user based on the presence of the route parameter.
These actions have literally nothing in common:
If our goal is to use more controllers to simplify our code, this isn't helping. We actually have one less controller action than before, resulting in another controller action getting much more complicated.
Don't reuse a single controller action for two different user actions.
✅ Option 3: Dedicated PodcastEpisodesController
If we want to stick to only standard actions, that means we want to use an
index
action here. But what is the resource we are requesting anindex
of?You could say it's
episodes
, but we're not requesting an index of all episodes. It's a specific podcast's episodes, so you could say we want an index of "podcast episodes".If we create a brand new controller for this, here's what our route definition would look like:
...and here's the controller with it's action:
When taking this approach, I usually make my template folder structure match the controller name, as you can see above with the
podcast-episodes.index
view.Side benefit: Attracting existing nested resource endpoints
If you look in the routes file, you'll see we have two other
episodes
related endpoints nested under the top-levelpodcasts
resource:This isn't bad on it's own, but it's sort of awkward that inside of
EpisodesController
, the$id
route parameter has multiple meanings.For example, here it represents an
Episode
ID:...but in our
create
action, it represents aPodcast
ID:To me this is a bit of a smell.
Each controller should only be concerned about the ID of one type of resource.
Now that we have a dedicated
PodcastEpisodesController
, we can move ourcreate
andstore
actions alongside our newindex
action, and now the$id
parameter will always refer to aPodcast
ID:After this refactoring, we're left with 3 controllers:
PodcastsController
, with 7 standard actions and 5 custom actionsEpisodesController
, with 4 standard actions and no custom actionsPodcastEpisodesController
, with 3 standard actions and no custom actionsNext up: Treat properties that are edited independently like a separate resource