Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Version 1

I need a way to store and retrieve callbacks in Ruby. So I made a Callbacks class. Any suggestions would be greatly appreciated, thank you!

class Callbacks
  def initialize
    # name: name of callback
    # event: event of which it belongs
    # callback: block to call
    @callbacks = []
  end

  def add name, event, &block
    if block_given?
      @callbacks << {
        name: name,
        event: event,
        callback: block
      }
    end
  end

  def get options={}
    name = options[:name]
    event = options[:event]

    @callbacks.each do |h|
      if name and event
        return h if h[:name] == name and h[:event] == event
      else
        return h if h[:name] == name or h[:event] == event
      end
    end
    nil
  end
end

UPDATE: Both the name and event can be the same. So there could be two callbacks with a name of :all and with an event of :join.

Version 2

My second attempt at this isn't that much different that version one. The main difference lies within the get function. For the updated version of the add function see blufox's answer.

def get options={}
  name, event = options.values_at(:name, :event)
  return @callbacks[options] if name and event

  @callbacks.inject [] do |a, (k, _)|
    a << k if k[:name] == name or k[:event] == event
    a
  end
end

The requirements for this function are.

  1. One match is returned if both :name and :event are supplied.
  2. An array of matches are returned if either :name or :event are supplied.

Those are the only requirements. Any suggestions would be wonderful. Thank you.

share|improve this question
Just to be sure, you are returning the callback even if one of the properties does not match. Is this intensional? – rahul May 31 '12 at 18:16
Yes, it is returning a callback even if one of the options is not defined. – Bryan Dunsmore May 31 '12 at 18:24
In your code, it would match even if both the options are defined but say the name does not match. Surely this is not the correct behavior then? – rahul May 31 '12 at 18:26
That doesn't sound right. If :name and :event are defined, it searches for a callback that matches both :name and :event. Otherwise, it matches one or the other. – Bryan Dunsmore May 31 '12 at 18:34
Oh ok, my mistake. – rahul May 31 '12 at 18:43

2 Answers

up vote 1 down vote accepted

Why not use a hashmap as the main DS? That way, you optimize for when name and event are defined, and suffer only a constant penalty and degrades to linear for other cases. (In your solution the cost is always linear). (As below)

class Callbacks
  def initialize
    @callbacks = {}
  end

  def add(name, event, &block)
    key = {:name => name, :event => event}
    @callbacks[key] = block if block_given?

shouldn't we throw an exception if block is not given?

  end

  def get(o={})

We can use values_at to extract multiple values from a hash.

    name,event = o.values_at(:name,:event)

We don't need to check if name and event separately because equality checking does it for us.

    return @callbacks[o] if name and event
    @callbacks.each do |k,v|
      return v if k[:name] == name or k[:event] == event
    end
    nil
  end
end

And shouldn't you provide a way to unregister from callback?

share|improve this answer
up vote 0 down vote

#get could be implemented like this:

def get(options)
  @callbacks.find do |callback|
    options.all? do |key, value|
      callback[key] == value
    end
  end
end
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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