From c6c163c2ac7075508d8ea6e4c372782dc335f87e Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 7 Dec 2014 20:38:51 +0100 Subject: [PATCH] Refactor #sort_link to extract mutation and order-dependent code into a single `initialize!` method and reduce what was previously a large, 100-line method with values mutating from start to finish, into a 5-line method that calls other small, order-independent, mutation-free methods. Consider this an experiment. It could be much better. There is a bit more to do and improve, and a few places where I think the code can be optimized to run faster. The two form_helper methods could probably benefit from much better documentation in the code itself, to help developers who would like to improve it and submit PRs. --- lib/ransack/helpers/form_helper.rb | 208 +++++++++++++++-------------- 1 file changed, 108 insertions(+), 100 deletions(-) diff --git a/lib/ransack/helpers/form_helper.rb b/lib/ransack/helpers/form_helper.rb index e9a9980bf..c8eb5b611 100644 --- a/lib/ransack/helpers/form_helper.rb +++ b/lib/ransack/helpers/form_helper.rb @@ -2,21 +2,25 @@ module Ransack module Helpers module FormHelper + # +search_form_for+ + # + # Example: <%= search_form_for(@q) do |f| %> + # def search_form_for(record, options = {}, &proc) - if record.is_a?(Ransack::Search) + if record.is_a? Ransack::Search search = record options[:url] ||= polymorphic_path( search.klass, format: options.delete(:format) ) - elsif record.is_a?(Array) && - (search = record.detect { |o| o.is_a?(Ransack::Search) }) + elsif record.is_a? Array && + (search = record.detect { |o| o.is_a? Ransack::Search }) options[:url] ||= polymorphic_path( - record.map { |o| o.is_a?(Ransack::Search) ? o.klass : o }, + record.map { |o| o.is_a? Ransack::Search ? o.klass : o }, format: options.delete(:format) ) else raise ArgumentError, - "No Ransack::Search object was provided to search_form_for!" + 'No Ransack::Search object was provided to search_form_for!' end options[:html] ||= {} html_options = { @@ -35,145 +39,149 @@ def search_form_for(record, options = {}, &proc) form_for(record, options, &proc) end - # sort_link(@q, :name, [:name, 'kind ASC'], 'Player Name') + # +sort_link+ + # + # Example: <%= sort_link(@q, :name, [:name, 'kind ASC'], 'Player Name') %> + # def sort_link(search, attribute, *args) - # Extract out a routing proxy for url_for scoping later - if search.is_a?(Array) - routing_proxy = search.shift - search = search.first - end + @search_object, routing_proxy = extract_search_obj_and_routing(search) + raise TypeError, 'First argument must be a Ransack::Search!' unless + Search === @search_object + initialize!(search, attribute, args) + link_to(name, url(routing_proxy), html_options(args)) + end - raise TypeError, "First argument must be a Ransack::Search!" unless - Search === search + private - # This is the field that this link represents. The direction of the sort icon (up/down arrow) will - # depend on the sort status of this field - field_name = attribute.to_s + # All mutation and order-dependent code happens here. + # Ivars are not mutated outside of this method. + def initialize!(search, attribute, args) + @field_name = attribute.to_s + @sort_fields, args = extract_sort_fields(args) + @label_text, args = extract_label_text(args) + @options, args = extract_options(args) + @hide_indicator = @options.delete :hide_indicator + @default_order = @options.delete :default_order + if Hash === @default_order + @default_order = @default_order.with_indifferent_access + end + @sort_params = initialize_sort_params + @sort_params = @sort_params.first if @sort_params.size == 1 + @current_dir = existing_sort_direction + end - # Determine the fields we want to sort on - sort_fields = if Array === args.first - args.shift - else - Array(field_name) + def name + [ERB::Util.h(@label_text), order_indicator] + .compact.join(Constants::NON_BREAKING_SPACE).html_safe end - label_text = - if String === args.first - args.shift.to_s + def url(routing_proxy) + if routing_proxy && respond_to?(routing_proxy) + send(routing_proxy).url_for(options_for_url(@options)) else - Translate.attribute(field_name, :context => search.context) + url_for(options_for_url(@options)) end + end - options = args.first.is_a?(Hash) ? args.shift.dup : {} - hide_indicator = options.delete :hide_indicator - default_order = options.delete :default_order - default_order_is_a_hash = Hash === default_order + def options_for_url(options) + options[@search_object.context.search_key] = search_and_sort_params + params.merge(options) + end - # If the default order is a hash of fields, duplicate it and let us - # access it with strings or symbols. - if default_order_is_a_hash - default_order = default_order.dup.with_indifferent_access + def search_and_sort_params + search_params.merge(:s => @sort_params) end - search_params = params[search.context.search_key].presence || + def search_params + params[@search_object.context.search_key].presence || {}.with_indifferent_access - - # Find the current direction (if there is one) of the primary sort field - if existing_sort = search.sorts.detect { |s| s.name == field_name } - field_current_dir = existing_sort.dir end - sort_params = initialize_sort_params(sort_fields, existing_sort, - search, default_order_is_a_hash, default_order) - - # if there is only one sort parameter, remove it from the array and just - # use the string as the parameter - if sort_params.size == 1 - sort_params = sort_params.first + def html_options(args) + html_options = extract_options(args).first + html_options.merge(class: [css, html_options[:class]] + .compact.join(Constants::SPACE)) end - html_options = args.first.is_a?(Hash) ? args.shift.dup : {} - css = [Constants::SORT_LINK, field_current_dir] - .compact.join(Constants::SPACE) - html_options[:class] = [css, html_options[:class]] - .compact.join(Constants::SPACE) - - query_hash = {} - query_hash[search.context.search_key] = search_params - .merge(:s => sort_params) - options.merge!(query_hash) - options_for_url = params.merge(options) + def css + [Constants::SORT_LINK, @current_dir].compact.join(Constants::SPACE) + end - url = build_url_for(routing_proxy, options_for_url) - name = link_name(label_text, field_current_dir, hide_indicator) + def extract_search_obj_and_routing(search) + if search.is_a? Array + [search.second, search.first] + else + [search, nil] + end + end - link_to(name, url, html_options) - end + def extract_sort_fields(args) + if Array === args.first + [args.shift, args] + else + [Array(@field_name), args] + end + end - private + def extract_label_text(args) + if String === args.first + [args.shift, args] + else + [Translate.attribute(@field_name, :context => @search_object.context), args] + end + end - # Extract out a routing proxy for url_for scoping later - def routing_proxy_and_search_object(search) - if search.is_a? Array - [search.first, search.second] + def extract_options(args) + if args.first.is_a? Hash + [args.shift, args] else - [nil, search] + [{}, args] end end - def initialize_sort_params(sort_fields, existing_sort, search, - default_order_is_a_hash, default_order) - sort_fields.each_with_object([]) do |field, a| + def initialize_sort_params + @sort_fields.each_with_object([]) do |field, _| attr_name, new_dir = field.to_s.split(/\s+/) - current_dir = nil - # if the user didn't specify the sort direction, detect the previous - # sort direction on this field and invert it. - if neither_asc_nor_desc?(new_dir) - if existing_sort = search.sorts.detect { |s| s.name == attr_name } - current_dir = existing_sort.dir - end - new_dir = - if current_dir - direction_text(current_dir) - elsif default_order_is_a_hash - default_order[attr_name] || Constants::ASC - else - default_order || Constants::ASC - end + if no_sort_direction_specified?(new_dir) + new_dir = detect_previous_sort_direction_and_invert_it(attr_name) end - a << "#{attr_name} #{new_dir}" + _ << "#{attr_name} #{new_dir}" end end - def build_url_for(routing_proxy, options_for_url) - if routing_proxy && respond_to?(routing_proxy) - send(routing_proxy).url_for(options_for_url) + def detect_previous_sort_direction_and_invert_it(attr_name) + sort_dir = existing_sort_direction(attr_name) + if sort_dir + direction_text(sort_dir) else - url_for(options_for_url) + default_order(attr_name) || Constants::ASC end end - def link_name(label_text, dir, hide_indicator) - [ERB::Util.h(label_text), order_indicator_for(dir, hide_indicator)] - .compact - .join(Constants::NON_BREAKING_SPACE) - .html_safe + def existing_sort_direction(attr_name = @field_name) + if _ = @search_object.sorts.detect { |s| s.name == attr_name } + _.dir + end + end + + def default_order(attr_name) + Hash === @default_order ? @default_order[attr_name] : @default_order end - def order_indicator_for(dir, hide_indicator) - if hide_indicator || neither_asc_nor_desc?(dir) + def order_indicator + if @hide_indicator || no_sort_direction_specified? nil else - direction_arrow(dir) + direction_arrow end end - def neither_asc_nor_desc?(dir) + def no_sort_direction_specified?(dir = @current_dir) Constants::ASC_DESC.none? { |d| d == dir } end - def direction_arrow(dir) - if dir == Constants::DESC + def direction_arrow + if @current_dir == Constants::DESC Constants::DESC_ARROW else Constants::ASC_ARROW