Refactor and cleanup in some ActiveRecord modules

* Avoid double hash lookups in AR::Reflection when reflecting associations/aggregations
* Minor cleanups: use elsif, do..end, if..else instead of unless..else
* Simplify DynamicMatchers#respond_to?
* Use "where" instead of scoped with conditions hash
* Extract `scoped_by` method pattern regexp to constant
* Extract noisy class_eval from method_missing in dynamic matchers
* Extract readonly check, avoid calling column#to_s twice in persistence
* Refactor predicate builder, remove some variables

activerecord/lib/active_record/autosave_association.rb

@@ -191,8 +191,7 @@ module ActiveRecord
             # Doesn't use after_save as that would save associations added in after_create/after_update twice
             after_create save_method
             after_update save_method
-          else
-            if reflection.macro == :has_one
+          elsif reflection.macro == :has_one
             define_method(save_method) { save_has_one_association(reflection) }
             # Configures two callbacks instead of a single after_save so that
             # the model may rely on their execution order relative to its
@@ -209,7 +208,6 @@ module ActiveRecord
             before_save save_method
           end
         end
-        end
 
         if reflection.validate? && !method_defined?(validation_method)
           method = (collection ? :validate_collection_association : :validate_single_association)

activerecord/lib/active_record/counter_cache.rb

@@ -71,7 +71,7 @@ module ActiveRecord
 
       IdentityMap.remove_by_id(symbolized_base_class, id) if IdentityMap.enabled?
 
-      update_all(updates.join(', '), primary_key => id )
+      update_all(updates.join(', '), primary_key => id)
     end
 
     # Increment a number field by one, usually representing a count.

activerecord/lib/active_record/dynamic_matchers.rb

@@ -1,13 +1,10 @@
 module ActiveRecord
   module DynamicMatchers
     def respond_to?(method_id, include_private = false)
-      if match = DynamicFinderMatch.match(method_id)
-        return true if all_attributes_exists?(match.attribute_names)
-      elsif match = DynamicScopeMatch.match(method_id)
-        return true if all_attributes_exists?(match.attribute_names)
-      end
+      match       = find_dynamic_match(method_id)
+      valid_match = match && all_attributes_exists?(match.attribute_names)
 
-      super
+      valid_match || super
     end
 
     private
@@ -22,22 +19,18 @@ module ActiveRecord
     # Each dynamic finder using <tt>scoped_by_*</tt> is also defined in the class after it
     # is first invoked, so that future attempts to use it do not run through method_missing.
     def method_missing(method_id, *arguments, &block)
-      if match = (DynamicFinderMatch.match(method_id) || DynamicScopeMatch.match(method_id))
+      if match = find_dynamic_match(method_id)
         attribute_names = match.attribute_names
         super unless all_attributes_exists?(attribute_names)
+
         unless match.valid_arguments?(arguments)
           method_trace = "#{__FILE__}:#{__LINE__}:in `#{method_id}'"
           backtrace = [method_trace] + caller
           raise ArgumentError, "wrong number of arguments (#{arguments.size} for #{attribute_names.size})", backtrace
         end
+
         if match.respond_to?(:scope?) && match.scope?
-          self.class_eval <<-METHOD, __FILE__, __LINE__ + 1
-            def self.#{method_id}(*args)                                    # def self.scoped_by_user_name_and_password(*args)
-              attributes = Hash[[:#{attribute_names.join(',:')}].zip(args)] #   attributes = Hash[[:user_name, :password].zip(args)]
-                                                                            #
-              scoped(:conditions => attributes)                             #   scoped(:conditions => attributes)
-            end                                                             # end
-          METHOD
+          define_scope_method(method_id, attribute_names)
           send(method_id, *arguments)
         elsif match.finder?
           options = arguments.extract_options!
@@ -51,17 +44,30 @@ module ActiveRecord
       end
     end
 
+    def define_scope_method(method_id, attribute_names) #:nodoc
+      self.class_eval <<-METHOD, __FILE__, __LINE__ + 1
+        def self.#{method_id}(*args)                                    # def self.scoped_by_user_name_and_password(*args)
+          conditions = Hash[[:#{attribute_names.join(',:')}].zip(args)] #   conditions = Hash[[:user_name, :password].zip(args)]
+          where(conditions)                                             #   where(conditions)
+        end                                                             # end
+      METHOD
+    end
+
+    def find_dynamic_match(method_id) #:nodoc:
+      DynamicFinderMatch.match(method_id) || DynamicScopeMatch.match(method_id)
+    end
+
     # Similar in purpose to +expand_hash_conditions_for_aggregates+.
     def expand_attribute_names_for_aggregates(attribute_names)
-      attribute_names.map { |attribute_name|
-        unless (aggregation = reflect_on_aggregation(attribute_name.to_sym)).nil?
+      attribute_names.map do |attribute_name|
+        if aggregation = reflect_on_aggregation(attribute_name.to_sym)
           aggregate_mapping(aggregation).map do |field_attr, _|
             field_attr.to_sym
           end
         else
           attribute_name.to_sym
         end
-      }.flatten
+      end.flatten
     end
 
     def all_attributes_exists?(attribute_names)
@@ -73,7 +79,5 @@ module ActiveRecord
       mapping = reflection.options[:mapping] || [reflection.name, reflection.name]
       mapping.first.is_a?(Array) ? mapping : [mapping]
     end
-
-
   end
 end

activerecord/lib/active_record/dynamic_scope_match.rb

@@ -7,10 +7,13 @@ module ActiveRecord
   # chain more <tt>scoped_by_* </tt> methods after the other. It acts like a named
   # scope except that it's dynamic.
   class DynamicScopeMatch
+    METHOD_PATTERN = /^scoped_by_([_a-zA-Z]\w*)$/
+
     def self.match(method)
-      return unless method.to_s =~ /^scoped_by_([_a-zA-Z]\w*)$/
+      if method.to_s =~ METHOD_PATTERN
         new(true, $1 && $1.split('_and_'))
       end
+    end
 
     def initialize(scope, attribute_names)
       @scope           = scope

activerecord/lib/active_record/persistence.rb

@@ -176,7 +176,7 @@ module ActiveRecord
     #
     def update_attribute(name, value)
       name = name.to_s
-      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
+      verify_readonly_attribute(name)
       send("#{name}=", value)
       save(:validate => false)
     end
@@ -191,7 +191,7 @@ module ActiveRecord
     # attribute is marked as readonly.
     def update_column(name, value)
       name = name.to_s
-      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
+      verify_readonly_attribute(name)
       raise ActiveRecordError, "can not update on a new record object" unless persisted?
       raw_write_attribute(name, value)
       self.class.update_all({ name => value }, self.class.primary_key => id) == 1
@@ -323,7 +323,8 @@ module ActiveRecord
         changes = {}
 
         attributes.each do |column|
-          changes[column.to_s] = write_attribute(column.to_s, current_time)
+          column = column.to_s
+          changes[column] = write_attribute(column, current_time)
         end
 
         changes[self.class.locking_column] = increment_lock if locking_enabled?
@@ -369,5 +370,9 @@ module ActiveRecord
       @new_record = false
       id
     end
+
+    def verify_readonly_attribute(name)
+      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
+    end
   end
 end

activerecord/lib/active_record/reflection.rb

@@ -44,7 +44,8 @@ module ActiveRecord
       #   Account.reflect_on_aggregation(:balance) # => the balance AggregateReflection
       #
       def reflect_on_aggregation(aggregation)
-        reflections[aggregation].is_a?(AggregateReflection) ? reflections[aggregation] : nil
+        reflection = reflections[aggregation]
+        reflection if reflection.is_a?(AggregateReflection)
       end
 
       # Returns an array of AssociationReflection objects for all the
@@ -68,7 +69,8 @@ module ActiveRecord
       #   Invoice.reflect_on_association(:line_items).macro  # returns :has_many
       #
       def reflect_on_association(association)
-        reflections[association].is_a?(AssociationReflection) ? reflections[association] : nil
+        reflection = reflections[association]
+        reflection if reflection.is_a?(AssociationReflection)
       end
 
       # Returns an array of AssociationReflection objects for all associations which have <tt>:autosave</tt> enabled.
@@ -77,7 +79,6 @@ module ActiveRecord
       end
     end
 
-
     # Abstract base class for AggregateReflection and AssociationReflection. Objects of
     # AggregateReflection and AssociationReflection are returned by the Reflection::ClassMethods.
     class MacroReflection
@@ -452,7 +453,6 @@ module ActiveRecord
           # Recursively fill out the rest of the array from the through reflection
           conditions += through_conditions
 
-          # And return
           conditions
         end
       end

activerecord/lib/active_record/relation/predicate_builder.rb

@@ -1,7 +1,7 @@
 module ActiveRecord
   class PredicateBuilder # :nodoc:
     def self.build_from_hash(engine, attributes, default_table)
-      predicates = attributes.map do |column, value|
+      attributes.map do |column, value|
         table = default_table
 
         if value.is_a?(Hash)
@@ -17,20 +17,18 @@ module ActiveRecord
 
           build(table[column.to_sym], value)
         end
-      end
-      predicates.flatten
+      end.flatten
     end
 
     def self.references(attributes)
-      references = attributes.map do |key, value|
+      attributes.map do |key, value|
         if value.is_a?(Hash)
           key
         else
           key = key.to_s
           key.split('.').first.to_sym if key.include?('.')
         end
-      end
-      references.compact
+      end.compact
     end
 
     private
@@ -43,23 +41,24 @@ module ActiveRecord
           values = value.to_a.map {|x| x.is_a?(ActiveRecord::Model) ? x.id : x}
           ranges, values = values.partition {|v| v.is_a?(Range) || v.is_a?(Arel::Relation)}
 
-          array_predicates = ranges.map {|range| attribute.in(range)}
-
-          if values.include?(nil)
+          values_predicate = if values.include?(nil)
             values = values.compact
+
             case values.length
             when 0
-              array_predicates << attribute.eq(nil)
+              attribute.eq(nil)
             when 1
-              array_predicates << attribute.eq(values.first).or(attribute.eq(nil))
+              attribute.eq(values.first).or(attribute.eq(nil))
             else
-              array_predicates << attribute.in(values).or(attribute.eq(nil))
+              attribute.in(values).or(attribute.eq(nil))
             end
           else
-            array_predicates << attribute.in(values)
+            attribute.in(values)
           end
 
-          array_predicates.inject {|composite, predicate| composite.or(predicate)}
+          array_predicates = ranges.map { |range| attribute.in(range) }
+          array_predicates << values_predicate
+          array_predicates.inject { |composite, predicate| composite.or(predicate) }
         when Range, Arel::Relation
           attribute.in(value)
         when ActiveRecord::Model

activerecord/lib/active_record/sanitization.rb

@@ -57,7 +57,7 @@ module ActiveRecord
       def expand_hash_conditions_for_aggregates(attrs)
         expanded_attrs = {}
         attrs.each do |attr, value|
-          unless (aggregation = reflect_on_aggregation(attr.to_sym)).nil?
+          if aggregation = reflect_on_aggregation(attr.to_sym)
             mapping = aggregate_mapping(aggregation)
             mapping.each do |field_attr, aggregate_attr|
               if mapping.size == 1 && !value.respond_to?(aggregate_attr)