Break conditional branches into separate methods

This breaks the two branches of the `if reflection.last` and `else`
to clearer see where the two methods can be refactored. Eventually
we hope to remove the need for these separated methods altogether.

Move the first branch outside the loop

This code doesn't need to be in the loop because it it always affects
the last chain. `get_bind_values` and `add_constraints` must match
in this context because `get_bind_values` is the caching of `add_constraints`

Use each_cons to remove need for `chain[i + 1]`

The `chain[i + 1]` is confusing because it's not immediately obvious
what it's trying to achieve. The use of `each_cons` makes it clear
we need to get the `next_reflection`.

1 files changed, 52 insertions, 35 deletions

diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb
index 4c47af8cb0..26fed22b4c 100644
--- a/activerecord/lib/active_record/associations/association_scope.rb
+++ b/activerecord/lib/active_record/associations/association_scope.rb
@@ -45,20 +45,20 @@ module ActiveRecord
       end
 
       def self.get_bind_values(owner, chain)
-        bvs = []
-        chain.each_with_index do |reflection, i|
-          if reflection == chain.last
-            bvs << reflection.join_id_for(owner)
-            if reflection.type
-              bvs << owner.class.base_class.name
-            end
-          else
-            if reflection.type
-              bvs << chain[i + 1].klass.base_class.name
-            end
+        binds = []
+        last_reflection = chain.last
+
+        binds << last_reflection.join_id_for(owner)
+        if last_reflection.type
+          binds << owner.class.base_class.name
+        end
+
+        chain.each_cons(2).each do |reflection, next_reflection|
+          if reflection.type
+            binds << next_reflection.klass.base_class.name
           end
         end
-        bvs
+        binds
       end
 
       private
@@ -96,38 +96,55 @@ module ActiveRecord
         bind_value scope, column, value, tracker
       end
 
+      def last_chain_scope(scope, table, reflection, owner, tracker, assoc_klass)
+        join_keys = reflection.join_keys(assoc_klass)
+        key = join_keys.key
+        foreign_key = join_keys.foreign_key
+
+        bind_val = bind scope, table.table_name, key.to_s, owner[foreign_key], tracker
+        scope    = scope.where(table[key].eq(bind_val))
+
+        if reflection.type
+          value    = owner.class.base_class.name
+          bind_val = bind scope, table.table_name, reflection.type, value, tracker
+          scope    = scope.where(table[reflection.type].eq(bind_val))
+        else
+          scope
+        end
+      end
+
+      def next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection)
+        join_keys = reflection.join_keys(assoc_klass)
+        key = join_keys.key
+        foreign_key = join_keys.foreign_key
+
+        constraint = table[key].eq(foreign_table[foreign_key])
+
+        if reflection.type
+          value    = next_reflection.klass.base_class.name
+          bind_val = bind scope, table.table_name, reflection.type, value, tracker
+          scope    = scope.where(table[reflection.type].eq(bind_val))
+        end
+
+        scope = scope.joins(join(foreign_table, constraint))
+      end
+
       def add_constraints(scope, owner, assoc_klass, refl, tracker)
         chain = refl.chain
         scope_chain = refl.scope_chain
 
         tables = construct_tables(chain, assoc_klass, refl, tracker)
 
+        reflection = chain.last
+        table = tables.last
+        scope = last_chain_scope(scope, table, reflection, owner, tracker, assoc_klass)
+
         chain.each_with_index do |reflection, i|
           table, foreign_table = tables.shift, tables.first
 
-          join_keys = reflection.join_keys(assoc_klass)
-          key = join_keys.key
-          foreign_key = join_keys.foreign_key
-
-          if reflection == chain.last
-            bind_val = bind scope, table.table_name, key.to_s, owner[foreign_key], tracker
-            scope    = scope.where(table[key].eq(bind_val))
-
-            if reflection.type
-              value    = owner.class.base_class.name
-              bind_val = bind scope, table.table_name, reflection.type, value, tracker
-              scope    = scope.where(table[reflection.type].eq(bind_val))
-            end
-          else
-            constraint = table[key].eq(foreign_table[foreign_key])
-
-            if reflection.type
-              value    = chain[i + 1].klass.base_class.name
-              bind_val = bind scope, table.table_name, reflection.type, value, tracker
-              scope    = scope.where(table[reflection.type].eq(bind_val))
-            end
-
-            scope = scope.joins(join(foreign_table, constraint))
+          unless reflection == chain.last
+            next_reflection = chain[i + 1]
+            scope = next_chain_scope(scope, table, reflection, chain, tracker, assoc_klass, foreign_table, next_reflection)
           end
 
           is_first_chain = i == 0