Ruby Koans Proxy Project

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.ch

@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



• 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)`.

StackExchange Code Review Q#2015, answer score: 11