Is aliasing methods frowned upon?

Question

I'm developing an online store, and the customer needs the ability to delete an order and have its products automatically restocked (e.g., for test orders). Here's my first try at implementing this:

class Order < ActiveRecord::Base
  def destroy_and_restock
    restock_products
    destroy
  end

  protected

  def restock_products
    line_items.each do |li|    
      li.product.quantity_on_hand += li.quantity
      li.product.save
    end
  end
end

But what if I need to create another destroy_and_x method later? Why not allow that X to be passed as a parameter to the destroy() method? So now I'm thinking of going with this:

alias :old_destroy :destroy
def destroy(options = {})
  if options['restock'] == true
    restock_products
  end
  old_destroy
end

protected

def restock_products
  line_items.each do |li|    
    li.product.quantity_on_hand += li.quantity
    li.product.save
  end

This is more extensible, but makes me feel somewhat dirty. Am I wrong to feel dirty? Is there a better way of doing this?

Answer 1

I'd say "yes, this is dirty." Your intention isn't to modify the behavior of the 'destroy' method, but rather to do some domain-specific work, then run destroy. Your first approach is great -- define a method that does what you want, and invoke destroy as needed. I think that 'wrapping' or 'monkey-patching' a method, as you're considering, is a technique that's best applied when standard OO approaches can't be used -- eg, when you need to modify/augment behavior in a class that is defined and used outside of your realm of control.

Even if you are considering modifying the behavior of the destroy method itself, I'd suggest overriding the method here, rather than wrapping it:

def destroy(options = {})
  restock_products if options['restock']
  super() # I think parens are necessary here, to avoid passing options up the chain
end

Answer 2

How about just use a block? Then you dont have to pull your hair apart while designing this in the class and you can do more as and when you need to:

def destroy_after &block
  yield if block
  destroy
end

Then call it like this:

order.destroy_after { order.restock_products }

I can not think of a good name for this function... but I hope you get the idea.

Answer 3

Horace, I misunderstood your question. I think you are looking for this:

http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html

Now you can keep your method protected and add as many before_destroy things as you like. Hope this works for you without overriding destroy.

Best of luck.

Answer 4

If monkey patching doesn't let you sleep at night, you can achieve the same thing by subclassing. When I'm in the need of a quick hack, or a quick debug hack, I monkey patch as well.