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.

• Fivell: Thanks for you comment!!! I made debug and connection returned the same object_id so I think yes this object is shared. "In add_response_method, it is unnecessary to pass 'http_response' as an argument" => user can define method name if he want to use different one. Also I can't defind http_response instance method on ActiveResource::Base because result of find and get methods can be array or even hash.
• Alex D: @Fivell, what difference does it make if find and get return arrays, hashes, or some other type of object? Whatever the case, each ActiveRecord model needs a http_response method, and the body is the same in each case: def http_response; @http_response; end. (That's what attr_reader generates for you.) Why do you want to generate this method dynamically every time find is executed?
• Alex D: Also, even if you want to make the name of the http_response method configurable, you don't need to create a symbol dynamically (using :"#{}") on each execution. You can just do something like this: define_method(method_name) do @__http_response__ end. Then assign the value with instance_variable_set(@__http_response__, connection.http_response).
• Alex D: OK, I see. I am running Ruby 1.9.2, so that's why it works for you but not for me. If you want to make this code compatible with earlier versions of Ruby, why don't you do something like: def result.http_response; @__http_response__; end
• Alex D: >> [].class_eval "attr_reader :http_response" NoMethodError: undefined method class_eval' for []:Array from (irb):1 from C:/Ruby192/bin/irb:12:in <main>'