Refactor Strategies

This changes Strategy from a class to a module and removes inheritance.
It introduces an Evaluation facade to make building strategies easier

lib/factory_girl.rb

@@ -3,16 +3,22 @@
 require 'factory_girl/errors'
 require 'factory_girl/factory_runner'
 require 'factory_girl/strategy_calculator'
-require 'factory_girl/strategy'
+require "factory_girl/strategy/build"
+require "factory_girl/strategy/create"
+require "factory_girl/strategy/attributes_for"
+require "factory_girl/strategy/stub"
+require "factory_girl/strategy/null"
 require 'factory_girl/registry'
 require 'factory_girl/null_factory'
 require 'factory_girl/null_object'
+require 'factory_girl/evaluation'
 require 'factory_girl/factory'
 require 'factory_girl/attribute_assigner'
 require 'factory_girl/evaluator'
 require 'factory_girl/evaluator_class_definer'
 require 'factory_girl/attribute'
 require 'factory_girl/callback'
+require 'factory_girl/callback_runner'
 require 'factory_girl/declaration_list'
 require 'factory_girl/declaration'
 require 'factory_girl/sequence'

lib/factory_girl/callback_runner.rb

@@ -0,0 +1,20 @@
+module FactoryGirl
+  class CallbackRunner
+    def initialize(callbacks, evaluator)
+      @callbacks = callbacks
+      @evaluator = evaluator
+    end
+
+    def update(name, result_instance)
+      callbacks_by_name(name).each do |callback|
+        callback.run(result_instance, @evaluator)
+      end
+    end
+
+    private
+
+    def callbacks_by_name(name)
+      @callbacks.select {|callback| callback.name == name }
+    end
+  end
+end

lib/factory_girl/evaluation.rb

@@ -0,0 +1,23 @@
+require "observer"
+
+module FactoryGirl
+  class Evaluation
+    include Observable
+
+    def initialize(attribute_assigner, to_create)
+      @attribute_assigner = attribute_assigner
+      @to_create = to_create
+    end
+
+    def create(result_instance)
+      @to_create[result_instance]
+    end
+
+    delegate :object, :hash, to: :@attribute_assigner
+
+    def notify(name, result_instance)
+      changed
+      notify_observers(name, result_instance)
+    end
+  end
+end

lib/factory_girl/evaluator.rb

@@ -3,7 +3,7 @@
 
 module FactoryGirl
   class Evaluator
-    class_attribute :callbacks, :attribute_lists
+    class_attribute :attribute_lists
 
     def self.attribute_list
       AttributeList.new.tap do |list|
@@ -25,8 +25,6 @@ def initialize(build_strategy, overrides = {})
       @overrides.each do |name, value|
         singleton_class.send :define_method, name, lambda { value }
       end
-
-      @build_strategy.add_observer(CallbackRunner.new(self.class.callbacks, self))
     end
 
     def association(factory_name, overrides = {})
@@ -56,20 +54,5 @@ def method_missing(method_name, *args, &block)
     def __overrides
       @overrides
     end
-
-    private
-
-    class CallbackRunner
-      def initialize(callbacks, evaluator)
-        @callbacks = callbacks
-        @evaluator = evaluator
-      end
-
-      def update(name, result_instance)
-        @callbacks.select {|callback| callback.name == name }.each do |callback|
-          callback.run(result_instance, @evaluator)
-        end
-      end
-    end
   end
 end

lib/factory_girl/evaluator_class_definer.rb

@@ -1,8 +1,7 @@
 module FactoryGirl
   class EvaluatorClassDefiner
-    def initialize(attributes, callbacks, parent_class)
+    def initialize(attributes, parent_class)
       @parent_class = parent_class
-      @callbacks    = callbacks
       @attributes   = attributes
 
       attributes.each do |attribute|
@@ -12,8 +11,6 @@ def initialize(attributes, callbacks, parent_class)
 
     def evaluator_class
       @evaluator_class ||= Class.new(@parent_class).tap do |klass|
-        klass.callbacks ||= []
-        klass.callbacks += @callbacks
         klass.attribute_lists ||= []
         klass.attribute_lists += [@attributes]
       end

lib/factory_girl/factory.rb

@@ -35,7 +35,10 @@ def run(strategy_class, overrides, &block) #:nodoc:
       evaluator = evaluator_class.new(strategy, overrides.symbolize_keys)
       attribute_assigner = AttributeAssigner.new(evaluator, &instance_builder)
 
-      strategy.result(attribute_assigner, to_create).tap(&block)
+      evaluation = Evaluation.new(attribute_assigner, to_create)
+      evaluation.add_observer(CallbackRunner.new(callbacks, evaluator))
+
+      strategy.result(evaluation).tap(&block)
     end
 
     def human_names
@@ -97,7 +100,7 @@ def class_name #:nodoc:
     end
 
     def evaluator_class
-      @evaluator_class ||= EvaluatorClassDefiner.new(attributes, callbacks, parent.evaluator_class).evaluator_class
+      @evaluator_class ||= EvaluatorClassDefiner.new(attributes, parent.evaluator_class).evaluator_class
     end
 
     def attributes
@@ -110,7 +113,7 @@ def attributes
     end
 
     def callbacks
-      processing_order.map {|factory| factory.callbacks }.flatten
+      parent.callbacks + processing_order.map {|factory| factory.callbacks }.flatten
     end
 
     def constructor

lib/factory_girl/strategy/attributes_for.rb

@@ -1,12 +1,12 @@
 module FactoryGirl
-  class Strategy #:nodoc:
-    class AttributesFor < Strategy #:nodoc:
+  module Strategy
+    class AttributesFor
       def association(runner)
         runner.run(Strategy::Null)
       end
 
-      def result(attribute_assigner, to_create)
-        attribute_assigner.hash
+      def result(evaluation)
+        evaluation.hash
       end
     end
   end

lib/factory_girl/strategy/build.rb

@@ -1,13 +1,13 @@
 module FactoryGirl
-  class Strategy #:nodoc:
-    class Build < Strategy #:nodoc:
+  module Strategy
+    class Build
       def association(runner)
         runner.run
       end
 
-      def result(attribute_assigner, to_create)
-        attribute_assigner.object.tap do |result_instance|
-          run_callbacks(:after_build, result_instance)
+      def result(evaluation)
+        evaluation.object.tap do |instance|
+          evaluation.notify(:after_build, instance)
         end
       end
     end

lib/factory_girl/strategy/create.rb

@@ -1,16 +1,16 @@
 module FactoryGirl
-  class Strategy #:nodoc:
-    class Create < Strategy #:nodoc:
+  module Strategy
+    class Create
       def association(runner)
         runner.run
       end
 
-      def result(attribute_assigner, to_create)
-        attribute_assigner.object.tap do |result_instance|
-          run_callbacks(:after_build, result_instance)
-          run_callbacks(:before_create, result_instance)
-          to_create[result_instance]
-          run_callbacks(:after_create, result_instance)
+      def result(evaluation)
+        evaluation.object.tap do |instance|
+          evaluation.notify(:after_build, instance)
+          evaluation.notify(:before_create, instance)
+          evaluation.create(instance)
+          evaluation.notify(:after_create, instance)
         end
       end
     end

lib/factory_girl/strategy/null.rb

@@ -1,10 +1,10 @@
 module FactoryGirl
-  class Strategy
+  module Strategy
     class Null
       def association(runner)
       end
 
-      def result(attribute_assigner, to_create)
+      def result(evaluation)
       end
     end
   end

lib/factory_girl/strategy/stub.rb

@@ -1,16 +1,16 @@
 module FactoryGirl
-  class Strategy
-    class Stub < Strategy #:nodoc:
+  module Strategy
+    class Stub
       @@next_id = 1000
 
       def association(runner)
         runner.run(Strategy::Stub)
       end
 
-      def result(attribute_assigner, to_create)
-        attribute_assigner.object.tap do |result_instance|
-          stub_database_interaction_on_result(result_instance)
-          run_callbacks(:after_stub, result_instance)
+      def result(evaluation)
+        evaluation.object.tap do |instance|
+          stub_database_interaction_on_result(instance)
+          evaluation.notify(:after_stub, instance)
         end
       end
 

lib/factory_girl/strategy_calculator.rb

@@ -15,7 +15,7 @@ def strategy
     private
 
     def strategy_is_object?
-      @name_or_object.is_a?(Class) && @name_or_object.ancestors.include?(::FactoryGirl::Strategy)
+      @name_or_object.is_a?(Class)
     end
 
     def strategy_name_to_object

spec/factory_girl/evaluator_class_definer_spec.rb

@@ -4,10 +4,9 @@
   let(:simple_attribute)                    { stub("simple attribute",   name: :simple, to_proc: lambda { 1 }) }
   let(:relative_attribute)                  { stub("relative attribute", name: :relative, to_proc: lambda { simple + 1 }) }
   let(:attribute_that_raises_a_second_time) { stub("attribute that would raise without a cache", name: :raises_without_proper_cache, to_proc: lambda { raise "failed" if @run; @run = true; nil }) }
-  let(:callbacks) { [stub("callback 1"), stub("callback 2")] }
 
   let(:attributes)    { [simple_attribute, relative_attribute, attribute_that_raises_a_second_time] }
-  let(:class_definer) { FactoryGirl::EvaluatorClassDefiner.new(attributes, callbacks, FactoryGirl::Evaluator) }
+  let(:class_definer) { FactoryGirl::EvaluatorClassDefiner.new(attributes, FactoryGirl::Evaluator) }
   let(:evaluator)     { class_definer.evaluator_class.new(stub("build strategy", add_observer: true)) }
 
   it "returns an evaluator when accessing the evaluator class" do
@@ -32,23 +31,14 @@
     class_definer.evaluator_class.attribute_lists.should == [attributes]
   end
 
-  it "sets callbacks on the evaluator class" do
-    class_definer.evaluator_class.callbacks.should == callbacks
-  end
-
   context "with a custom evaluator as a parent class" do
-    let(:child_callbacks)  { [stub("child callback 1"), stub("child callback 2")] }
     let(:child_attributes) { [stub("child attribute", name: :simple, to_proc: lambda { 1 })] }
-    let(:child_definer)    { FactoryGirl::EvaluatorClassDefiner.new(child_attributes, child_callbacks, class_definer.evaluator_class) }
+    let(:child_definer)    { FactoryGirl::EvaluatorClassDefiner.new(child_attributes, class_definer.evaluator_class) }
 
     subject { child_definer.evaluator_class }
 
     it "bases its attribute lists on itself and its parent evaluator" do
       subject.attribute_lists.should == [attributes, child_attributes]
     end
-
-    it "bases its callbacks on itself and its parent evaluator" do
-      subject.callbacks.should == callbacks + child_callbacks
-    end
   end
 end

spec/factory_girl/factory_spec.rb

@@ -25,7 +25,7 @@
 
     factory.run(FactoryGirl::Strategy::Build, {})
 
-    strategy.should have_received(:result).with(instance_of(FactoryGirl::AttributeAssigner), block)
+    strategy.should have_received(:result).with(instance_of(FactoryGirl::Evaluation))
   end
 
   it "returns associations" do

spec/factory_girl/strategy/attributes_for_spec.rb

@@ -1,18 +1,18 @@
 require 'spec_helper'
 
 describe FactoryGirl::Strategy::AttributesFor do
-  let(:result)             { { name: "John Doe", gender: "Male", admin: false } }
-  let(:attribute_assigner) { stub("attribute assigner", hash: result) }
+  let(:result)     { { name: "John Doe", gender: "Male", admin: false } }
+  let(:evaluation) { stub("evaluation", hash: result) }
 
   it_should_behave_like "strategy without association support"
 
-  it "returns the hash from the attribute assigner" do
-    subject.result(attribute_assigner, lambda {|item| item }).should == result
+  it "returns the hash from the evaluation" do
+    subject.result(evaluation).should == result
   end
 
   it "does not run the to_create block" do
     expect do
-      subject.result(attribute_assigner, lambda {|item| raise "failed" })
+      subject.result(evaluation)
     end.to_not raise_error
   end
 end

spec/factory_girl/strategy/create_spec.rb

@@ -5,9 +5,21 @@
   it_should_behave_like "strategy with callbacks", :after_build, :before_create, :after_create
 
   it "runs a custom create block" do
-    block_run = false
-    block = lambda {|instance| block_run = true }
-    subject.result(stub("assigner", object: stub("result instance")), block)
-    block_run.should be_true
+    evaluation_class = Class.new do
+      def initialize
+        @block_run = false
+      end
+
+      attr_reader :block_run
+
+      def create(*instance)
+        @block_run = true
+      end
+    end
+
+    evaluation = evaluation_class.new
+    evaluation.stubs(object: nil, notify: nil)
+    subject.result(evaluation)
+    evaluation.block_run.should be_true
   end
 end