views:

1313

answers:

3

I'm having trouble removing some duplication I've introduced in a rails plugin.

The code below modifies the find and calculate methods of ActiveRecord in the same way, but I've been unable to remove the duplication.

The find and calculate methods below make use of the super keyword which is one hurdle as the super keyword can only be used to call a method sharing the same name as the calling method, so I can't move the super keyword to a method shared by find and calculate.

So next I tried aliasing the find and calculate class methods from the superclass ActiveRecord, however, I've not been able to get the syntax right for the aliasing. If someone could show me that, it would be a great help.

If you've got a better way entirely of doing this I'd love for you to post that too.

Below I've trimmed the code down a little to highlight the problem:

module Geocodable #:nodoc:

  def self.included(mod)
    mod.extend(ClassMethods)
  end

  module ClassMethods
    def acts_as_geocodable(options = {})
      extend Geocodable::SingletonMethods
    end
  end

  module SingletonMethods

    def find(*args)
      some_method_1
      super *args.push(options)
      some_method_2
    end

    # TODO: Remove duplication of find above and calculate below.

    def calculate(*args)
      some_method_1
      super *args.push(options)
      some_method_2
    end
  end
end
+1  A: 

Your best way to refactor this code is to leave find and calculate unchanged, and add apply the wrapping using a class-level function.

Here's rough sketch, without your module and mixin logic:

class A
  def find x
    puts 'finding'
  end

  def calculate x
    puts 'calculating'
  end
end

class B < A
  def self.make_wrapper_method name
    define_method name do |*args|
      puts "entering"
      result = super *args
      puts "exiting"
      result
    end
  end

  make_wrapper_method :find
  make_wrapper_method :calculate
end

Note that this will need to be modified if B has already overridden find or calculate.

To use this code, first make your version work correctly, then modify it to use define_method. (And if you need extremely high performance, you may need to use one of the *_eval functions to create the wrappers instead of define_method.)

emk
Thank you emk, your solution took me all the way to my final solution which I've presented below
Eliot Sykes
A: 

To just alias the find method:

module SingletonMethods
  def find(*args)
    some_method_1
    super *args.push(options)
    some_method_2
  end
  alias :calculate :find
end
marktucks
Thanks for your advice
Eliot Sykes
+1  A: 

This is the option I went for in the end, thanks to emk for guidance to get to this point!

module Geocodable

  def self.included(mod)
    mod.extend(ClassMethods)
  end

  module ClassMethods
    def acts_as_geocodable(options = {})
      geoify_query_methods
    end

    private
      # This is where the duplication has been removed
      def geoify_query_methods
        class << self
          [:calculate, :find].each do |method_name|
            define_method method_name do |*args|
              some_method_1
              super *args.push(options)
              some_method_2
            end
          end
        end
      end

  end
end
Eliot Sykes