Activeresource-response gem

Question

I created my first gem for Rails today. https://github.com/Fivell/activeresource-response. This gem adds possibility to access http response object from result of activeresource call.

• I don't know how to create test for this gem.
• Also I want to ask if there is more simply and elegant way to do this?
• Is it really thread-safe?
• Other ways to write this functionllity except alias_method?

Here is source of library.

require "activeresource-response/version"
module ActiveresourceResponse
   module Connection
     def self.included(base)
       base.class_eval  <<-EOS
           alias_method :origin_handle_response, :handle_response 
           attr_reader :http_response
           def handle_response(response)
             @http_response= response
             origin_handle_response(response)
           end
        EOS
     end 
   end   
   module AddResponseMethod
     def self.included(base)
        base.extend ClassMethods
      end
      module ClassMethods
         def add_response_method(method_name = 'http_response')
             class_eval  <<-EOS
               class << self
                 alias_method :origin_find, :find
                 def find(*arguments)
                     result = origin_find(*arguments)
                     result.class_eval("attr_reader :#{method_name}")
                     result.instance_variable_set(:"@#{method_name}", connection.http_response)
                     result
                 end
                end 
             EOS
         end   
      end
   end   
end
ActiveResource::Connection.send :include, ActiveresourceResponse::Connection
ActiveResource::Base.send :include, ActiveresourceResponse::AddResponseMethod

UPDATE

I made some refactoring, please look at github sources.

Answer 1

First thing, it unnecessary to use the included hook methods. Since your purpose is to redefine some methods in ActiveresourceResponse::Connection and Activeresource::Base, why not just do so directly?

# if ActiveResource has not been loaded yet, Rails' const_missing hook
#   will load it automatically
ActiveResource::Connection.class_eval do
  alias_method :__handle_response__ :handle_response
  # ... and so on
end
ActiveResource::Base.instance_eval do
  alias_method :__find__ :find
  # ... and so on
end

By using instance_eval in the second case, we can redefine class methods with 1 less line of code, because class << self is unnecessary.

In add_response_method, it is unnecessary to pass 'http_response' as an argument. That code will never be run with any other value, so why use an argument? Just use a literal value, it's clearer and easier to read.

Every time a model object is returned from ActiveResource::Base.find, you define the http_response method on it with class_eval. This is very inefficient -- I think you should just define a http_response instance method on ActiveResource::Base, and all model classes derived from ActiveResource::Base will inherit it. (Think of the work which is done to define a method -- the Ruby parser has to run on the code, it has to be compiled to bytecode, then an entry has to be made in a method table...)

I just benchmarked Object#instance_variable_set, and it's very fast, faster than using instance_eval or instance_exec. So I think that should stay, but don't generate a new symbol using :"#{}" each time. Just use something like :@__http_response__. (Why the "special" name? Because when you are adding instance variables to a user-defined class, you want to reduce the chances of a collision with a user-defined variable.) Of course, this will mean you can't use attr_reader, but that's no problem. You can just do something like: def http_response; @__http_response__; end

For tests, why don't you look at how ActiveResource itself is tested? I'm sure all that code is open-source and should be available from a public repo.

EDIT: I just looked at the GitHub repo and noticed that you changed to a thread-local variable to try to make the code thread-safe. My question is: is a single ActiveResource::Connection object shared by the whole application? Or each time you make a request, does it create a new Connection object? If it creates a new Connection object for each request, then using thread-local variables is unnecessary, and an instance variable would be better. If the whole application is sharing only one Connection object, then using the thread-local variable is a good idea.

EDIT 2: I just found a bug. If a user tries to use add_response_method to add an alias for http_response, subsequent calls to find will go into an infinite loop. Try it and you will see.