Ruby Koans Proxy Project

Question

I've just finished the Ruby Koans and one of the last projects was creating a Proxy class that sends methods to another class. I was wondering whether you would change anything about my solution (if there's something wrong or a better way to do it). I'm still a beginner and I want to get better.

require File.expand_path(File.dirname(__FILE__) + '/edgecase')

# Project: Create a Proxy Class
#
# In this assignment, create a proxy class (one is started for you
# below).  You should be able to initialize the proxy object with any
# object.  Any messages sent to the proxy object should be forwarded
# to the target object.  As each message is sent, the proxy should
# record the name of the method send.
#
# The proxy class is started for you.  You will need to add a method
# missing handler and any other supporting methods.  The specification
# of the Proxy class is given in the AboutProxyObjectProject koan.

class Proxy
  attr_reader(:messages)

  def method_missing(method_name, *args, &block)
    if @methods_to_pass.include?(method_name.to_s)
      @messages << method_name
      @object.__send__(method_name, *args, &block)
    else
      super(method_name, *args, &block)
    end
  end

  def called?(method_name)
    @messages.include?(method_name)
  end

  def number_of_times_called(method_name)
    @messages.count(method_name)
  end

  def initialize(target_object)
    @methods_to_pass, @messages = [], []
    @object = target_object
    @object.class.instance_methods(true).each { |method_name| @methods_to_pass << method_name; method_name }
  end

end

# The proxy object should pass the following Koan:
#
class AboutProxyObjectProject < EdgeCase::Koan
  def test_proxy_method_returns_wrapped_object
    # NOTE: The Television class is defined below
    tv = Proxy.new(Television.new)

    assert tv.instance_of?(Proxy)
  end

  def test_tv_methods_still_perform_their_function
    tv = Proxy.new(Television.new)

    tv.channel = 10
    tv.power

    assert_equal 10, tv.channel
    assert tv.on?
  end

  def test_proxy_records_messages_sent_to_tv
    tv = Proxy.new(Television.new)

    tv.power
    tv.channel = 10

    assert_equal [:power, :channel=], tv.messages
  end

  def test_proxy_handles_invalid_messages
    tv = Proxy.new(Television.new)

    assert_raise(NoMethodError) do
      tv.no_such_method
    end
  end

  def test_proxy_reports_methods_have_been_called
    tv = Proxy.new(Television.new)

    tv.power
    tv.power

    assert tv.called?(:power)
    assert ! tv.called?(:channel)
  end

  def test_proxy_counts_method_calls
    tv = Proxy.new(Television.new)

    tv.power
    tv.channel = 48
    tv.power

    assert_equal 2, tv.number_of_times_called(:power)
    assert_equal 1, tv.number_of_times_called(:channel=)
    assert_equal 0, tv.number_of_times_called(:on?)
  end

  def test_proxy_can_record_more_than_just_tv_objects
    proxy = Proxy.new("Code Mash 2009")

    proxy.upcase!
    result = proxy.split

    assert_equal ["CODE", "MASH", "2009"], result
    assert_equal [:upcase!, :split], proxy.messages
  end
end


# ====================================================================
# The following code is to support the testing of the Proxy class.  No
# changes should be necessary to anything below this comment.

# Example class using in the proxy testing above.
class Television
  attr_accessor :channel

  def power
    if @power == :on
      @power = :off
    else
      @power = :on
    end
  end

  def on?
    @power == :on
  end
end

# Tests for the Television class.  All of theses tests should pass.
class TelevisionTest < EdgeCase::Koan
  def test_it_turns_on
    tv = Television.new

    tv.power
    assert tv.on?
  end

  def test_it_also_turns_off
    tv = Television.new

    tv.power
    tv.power

    assert ! tv.on?
  end

  def test_edge_case_on_off
    tv = Television.new

    tv.power
    tv.power
    tv.power

    assert tv.on?

    tv.power

    assert ! tv.on?
  end

  def test_can_set_the_channel
    tv = Television.new

    tv.channel = 11
    assert_equal 11, tv.channel
  end
end

Answer 1

@methods_to_pass, @messages = [], []
@object = target_object
@object.class.instance_methods(true).each { |method_name| @methods_to_pass << method_name; method_name }

Couple of things to note here:

First of all the ; method_name bit is completely unnecessary. I don't know why you put that there, but it serves no purpose and just confuses the reader.

Secondly foo = []; bar.each {|x| foo << x} is an overly verbose way to copy an array. You can just say foo = bar.dup. However in this case there is no reason to perform a copy, so you can just say foo = bar, i.e. @methods_to_pass = @object.class.instance_methods(true) (but don't - see below).

That being said storing the methods in an array like that is a bad idea for two reasons:

1. Since checking whether an item is in an array is O(n), your method_missing method and thus every proxied call becomes O(n). This is needlessly inefficient.
2. If @object has singleton methods or dynamically defined methods, those won't be forwarded as they won't show up in @object.class.instance_methods.

To fix these issues you should get rid of @methods_to_pass and instead use one of the following approaches:

Option a:

In method_missing replace if @methods_to_pass.include?(method_name.to_s) with if @object.respond_to?(method_name). This will be faster than searching through an array and it will also work for dynamically defined or singleton methods. However it will not work if @object has a custom method_missing method and does not override respond_to? to be in sync with method_missing. If you want to handle that case as well, you'll need to use one of the other two options.

Option b:

Just remove the whole if and always forward (and record) the method call. This way the behavior will be different from your code as invalid calls will be recorded in @messages. However since they will still cause a NoMethodError and the assignment doesn't actually state whether invalid calls should be recorded or not, it still meets the requirements.

---

Now let's talk about the @messages array:

Using an array to store the messages and then using count to find out how often a given message appears is again needless inefficient. What I'd do instead is use a Hash which maps each message to the number of times it's been called. This way you'd write @message = Hash.new(0) instead of @messages = [], @messages[method_name] += 1 instead of @messages << method_name and @messages[method_name] instead of @messages.count(method_name).

Answer 2

sepp2k's responses are all good, I would add one more suggestion. Your Proxy class should derive from BasicObject (if you use ruby 1.9.x). At the moment any method defined on Proxy is by definition not missing. For example given:

foo = Foo.new
pfoo = Proxy.new(foo)

if I asked for pfoo.class I would get Proxy because class is a method in the inheritance of Proxy. With BasicObject this would instead return Foo and add an entry for "class" in the proxy object.

One other nit to pick, I would add an underscore in front of called and number_of_times_called. Putting an underscore in a method is a clue to other programmers that the method is outside the normal operation of a class. In this case it will help prevent name collision with the object you are going to proxy to. The truly paranoid would actually call a method on a separate object to count calls leaving the entire namespace unused. Think what would happen if you tried to proxy a Proxy instance. :)

Answer 3

In my view, there were problems with every one of the suggested solutions, which is why I took a different approach: the wrapper class dynamically programs itself during object initialization: it just "implements" every single public method (except for a few system-specific ones) of the wrapped class by recording its name and forwarding the call to the wrapped object.

In this way, not only methods that are not defined in the wrapper class are forwarded, but also (most of) those that are defined. This is crucial if the wrapped class (Television in this example) overwrites some standard method such as to_s. In all the other solutions above, sending the to_s message to the wrapper object will just call the wrapper's to_s (inherited from Object). In my solution, the method being called really is the wrapped object's own to_s method.

Moreover, I think my approach is the only one here that makes the wrapper object reply to respond_to? correctly, i.e. returning true for all the methods defined in the wrapped class.

class Proxy
  attr_reader :messages   # return an array of messages called

  def initialize(target_object)
    @object = target_object
    @methods_called = Hash.new(0);
    @messages = [] # Array for storing the message names

    # For each of the object's methods
    methods_to_forward = @object.methods - [:__send__, :object_id, :instance_of?, :method_missing]
    methods_to_forward.each do |method|
        eval """ def #{method}(*args, &block)
                    @methods_called[:#{method}] += 1
                    @messages.push :#{method}
                    @object.send(:#{method}, *args, &block)
                 end            
             """
    end
  end

  def called? method
    number_of_times_called(method) > 0 
  end

  def number_of_times_called(method)
    @methods_called[method]
  end
end