# GET /neighbourhoods/map_filter
def map_filter
@map_filter_errors = nil # Stores errors related to a filter request.
@properties = nil # Stores the properties that match the filter constraints.
# Extract the "accessible" attributes from the params.
property_filter_attributes = AccessibleModelAttributes.extract PropertyFilter, params
filtered_properties = PropertyFilter.new(property_filter_attributes).properties
# If properties were returned.
if filtered_properties[:properties]
@properties = filtered_properties[:properties]
@property_data = render_to_string(
:partial => 'properties/map_properties_table',
:locals => {:properties => @properties}
)
@number_of_properties_found_sentence = render_to_string :partial => 'properties/number_of_properties_found'
else
@map_filter_errors = filtered_properties[:errors]
end
end
Refactorings
No refactoring yet !
Pat Maddox
November 1, 2008, November 01, 2008 00:13, permalink
def map_filter
filtered_properties = PropertyFilter.filter_by params
unless filtered_properties.blank?
@property_data = render_to_string(
:partial => 'properties/map_properties_table',
:locals => {:properties => filtered_properties}
)
@number_of_properties_found_sentence = render_to_string :partial => 'properties/number_of_properties_found'
end
@map_filter_errors = filtered_properties.errors
end
class PropertyFilter
def self.filter_by(options)
property_filter_attributes = AccessibleModelAttributes.extract self, options
new property_filter_attributes
end
def empty?
@properties.empty?
end
def each(&block)
@properties.each(&block)
end
end
Jarkko Laine
November 1, 2008, November 01, 2008 05:51, permalink
* You don't need to set instance variables to nil, they are nil implicitly (and don't ever cause the "no such method or variable" exception).
* Move line 7 to PropertyFilter#initialize (or some helper method inside that class). It's imho really part of the domain model.
* Don't render to string and store it in instance variables. Just render partials normally in the views, jut like Pat already said.
def map_filter
filtered_properties = PropertyFilter.new(params).properties
# If properties were returned.
if filtered_properties[:properties]
@properties = filtered_properties[:properties]
else
@map_filter_errors = filtered_properties[:errors]
end
end
Nick
November 1, 2008, November 01, 2008 17:10, permalink
Thanks Jarkko. The action's a lot more concise without those instance variable definitions up top, and the calls to #render_to_string .
I have a habit of defining variables at the beginning of my methods...Perl's "use strict;" battered that into my head many years ago.
Adam
November 3, 2008, November 03, 2008 05:32, permalink
I get feeling that you really need a model around the properties. Your controller still seems unnecessarily fat. Unfortunately, there's not enough code here for me to really suggest a better way.
Nick
November 3, 2008, November 03, 2008 05:46, permalink
Hey Adam. There's a Property model, which originally containted what's now the PropertyFilter model. Basically, I needed to do large, multi-param finds, so started building it into the Property model. After a few iterations and feedback from others, I moved the processing of the search params, the building of the args to #find , and the call to #find out of the Property model and into a new model, PropertyFilter.
Because PropertyFilter doesn't require data to persist, it inherits from AR::BaseWithoutTable .
My Neighbourhood "map_filter" action finds Property objects that match search criteria submitted by the user. It works, but I reckon it could be a bit shorter. Any suggestions?