3
\$\begingroup\$

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.

\$\endgroup\$
1
+50
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Fivell Feb 22 '12 at 15:39
  • \$\begingroup\$ @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? \$\endgroup\$ – Alex D Feb 22 '12 at 17:13
  • 1
    \$\begingroup\$ 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). \$\endgroup\$ – Alex D Feb 22 '12 at 17:18
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – Alex D Feb 23 '12 at 7:54
  • 1
    \$\begingroup\$ >> [].class_eval "attr_reader :http_response" NoMethodError: undefined method class_eval' for []:Array from (irb):1 from C:/Ruby192/bin/irb:12:in <main>' \$\endgroup\$ – Alex D Feb 23 '12 at 9:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.