Hey Brad, for your curiosity, here's how I would approach this.
First, I would use a scope instead of a class method and remove take from it to leave it chainable as is expected from a scope.
Take performs the query immediately and you will get an array of values instead of an object that can be chained with another query method (where, etc.).
class Trail < ApplicationRecord
belongs_to :park
scope :in_state, -> (state) { joins(:park).where(parks: { state: state }) }
end
There's no more pagination, but this should really be in the controller, just like the logic about the trails.
I would introduce a private method to filter trails and fall back on the default all scope if no state is provided.
I use limit instead of take to postpone any query and leave the @trails chainable in case we need to filter further down the road. I introduced an optional :per param that you can send along with your query to have a more flexible limit (eg. trail_state_path("CA", per: 100))
I would also save the selected state to use it in the view.
class TrailsController < ApplicationController
def index
@state = params[:state]
@trails = filtered_trails || Trail.all
end
private
def filtered_trails
return unless params[:state].present?
Trail.in_state(params[:state]).limit(params[:per].presence || 10)
end
end
And now you don't need to have two blocks anymore for your list of trails in your index.
Also, you can use collection rendering instead of iterating.
@arnaud
Joubay Thank you for the time and suggestions. I'll remember the idea of keeping things as objects so one has the flexibility later. I like the refactor of removing the two blocks to show the states via collection rendering (a new concept for me).
Hey Brad, for your curiosity, here's how I would approach this.
First, I would use a scope instead of a class method and remove
takefrom it to leave it chainable as is expected from a scope.Take performs the query immediately and you will get an array of values instead of an object that can be chained with another query method (where, etc.).
There's no more pagination, but this should really be in the controller, just like the logic about the trails.
I would introduce a private method to filter trails and fall back on the default
allscope if no state is provided.I use
limitinstead oftaketo postpone any query and leave the@trailschainable in case we need to filter further down the road. I introduced an optional:perparam that you can send along with your query to have a more flexible limit (eg.trail_state_path("CA", per: 100))I would also save the selected state to use it in the view.
And now you don't need to have two blocks anymore for your list of trails in your index.
Also, you can use collection rendering instead of iterating.
You will probably want to make the currently selected state bolder, but I'll leave it there.
@arnaud Joubay Thank you for the time and suggestions. I'll remember the idea of keeping things as objects so one has the flexibility later. I like the refactor of removing the two blocks to show the states via collection rendering (a new concept for me).
You're welcome, glad you found my comment interesting.