Merge pull request #18845 from bogdan/remove-code-dups-in-action-view

Remove some code duplication in ActionView tags code

2 files changed, 25 insertions, 33 deletions

diff --git a/actionview/lib/action_view/helpers/tags/base.rb b/actionview/lib/action_view/helpers/tags/base.rb
index 7740c60eac..acc6443a96 100644
--- a/actionview/lib/action_view/helpers/tags/base.rb
+++ b/actionview/lib/action_view/helpers/tags/base.rb
@@ -14,7 +14,7 @@ module ActionView
           @object_name.sub!(/\[\]$/,"") || @object_name.sub!(/\[\]\]$/,"]")
           @object = retrieve_object(options.delete(:object))
           @options = options
-          @auto_index = retrieve_autoindex(Regexp.last_match.pre_match) if Regexp.last_match
+          @auto_index = Regexp.last_match ? retrieve_autoindex(Regexp.last_match.pre_match) : nil
         end
 
         # This is what child classes implement.
@@ -79,35 +79,30 @@ module ActionView
         end
 
         def add_default_name_and_id(options)
-          if options.has_key?("index")
-            options["name"] ||= options.fetch("name"){ tag_name_with_index(options["index"], options["multiple"]) }
-            options["id"] = options.fetch("id"){ tag_id_with_index(options["index"]) }
-            options.delete("index")
-          elsif defined?(@auto_index)
-            options["name"] ||= options.fetch("name"){ tag_name_with_index(@auto_index, options["multiple"]) }
-            options["id"] = options.fetch("id"){ tag_id_with_index(@auto_index) }
-          else
-            options["name"] ||= options.fetch("name"){ tag_name(options["multiple"]) }
-            options["id"] = options.fetch("id"){ tag_id }
+          index = name_and_id_index(options)
+          options["name"] = options.fetch("name"){ tag_name(options["multiple"], index) }
+          options["id"] = options.fetch("id"){ tag_id(index) }
+          if namespace = options.delete("namespace")
+            options['id'] = options['id'] ? "#{namespace}_#{options['id']}" : namespace
           end
-
-          options["id"] = [options.delete('namespace'), options["id"]].compact.join("_").presence
-        end
-
-        def tag_name(multiple = false)
-          "#{@object_name}[#{sanitized_method_name}]#{"[]" if multiple}"
-        end
-
-        def tag_name_with_index(index, multiple = false)
-          "#{@object_name}[#{index}][#{sanitized_method_name}]#{"[]" if multiple}"
         end
 
-        def tag_id
-          "#{sanitized_object_name}_#{sanitized_method_name}"
+        def tag_name(multiple = false, index = nil)
+          # a little duplication to construct less strings
+          if index
+            "#{@object_name}[#{index}][#{sanitized_method_name}]#{"[]" if multiple}"
+          else
+            "#{@object_name}[#{sanitized_method_name}]#{"[]" if multiple}"
+          end
         end
 
-        def tag_id_with_index(index)
-          "#{sanitized_object_name}_#{index}_#{sanitized_method_name}"
+        def tag_id(index = nil)
+          # a little duplication to construct less strings
+          if index
+            "#{sanitized_object_name}_#{index}_#{sanitized_method_name}"
+          else
+            "#{sanitized_object_name}_#{sanitized_method_name}"
+          end
         end
 
         def sanitized_object_name
@@ -149,6 +144,10 @@ module ActionView
           end
           option_tags
         end
+
+        def name_and_id_index(options)
+          options.key?("index") ?  options.delete("index") || "" : @auto_index
+        end
       end
     end
   end
diff --git a/actionview/lib/action_view/helpers/tags/collection_check_boxes.rb b/actionview/lib/action_view/helpers/tags/collection_check_boxes.rb
index 6242a2a085..1765fa6558 100644
--- a/actionview/lib/action_view/helpers/tags/collection_check_boxes.rb
+++ b/actionview/lib/action_view/helpers/tags/collection_check_boxes.rb
@@ -41,14 +41,7 @@ module ActionView
         end
 
         def hidden_field
-          hidden_name = @html_options[:name]
-
-          hidden_name ||= if @options.has_key?(:index)
-            "#{tag_name_with_index(@options[:index])}[]"
-          else
-            "#{tag_name}[]"
-          end
-
+          hidden_name = @html_options[:name] || "#{tag_name(false, @options[:index])}[]"
           @template_object.hidden_field_tag(hidden_name, "", id: nil)
         end
       end