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

Things to refactor #74

Closed
mzagaja opened this Issue Mar 31, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@mzagaja
Contributor

mzagaja commented Mar 31, 2017

Need to figure out why nested routes don't work right.

We are not using the owned action in the positions controller. Need to figure out better way to serve the positions owned by the requesting user.

The sites model is a bit ugly.

@mzagaja

This comment has been minimized.

Show comment
Hide comment
@mzagaja

mzagaja Mar 31, 2017

Contributor

@allthesignals Found this pattern for nested and un-nested resources, but more Googling seems to reveal that your pattern is actually not far off from the accepted practice: http://stackoverflow.com/questions/21766957/how-to-manage-both-nested-and-not-nested-resources

Contributor

mzagaja commented Mar 31, 2017

@allthesignals Found this pattern for nested and un-nested resources, but more Googling seems to reveal that your pattern is actually not far off from the accepted practice: http://stackoverflow.com/questions/21766957/how-to-manage-both-nested-and-not-nested-resources

@allthesignals

This comment has been minimized.

Show comment
Hide comment
@allthesignals

allthesignals Mar 31, 2017

Contributor

@mzagaja Rails docs seem to suggest nested resources should work out of the box... see the table here: http://guides.rubyonrails.org/v3.2.9/routing.html#nested-resources.

image

Maybe it's not saying it will handle the controller logic of only getting particular related resources when they're nested? Thoughts?

It's just sad to have to write all those methods!

Contributor

allthesignals commented Mar 31, 2017

@mzagaja Rails docs seem to suggest nested resources should work out of the box... see the table here: http://guides.rubyonrails.org/v3.2.9/routing.html#nested-resources.

image

Maybe it's not saying it will handle the controller logic of only getting particular related resources when they're nested? Thoughts?

It's just sad to have to write all those methods!

@mzagaja

This comment has been minimized.

Show comment
Hide comment
@mzagaja

mzagaja Apr 4, 2017

Contributor

So http://railscasts.com/episodes/139-nested-resources?autoplay=true at 4m mark shows a pattern whereby the child nested controller needs to be modified to summon the parent nested resource and then summon the child resources through the parent. If we follow this pattern to refactor your code it'd be something like

def index
  @position = Position.find(params[:position_id])
  @applicants = @position.applicants.find(params[:id])
end

instead of @applicants = Applicant.includes(:positions).where('positions.id' => params[:position_id])

The StackOverflow above demonstrates that if you have both nested and un-nested routes, that your if params logic is unavoidable. Definitely agree that it could be more elegant.

Contributor

mzagaja commented Apr 4, 2017

So http://railscasts.com/episodes/139-nested-resources?autoplay=true at 4m mark shows a pattern whereby the child nested controller needs to be modified to summon the parent nested resource and then summon the child resources through the parent. If we follow this pattern to refactor your code it'd be something like

def index
  @position = Position.find(params[:position_id])
  @applicants = @position.applicants.find(params[:id])
end

instead of @applicants = Applicant.includes(:positions).where('positions.id' => params[:position_id])

The StackOverflow above demonstrates that if you have both nested and un-nested routes, that your if params logic is unavoidable. Definitely agree that it could be more elegant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment