[rspec-users] Rate my code: refactoring from spec

Pat Maddox pat.maddox at gmail.com
Wed Apr 15 13:25:52 EDT 2009


On Wed, Apr 15, 2009 at 9:36 AM, Fernando Perez <lists at ruby-forum.com> wrote:
> Hi,
>
> I used to have the following method:
>
> def Paypal.process_ipn(params)
>  ...
>  paypal = create!(params)
>  paypal.order.update_from_ipn(completed?)
> end
>
> That method obviously is not easily specable because of the double dot
> method call, and when specing it, it would hit the DB for nothing. I
> used to actually spec the associated order status to know if everything
> went on well which breaks the isolation of unit tests. Bad bad bad.
>
> -----------
>
> So I refactored it as following:
>
> def Paypal.new_from_ipn(params)
>  ...
>  new(params)
> end
>
> So that it doesn't hit the DB when I spec the method, and specing the
> method can now be done in isolation, as I only set some values returned
> by IPN to something more compliant with my DB storage and then in the
> controller that calls this method:
>
> paypal = Paypal.new_from_ipn(params)
> paypal.update_order if paypal.save!
>
>
> So the paypal instance still gets saved, but instead of getting saved in
> the model it occurs in the controller, no big deal. Also I added a new
> instance method to Paypal class to abid to Demeter's law:
>
> def update_order
>  order.update_from_paypal_ipn!(completed?)
> end
>
> But now I am wondering how to spec this instance method, I thought of
> the following:
>
> it "should update the associated order" do
>  @paypal = Paypal.new(:payment_status => 'Completed')
>  @order = @paypal.order
>  @order.expects(:update_from_paypal_ipn!).with(true)
>
>  @paypal.update_order
> end
>
>
> Is that spec acceptable? I mean I am not sure about the following two
> lines:
>
> @order = @paypal.order
> @order.expects(:update_from_paypal_ipn!).with(true)
>
> I thought about this other solution which is slightly more complicated:
>  @order = Order.new
>  @paypal = Paypal.new(:payment_status => 'Completed')
>  @paypal.stubs(:order).returns(@order)
>  @order.expects(:update_from_paypal_ipn!).with(true)
>  @paypal.update_order
>
> So which is best?
>
> And what do you think of my refactoring and my new specs? Did I improve
> the code or is it just crap?
> --
> Posted via http://www.ruby-forum.com/.
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
>

I would probably move the call to update_order into a
before/after_create on Paypal.  Then I would write a spec on Paypal
that checks that the right thing happened.  I would not likely use a
mock unless Order#update_from_paypal_ipn! is very complex.

describe Paypal, "from IPN" do
  it "should update its order when created" do
    paypal = Paypal.new_from_ipn :ipn => {:completed => true}
    paypal.save!
    paypal.order.should be_completed
  end
end

class Paypal
  after_create :update_order

  def update_order
    order.update_from_paypal_ipn! completed?
  end
end

By doing it in a callback you get
* transaction semantics for free
* ability to use resource_controller
* simpler model API

If you only want to update the order when Paypal.new_from_ipn is
called (and not Paypal.new, for example) then just put an ivar in
Paypal and check for it in the callback.

So to recap, I would test this behavior via the Paypal examples,
because that's where the behavior originates.  I may or may not mock
the call to order.update_from_paypal depending on how complex it is.

Does that make sense?

Pat


More information about the rspec-users mailing list