Skip to content
This repository
Browse code

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
  • Loading branch information...
commit 50725cec397d4fa0ecf1dda4e6ae845a993f1ba7 1 parent eafa58b
Carlos Antonio da Silva authored March 03, 2012
30  activerecord/lib/active_record/autosave_association.rb
@@ -191,23 +191,21 @@ def add_autosave_association_callbacks(reflection)
191 191
             # Doesn't use after_save as that would save associations added in after_create/after_update twice
192 192
             after_create save_method
193 193
             after_update save_method
  194
+          elsif reflection.macro == :has_one
  195
+            define_method(save_method) { save_has_one_association(reflection) }
  196
+            # Configures two callbacks instead of a single after_save so that
  197
+            # the model may rely on their execution order relative to its
  198
+            # own callbacks.
  199
+            #
  200
+            # For example, given that after_creates run before after_saves, if
  201
+            # we configured instead an after_save there would be no way to fire
  202
+            # a custom after_create callback after the child association gets
  203
+            # created.
  204
+            after_create save_method
  205
+            after_update save_method
194 206
           else
195  
-            if reflection.macro == :has_one
196  
-              define_method(save_method) { save_has_one_association(reflection) }
197  
-              # Configures two callbacks instead of a single after_save so that
198  
-              # the model may rely on their execution order relative to its
199  
-              # own callbacks.
200  
-              #
201  
-              # For example, given that after_creates run before after_saves, if
202  
-              # we configured instead an after_save there would be no way to fire
203  
-              # a custom after_create callback after the child association gets
204  
-              # created.
205  
-              after_create save_method
206  
-              after_update save_method
207  
-            else
208  
-              define_non_cyclic_method(save_method, reflection) { save_belongs_to_association(reflection) }
209  
-              before_save save_method
210  
-            end
  207
+            define_non_cyclic_method(save_method, reflection) { save_belongs_to_association(reflection) }
  208
+            before_save save_method
211 209
           end
212 210
         end
213 211
 
2  activerecord/lib/active_record/counter_cache.rb
@@ -71,7 +71,7 @@ def update_counters(id, counters)
71 71
 
72 72
       IdentityMap.remove_by_id(symbolized_base_class, id) if IdentityMap.enabled?
73 73
 
74  
-      update_all(updates.join(', '), primary_key => id )
  74
+      update_all(updates.join(', '), primary_key => id)
75 75
     end
76 76
 
77 77
     # Increment a number field by one, usually representing a count.
42  activerecord/lib/active_record/dynamic_matchers.rb
... ...
@@ -1,13 +1,10 @@
1 1
 module ActiveRecord
2 2
   module DynamicMatchers
3 3
     def respond_to?(method_id, include_private = false)
4  
-      if match = DynamicFinderMatch.match(method_id)
5  
-        return true if all_attributes_exists?(match.attribute_names)
6  
-      elsif match = DynamicScopeMatch.match(method_id)
7  
-        return true if all_attributes_exists?(match.attribute_names)
8  
-      end
  4
+      match       = find_dynamic_match(method_id)
  5
+      valid_match = match && all_attributes_exists?(match.attribute_names)
9 6
 
10  
-      super
  7
+      valid_match || super
11 8
     end
12 9
 
13 10
     private
@@ -22,22 +19,18 @@ def respond_to?(method_id, include_private = false)
22 19
     # Each dynamic finder using <tt>scoped_by_*</tt> is also defined in the class after it
23 20
     # is first invoked, so that future attempts to use it do not run through method_missing.
24 21
     def method_missing(method_id, *arguments, &block)
25  
-      if match = (DynamicFinderMatch.match(method_id) || DynamicScopeMatch.match(method_id))
  22
+      if match = find_dynamic_match(method_id)
26 23
         attribute_names = match.attribute_names
27 24
         super unless all_attributes_exists?(attribute_names)
  25
+
28 26
         unless match.valid_arguments?(arguments)
29 27
           method_trace = "#{__FILE__}:#{__LINE__}:in `#{method_id}'"
30 28
           backtrace = [method_trace] + caller
31 29
           raise ArgumentError, "wrong number of arguments (#{arguments.size} for #{attribute_names.size})", backtrace
32 30
         end
  31
+
33 32
         if match.respond_to?(:scope?) && match.scope?
34  
-          self.class_eval <<-METHOD, __FILE__, __LINE__ + 1
35  
-            def self.#{method_id}(*args)                                    # def self.scoped_by_user_name_and_password(*args)
36  
-              attributes = Hash[[:#{attribute_names.join(',:')}].zip(args)] #   attributes = Hash[[:user_name, :password].zip(args)]
37  
-                                                                            #
38  
-              scoped(:conditions => attributes)                             #   scoped(:conditions => attributes)
39  
-            end                                                             # end
40  
-          METHOD
  33
+          define_scope_method(method_id, attribute_names)
41 34
           send(method_id, *arguments)
42 35
         elsif match.finder?
43 36
           options = arguments.extract_options!
@@ -51,17 +44,30 @@ def self.#{method_id}(*args)                                    # def self.scope
51 44
       end
52 45
     end
53 46
 
  47
+    def define_scope_method(method_id, attribute_names) #:nodoc
  48
+      self.class_eval <<-METHOD, __FILE__, __LINE__ + 1
  49
+        def self.#{method_id}(*args)                                    # def self.scoped_by_user_name_and_password(*args)
  50
+          conditions = Hash[[:#{attribute_names.join(',:')}].zip(args)] #   conditions = Hash[[:user_name, :password].zip(args)]
  51
+          where(conditions)                                             #   where(conditions)
  52
+        end                                                             # end
  53
+      METHOD
  54
+    end
  55
+
  56
+    def find_dynamic_match(method_id) #:nodoc:
  57
+      DynamicFinderMatch.match(method_id) || DynamicScopeMatch.match(method_id)
  58
+    end
  59
+
54 60
     # Similar in purpose to +expand_hash_conditions_for_aggregates+.
55 61
     def expand_attribute_names_for_aggregates(attribute_names)
56  
-      attribute_names.map { |attribute_name|
57  
-        unless (aggregation = reflect_on_aggregation(attribute_name.to_sym)).nil?
  62
+      attribute_names.map do |attribute_name|
  63
+        if aggregation = reflect_on_aggregation(attribute_name.to_sym)
58 64
           aggregate_mapping(aggregation).map do |field_attr, _|
59 65
             field_attr.to_sym
60 66
           end
61 67
         else
62 68
           attribute_name.to_sym
63 69
         end
64  
-      }.flatten
  70
+      end.flatten
65 71
     end
66 72
 
67 73
     def all_attributes_exists?(attribute_names)
@@ -73,7 +79,5 @@ def aggregate_mapping(reflection)
73 79
       mapping = reflection.options[:mapping] || [reflection.name, reflection.name]
74 80
       mapping.first.is_a?(Array) ? mapping : [mapping]
75 81
     end
76  
-
77  
-
78 82
   end
79 83
 end
7  activerecord/lib/active_record/dynamic_scope_match.rb
@@ -7,9 +7,12 @@ module ActiveRecord
7 7
   # chain more <tt>scoped_by_* </tt> methods after the other. It acts like a named
8 8
   # scope except that it's dynamic.
9 9
   class DynamicScopeMatch
  10
+    METHOD_PATTERN = /^scoped_by_([_a-zA-Z]\w*)$/
  11
+
10 12
     def self.match(method)
11  
-      return unless method.to_s =~ /^scoped_by_([_a-zA-Z]\w*)$/
12  
-      new(true, $1 && $1.split('_and_'))
  13
+      if method.to_s =~ METHOD_PATTERN
  14
+        new(true, $1 && $1.split('_and_'))
  15
+      end
13 16
     end
14 17
 
15 18
     def initialize(scope, attribute_names)
11  activerecord/lib/active_record/persistence.rb
@@ -176,7 +176,7 @@ def becomes(klass)
176 176
     #
177 177
     def update_attribute(name, value)
178 178
       name = name.to_s
179  
-      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
  179
+      verify_readonly_attribute(name)
180 180
       send("#{name}=", value)
181 181
       save(:validate => false)
182 182
     end
@@ -191,7 +191,7 @@ def update_attribute(name, value)
191 191
     # attribute is marked as readonly.
192 192
     def update_column(name, value)
193 193
       name = name.to_s
194  
-      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
  194
+      verify_readonly_attribute(name)
195 195
       raise ActiveRecordError, "can not update on a new record object" unless persisted?
196 196
       raw_write_attribute(name, value)
197 197
       self.class.update_all({ name => value }, self.class.primary_key => id) == 1
@@ -323,7 +323,8 @@ def touch(name = nil)
323 323
         changes = {}
324 324
 
325 325
         attributes.each do |column|
326  
-          changes[column.to_s] = write_attribute(column.to_s, current_time)
  326
+          column = column.to_s
  327
+          changes[column] = write_attribute(column, current_time)
327 328
         end
328 329
 
329 330
         changes[self.class.locking_column] = increment_lock if locking_enabled?
@@ -369,5 +370,9 @@ def create
369 370
       @new_record = false
370 371
       id
371 372
     end
  373
+
  374
+    def verify_readonly_attribute(name)
  375
+      raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
  376
+    end
372 377
   end
373 378
 end
18  activerecord/lib/active_record/reflection.rb
@@ -23,11 +23,11 @@ module Reflection # :nodoc:
23 23
     module ClassMethods
24 24
       def create_reflection(macro, name, options, active_record)
25 25
         case macro
26  
-          when :has_many, :belongs_to, :has_one, :has_and_belongs_to_many
27  
-            klass = options[:through] ? ThroughReflection : AssociationReflection
28  
-            reflection = klass.new(macro, name, options, active_record)
29  
-          when :composed_of
30  
-            reflection = AggregateReflection.new(macro, name, options, active_record)
  26
+        when :has_many, :belongs_to, :has_one, :has_and_belongs_to_many
  27
+          klass = options[:through] ? ThroughReflection : AssociationReflection
  28
+          reflection = klass.new(macro, name, options, active_record)
  29
+        when :composed_of
  30
+          reflection = AggregateReflection.new(macro, name, options, active_record)
31 31
         end
32 32
 
33 33
         self.reflections = self.reflections.merge(name => reflection)
@@ -44,7 +44,8 @@ def reflect_on_all_aggregations
44 44
       #   Account.reflect_on_aggregation(:balance) # => the balance AggregateReflection
45 45
       #
46 46
       def reflect_on_aggregation(aggregation)
47  
-        reflections[aggregation].is_a?(AggregateReflection) ? reflections[aggregation] : nil
  47
+        reflection = reflections[aggregation]
  48
+        reflection if reflection.is_a?(AggregateReflection)
48 49
       end
49 50
 
50 51
       # Returns an array of AssociationReflection objects for all the
@@ -68,7 +69,8 @@ def reflect_on_all_associations(macro = nil)
68 69
       #   Invoice.reflect_on_association(:line_items).macro  # returns :has_many
69 70
       #
70 71
       def reflect_on_association(association)
71  
-        reflections[association].is_a?(AssociationReflection) ? reflections[association] : nil
  72
+        reflection = reflections[association]
  73
+        reflection if reflection.is_a?(AssociationReflection)
72 74
       end
73 75
 
74 76
       # Returns an array of AssociationReflection objects for all associations which have <tt>:autosave</tt> enabled.
@@ -77,7 +79,6 @@ def reflect_on_all_autosave_associations
77 79
       end
78 80
     end
79 81
 
80  
-
81 82
     # Abstract base class for AggregateReflection and AssociationReflection. Objects of
82 83
     # AggregateReflection and AssociationReflection are returned by the Reflection::ClassMethods.
83 84
     class MacroReflection
@@ -452,7 +453,6 @@ def conditions
452 453
           # Recursively fill out the rest of the array from the through reflection
453 454
           conditions += through_conditions
454 455
 
455  
-          # And return
456 456
           conditions
457 457
         end
458 458
       end
27  activerecord/lib/active_record/relation/predicate_builder.rb
... ...
@@ -1,7 +1,7 @@
1 1
 module ActiveRecord
2 2
   class PredicateBuilder # :nodoc:
3 3
     def self.build_from_hash(engine, attributes, default_table)
4  
-      predicates = attributes.map do |column, value|
  4
+      attributes.map do |column, value|
5 5
         table = default_table
6 6
 
7 7
         if value.is_a?(Hash)
@@ -17,20 +17,18 @@ def self.build_from_hash(engine, attributes, default_table)
17 17
 
18 18
           build(table[column.to_sym], value)
19 19
         end
20  
-      end
21  
-      predicates.flatten
  20
+      end.flatten
22 21
     end
23 22
 
24 23
     def self.references(attributes)
25  
-      references = attributes.map do |key, value|
  24
+      attributes.map do |key, value|
26 25
         if value.is_a?(Hash)
27 26
           key
28 27
         else
29 28
           key = key.to_s
30 29
           key.split('.').first.to_sym if key.include?('.')
31 30
         end
32  
-      end
33  
-      references.compact
  31
+      end.compact
34 32
     end
35 33
 
36 34
     private
@@ -43,23 +41,24 @@ def self.build(attribute, value)
43 41
           values = value.to_a.map {|x| x.is_a?(ActiveRecord::Model) ? x.id : x}
44 42
           ranges, values = values.partition {|v| v.is_a?(Range) || v.is_a?(Arel::Relation)}
45 43
 
46  
-          array_predicates = ranges.map {|range| attribute.in(range)}
47  
-
48  
-          if values.include?(nil)
  44
+          values_predicate = if values.include?(nil)
49 45
             values = values.compact
  46
+
50 47
             case values.length
51 48
             when 0
52  
-              array_predicates << attribute.eq(nil)
  49
+              attribute.eq(nil)
53 50
             when 1
54  
-              array_predicates << attribute.eq(values.first).or(attribute.eq(nil))
  51
+              attribute.eq(values.first).or(attribute.eq(nil))
55 52
             else
56  
-              array_predicates << attribute.in(values).or(attribute.eq(nil))
  53
+              attribute.in(values).or(attribute.eq(nil))
57 54
             end
58 55
           else
59  
-            array_predicates << attribute.in(values)
  56
+            attribute.in(values)
60 57
           end
61 58
 
62  
-          array_predicates.inject {|composite, predicate| composite.or(predicate)}
  59
+          array_predicates = ranges.map { |range| attribute.in(range) }
  60
+          array_predicates << values_predicate
  61
+          array_predicates.inject { |composite, predicate| composite.or(predicate) }
63 62
         when Range, Arel::Relation
64 63
           attribute.in(value)
65 64
         when ActiveRecord::Model
8  activerecord/lib/active_record/sanitization.rb
@@ -37,9 +37,9 @@ def sanitize_sql_for_conditions(condition, table_name = self.table_name)
37 37
       #   { :name => nil, :group_id => 4 }  returns "name = NULL , group_id='4'"
38 38
       def sanitize_sql_for_assignment(assignments)
39 39
         case assignments
40  
-          when Array; sanitize_sql_array(assignments)
41  
-          when Hash;  sanitize_sql_hash_for_assignment(assignments)
42  
-          else        assignments
  40
+        when Array; sanitize_sql_array(assignments)
  41
+        when Hash;  sanitize_sql_hash_for_assignment(assignments)
  42
+        else        assignments
43 43
         end
44 44
       end
45 45
 
@@ -57,7 +57,7 @@ def sanitize_sql_for_assignment(assignments)
57 57
       def expand_hash_conditions_for_aggregates(attrs)
58 58
         expanded_attrs = {}
59 59
         attrs.each do |attr, value|
60  
-          unless (aggregation = reflect_on_aggregation(attr.to_sym)).nil?
  60
+          if aggregation = reflect_on_aggregation(attr.to_sym)
61 61
             mapping = aggregate_mapping(aggregation)
62 62
             mapping.each do |field_attr, aggregate_attr|
63 63
               if mapping.size == 1 && !value.respond_to?(aggregate_attr)

0 notes on commit 50725ce

Please sign in to comment.
Something went wrong with that request. Please try again.