Merge pull request #5255 from carlosantoniodasilva/active-record-review

Refactor and cleanup in some ActiveRecord modules

8 files changed, 77 insertions, 68 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 4bafadc666..eb3ae7014e 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -191,23 +191,21 @@ 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
+          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
+            # own callbacks.
+            #
+            # For example, given that after_creates run before after_saves, if
+            # we configured instead an after_save there would be no way to fire
+            # a custom after_create callback after the child association gets
+            # created.
+            after_create save_method
+            after_update save_method
           else
-            if 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
-              # own callbacks.
-              #
-              # For example, given that after_creates run before after_saves, if
-              # we configured instead an after_save there would be no way to fire
-              # a custom after_create callback after the child association gets
-              # created.
-              after_create save_method
-              after_update save_method
-            else
-              define_non_cyclic_method(save_method, reflection) { save_belongs_to_association(reflection) }
-              before_save save_method
-            end
+            define_non_cyclic_method(save_method, reflection) { save_belongs_to_association(reflection) }
+            before_save save_method
           end
         end
 
diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index 224f5276eb..8d5388e1f3 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/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.
diff --git a/activerecord/lib/active_record/dynamic_matchers.rb b/activerecord/lib/active_record/dynamic_matchers.rb
index 60ce3dd4f1..e35b1c91a0 100644
--- a/activerecord/lib/active_record/dynamic_matchers.rb
+++ b/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
diff --git a/activerecord/lib/active_record/dynamic_scope_match.rb b/activerecord/lib/active_record/dynamic_scope_match.rb
index a502155aac..6c043d29c4 100644
--- a/activerecord/lib/active_record/dynamic_scope_match.rb
+++ b/activerecord/lib/active_record/dynamic_scope_match.rb
@@ -7,9 +7,12 @@ 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*)$/
-      new(true, $1 && $1.split('_and_'))
+      if method.to_s =~ METHOD_PATTERN
+        new(true, $1 && $1.split('_and_'))
+      end
     end
 
     def initialize(scope, attribute_names)
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 9bc046c775..c4bce87311 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/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
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 8f8a3ec3bb..d4f4d593c6 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -23,11 +23,11 @@ module ActiveRecord
     module ClassMethods
       def create_reflection(macro, name, options, active_record)
         case macro
-          when :has_many, :belongs_to, :has_one, :has_and_belongs_to_many
-            klass = options[:through] ? ThroughReflection : AssociationReflection
-            reflection = klass.new(macro, name, options, active_record)
-          when :composed_of
-            reflection = AggregateReflection.new(macro, name, options, active_record)
+        when :has_many, :belongs_to, :has_one, :has_and_belongs_to_many
+          klass = options[:through] ? ThroughReflection : AssociationReflection
+          reflection = klass.new(macro, name, options, active_record)
+        when :composed_of
+          reflection = AggregateReflection.new(macro, name, options, active_record)
         end
 
         self.reflections = self.reflections.merge(name => reflection)
@@ -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
diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb
index 1d04e763f6..1088773bc7 100644
--- a/activerecord/lib/active_record/relation/predicate_builder.rb
+++ b/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
diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb
index 2d7d83d160..81b13fe529 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -37,9 +37,9 @@ module ActiveRecord
       #   { :name => nil, :group_id => 4 }  returns "name = NULL , group_id='4'"
       def sanitize_sql_for_assignment(assignments)
         case assignments
-          when Array; sanitize_sql_array(assignments)
-          when Hash;  sanitize_sql_hash_for_assignment(assignments)
-          else        assignments
+        when Array; sanitize_sql_array(assignments)
+        when Hash;  sanitize_sql_hash_for_assignment(assignments)
+        else        assignments
         end
       end
 
@@ -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)