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 "active_support/core_ext/module/delegation"
 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 @@ require "active_support/core_ext/class/attribute"
 
 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 @@ module FactoryGirl
       @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 @@ module FactoryGirl
     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 @@ module FactoryGirl
 
     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 @@ module FactoryGirl
       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 @@ module FactoryGirl
     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 @@ module FactoryGirl
     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.rb

@@ -1,33 +0,0 @@
-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 "observer"
-
-module FactoryGirl
-  class Strategy #:nodoc:
-    include Observable
-
-    def association(runner)
-      raise NotImplementedError, "Strategies must return an association"
-    end
-
-    def result(attribute_assigner, to_create)
-      raise NotImplementedError, "Strategies must return a result"
-    end
-
-    def self.ensure_strategy_exists!(strategy)
-      unless Strategy.const_defined? strategy.to_s.camelize
-        raise ArgumentError, "Unknown strategy: #{strategy}"
-      end
-    end
-
-    private
-
-    def run_callbacks(name, result_instance)
-      changed
-      notify_observers(name, result_instance)
-    end
-  end
-end

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 @@ module FactoryGirl
     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 @@ describe FactoryGirl::EvaluatorClassDefiner do
   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 @@ describe FactoryGirl::EvaluatorClassDefiner do
     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 @@ describe FactoryGirl::Factory do
 
     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 @@ describe FactoryGirl::Strategy::Create do
   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

spec/factory_girl/strategy/stub_spec.rb

@@ -14,25 +14,24 @@ describe FactoryGirl::Strategy::Stub do
       end.new
     end
 
-    let(:assigner)  { stub("attribute assigner", object: result_instance) }
-    let(:to_create) { lambda {|instance| instance } }
+    let(:evaluation)  { stub("evaluation", object: result_instance, notify: true) }
 
-    it { subject.result(assigner, to_create).should_not be_new_record }
-    it { subject.result(assigner, to_create).should be_persisted }
+    it { subject.result(evaluation).should_not be_new_record }
+    it { subject.result(evaluation).should be_persisted }
 
     it "assigns created_at" do
-      created_at = subject.result(assigner, to_create).created_at
+      created_at = subject.result(evaluation).created_at
       created_at.should == Time.now
 
       Timecop.travel(150000)
 
-      subject.result(assigner, to_create).created_at.should == created_at
+      subject.result(evaluation).created_at.should == created_at
     end
 
     [:save, :destroy, :connection, :reload, :update_attribute].each do |database_method|
       it "raises when attempting to connect to the database by calling #{database_method}" do
         expect do
-          subject.result(assigner, to_create).send(database_method)
+          subject.result(evaluation).send(database_method)
         end.to raise_error(RuntimeError, "stubbed models are not allowed to access the database")
       end
     end

spec/factory_girl/strategy_calculator_spec.rb

@@ -8,16 +8,6 @@ describe FactoryGirl::StrategyCalculator, "with a FactoryGirl::Strategy object"
   end
 end
 
-describe FactoryGirl::StrategyCalculator, "with a non-FactoryGirl::Strategy object" do
-  before { define_class "MyAwesomeStrategy" }
-
-  let(:strategy) { MyAwesomeStrategy }
-
-  it "returns the strategy object" do
-    expect { FactoryGirl::StrategyCalculator.new(strategy).strategy }.to raise_error "unrecognized method MyAwesomeStrategy"
-  end
-end
-
 describe FactoryGirl::StrategyCalculator do
   it "returns the correct strategy object for :build" do
     FactoryGirl::StrategyCalculator.new(:build).strategy.should == FactoryGirl::Strategy::Build

spec/factory_girl/strategy_spec.rb

@@ -1,21 +0,0 @@
-require 'spec_helper'
-
-describe FactoryGirl::Strategy do
-  it "raises an error when asking for the result" do
-    expect { subject.result(stub("assigner"), lambda {|instance| instance }) }.to raise_error(NotImplementedError, "Strategies must return a result")
-  end
-
-  it "raises an error when asking for the association" do
-    expect { subject.association(stub("runner")) }.to raise_error(NotImplementedError, "Strategies must return an association")
-  end
-end
-
-describe FactoryGirl::Strategy, ".ensure_strategy_exists!" do
-  it "raises when passed a nonexistent strategy" do
-    expect { FactoryGirl::Strategy.ensure_strategy_exists!(:nonexistent) }.to raise_error(ArgumentError, "Unknown strategy: nonexistent")
-  end
-
-  it "doesn't raise when passed a valid strategy" do
-    expect { FactoryGirl::Strategy.ensure_strategy_exists!(:create) }.to_not raise_error
-  end
-end

spec/support/shared_examples/strategy.rb

@@ -70,36 +70,22 @@ shared_examples_for "strategy with strategy: :build" do |factory_girl_strategy_c
 end
 
 shared_examples_for "strategy with callbacks" do |*callback_names|
-  let(:callback_observer) do
-    define_class("CallbackObserver") do
-      attr_reader :callbacks_called
-
-      def initialize
-        @callbacks_called = []
-      end
-
-      def update(callback_name, assigner)
-        @callbacks_called << [callback_name, assigner]
-      end
-    end.new
-  end
-
   let(:result_instance) do
     define_class("ResultInstance") do
       attr_accessor :id
     end.new
   end
 
-  let(:assigner) { stub("attribute assigner", object: result_instance) }
+  let(:evaluation) { stub("evaluation", object: result_instance, notify: true, create: nil) }
 
-  before { subject.add_observer(callback_observer) }
-
-  it "runs the callbacks #{callback_names} with the assigner's object" do
-    subject.result(assigner, lambda {|instance| instance })
-    callback_observer.callbacks_called.should == callback_names.map {|name| [name, assigner.object] }
+  it "runs the callbacks #{callback_names} with the evaluation's object" do
+    subject.result(evaluation)
+    callback_names.each do |name|
+      evaluation.should have_received(:notify).with(name, evaluation.object)
+    end
   end
 
-  it "returns the object from the assigner" do
-    subject.result(assigner, lambda {|instance| instance }).should == assigner.object
+  it "returns the object from the evaluation" do
+    subject.result(evaluation).should == evaluation.object
   end
 end