5
\$\begingroup\$

For something I'm doing, it would just make things so much easier, quicker, and cleaner to have C# style events. I've written a class I believe should replicate a C# style event using reflection. Since I've never actually used reflection before in any meaningful sense, I'm very much not happy using it without, at the very least, a second opinion.

I want to declare an event with:

public Event SomeEvent = new Event();

Raise the event (presumably within the declaring class) with (where SomeArguments is whatever class that may be specified in the Javadocs boxed in an Object):

SomeEvent.Raise(this, SomeArguments);

Add a method to be called when the event is raised with:

SomeEvent.AddListener(this, "onSomeEvent");

Stop a method being called when an event is raised with:

SomeEvent.RemoveListener(this, "onSomeEvent");

And cancel all listeners with:

SomeEvent.ClearListeners();

Methods to listen to the event should be formatted like:

public void onSomeEvent(object Sender, object SomeArguments)
{ }

And this is my class:

package puppy.hanii.library;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.ArrayList;

public class Event
{
    public Event()
    {
        Methods = new ArrayList<Method>();
        DeclaringClasses = new ArrayList<Object>();
    }

    ArrayList<Method> Methods;
    ArrayList<Object> DeclaringClasses;

    public boolean AddListener(Object Class, String MethodName)
    {
        try
        {
            Method MTR = Class.getClass().getMethod(MethodName, Object.class, Object.class); // MTR = Method To Register.

            if(Methods.contains(MTR))
                return false;
            else
            {
                Methods.add(MTR);
                DeclaringClasses.add(Class);
                return true;
            }
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overloads of method, exists."); }
    }

    public boolean RemoveListener(Object Class, String MethodName)
    {
        try
        { 
            DeclaringClasses.remove(Class);
            return Methods.remove(Class.getClass().getMethod(MethodName, Object.class, Object.class)); 
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overload of method, exists."); }
    }

    public void ClearListeners()
    { 
        Methods.clear();
        DeclaringClasses.clear();
    }

    public void Raise(Object Sender, Object Arguments)
    {
        for(int i = 0; i < Methods.size(); i++)
        {
            try
            { Methods.get(i).invoke(DeclaringClasses.get(i), Sender, Arguments); }
            catch(IllegalAccessException error)
            { }
            catch(InvocationTargetException error)
            { throw new RuntimeException("Error in event handler"); }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Have you considered creating an anonymous class that call the event handler instead of string method names? \$\endgroup\$ – svick Sep 12 '12 at 20:05
  • \$\begingroup\$ That seems like it would make it somewhat more fiddly when registering listeners than it needs to be :X Plus I like keeping the listener method separate from its assignment to an event. What if I wanted to register, deregister, and re-register the same method to an event multiple times? I'd have to either type the same method multiple times or create a method in the listener's class for the sole purpose of registering the event. Although I'm not too experienced with anonymous classes either ^^;; so maybe I've picked something up wrong. \$\endgroup\$ – Hanii Puppy Sep 13 '12 at 14:00
3
\$\begingroup\$
    ArrayList<Method> Methods;
    ArrayList<Object> DeclaringClasses;

Two things: firstly, you're using C# naming conventions rather than Java. Secondly, code to the interface: the fields should be List<XYZ> rather than ArrayList<XYZ>.

    public boolean AddListener(Object Class, String MethodName)

Class isn't an obvious name for something which isn't a class. And given that you're using java.lang.Class inside the method without fully qualifying the name, it has even more potential to confuse.

            Method MTR = Class.getClass().getMethod(MethodName, Object.class, Object.class); // MTR = Method To Register.

If you have to add a comment explaining the name, perhaps you chose the wrong name? What's wrong with Method method = ...?

            if(Methods.contains(MTR))
                return false;

So I can't have the event notify two different objects of the same class?

    public boolean RemoveListener(Object Class, String MethodName)
    {
        try
        { 
            DeclaringClasses.remove(Class);
            return Methods.remove(Class.getClass().getMethod(MethodName, Object.class, Object.class)); 
        }

Buggy. Collection.remove removes one instance of the object, but you're making no attempt to ensure it's the right one. If I register two methods on the same object, this could remove the wrong instance of the object, which would then make objects and methods not line up correctly.

In fact, it's even worse than that: if I register a method on one object and then remove it on another, or register one method on an object and then remove a different one, I'll end up with Methods and DeclaringClasses not even being the same size.

Also, why would you want to call getMethod again unless the method hasn't been registered? If the method has been registered, you've already resolved it; if it hasn't, you don't need to resolve it except to check for typos.

The best way to store the resolved methods is probably Map<Tuple<Object, String>, Method>: that allows you to iterate through the entry set in the Raise method and to efficiently implement RemoveListener.

            catch(IllegalAccessException error)
            { }

No. Swallowing exceptions like that is bad, especially when you've done nothing to prevent them occurring. You should check in AddListener whether the method is accessible, and throw an exception there if it isn't. Then here you should log an error for debugging purposes.

Other issues to consider:

  1. Thread-safety. This has no thread-safety whatsoever.

  2. Typing. If you're going to emulate C#'s events then don't just emulate the ungenericised EventHandler. At the very least, emulate EventHandler<T>. Because Java's generics aren't reified this requires a bit of boilerplate:

    public class Event<T>
    {
        private final Class<T> typeParam;
    
        public Event(Class<T> typeParam)
        {
            this.typeParam = typeParam;
        }
    

    You can now use typeParam in the Class.getMethod call.

\$\endgroup\$
  • \$\begingroup\$ 1: I don't understand. I'm using the ArrayLists to store the class' data, not store other ArrayLists external to the class. Also, What would make those member names adhere to Java conventions? I thought they did. 2: Noted, I'll change it to ListeningClass. I could see how that could become a problem XD 3: I'm a fan of self-documenting code. The only reason I abbreviated it was because I thought such a long variable name would make method calls needlessly long. Although since I only actually use it twice after declaring it, I'll change that ^^;; \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 18:01
  • \$\begingroup\$ 4: That was a mistake that I only noticed last night. I could have sworn that I'd updated the post when I fixed that, but I guess I must have forgotten. >.< 5: 1: See above. 2: Where I've used getMethod again, I'm getting the method from the methods list to remove. There's probably a better way to go about that actually. 3: I hadn't thought about that. (@ the map bit) 6: I had no idea how to handle that exception, thankyou. 7: I'll look into those, thankyou. I'll post another comment once I've modified my class accordingly. \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 18:01
  • \$\begingroup\$ On 1: Eek, I see what you meant about the Lists/ArrayLists, I misunderstood you, sorry. \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 18:19
  • \$\begingroup\$ @HaniiPuppy, Java naming conventions are that the only names which start with an upper case letter are types and static final fields (which are only upper case). oracle.com/technetwork/java/javase/documentation/… \$\endgroup\$ – Peter Taylor Sep 14 '12 at 19:18
  • \$\begingroup\$ <See my edit on the question.> I thought creating an inner-class for my method/class pairing would solve some of my problems. \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 22:29
0
\$\begingroup\$

Your listener is pair {object, method name}. In Java, typical approach is to represent listener as a single object, which implements specific interface. This way we do not need reflection, things work faster, and errors are discovered at compile time, not run time.

\$\endgroup\$
  • \$\begingroup\$ As an object that implements a specific interface? That brings up a number of the reasons C#-style events would be better for me in the first place. If I wanted to have a class that listens to multiple events, I'd have to create multiple interfaces for each event. That would entail creating different event classes for each event that might be declared. I don't think I'd be much better off than just implementing the observer normally :\ \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 4:14
  • \$\begingroup\$ °Observer pattern normally. \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 15:59
  • \$\begingroup\$ What I have described earlier in the answer is normal java implementation of observer pattern. Creating different event classes for each event is not that hard - you can use anonymous inner classes based on handler interface. As for event handler interfaces, you can use single interface, or different interfaces - its up to you. \$\endgroup\$ – Alexei Kaigorodov Sep 14 '12 at 16:32
  • \$\begingroup\$ It's not hard, but it's not certainly not clean and would likely result in problems down the road in what I'm doing. C#-style events vastly simplifies everything. \$\endgroup\$ – Hanii Puppy Sep 14 '12 at 18:07
0
\$\begingroup\$

C# events are only privately raisable and clearable. This could be achieved with a nested class:

// Wrapper for a method and possible instance
public class EventHandlerDelegate
{
    /** for wrapping static methods */
    public EventHandlerDelegate(Class clazz, string methodName)
    {
        if(clazz == null)
            throw new RuntimeException("clazz was null.");

        try
        {
            Method method = clazz.getMethod(methodName, Object.class, Object.class);
            if(!method.isAccessible())
                throw new RuntimeException(MethodName + "(Object, " + "); is not accessible.");
            else
                this(null, method);
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overloads of method, exists."); }
    }

    public EventHandlerDelegate(Object instance, string methodName)
    {
        if(instance == null)
            throw new RuntimeException("instance was null.");
        try
        {
            Method method = instance.getClass().getMethod(MethodName, Object.class, Object.class);
            if(!method.isAccessible())
                throw new RuntimeException(MethodName + "(Object, " + "); is not accessible.");
            else
                this(instance, method);
        }
        catch (NoSuchMethodException error) { throw new RuntimeException("No such method, or correct overloads of method, exists."); }
    }

    public EventHandlerDelegate(Object instance, Method method)
    {
        if(passedMethod == null)
            throw new RuntimeException("passedMethod were null.");
        else
        {
            _instance = instance;
            _method = method;
        }
    }

    private Object _instance;
    private Method _method;

    @Override
    public boolean equals(Object obj)
    {
        if(obj instanceof EventHandlerDelegate)
        {
            EventHandlerDelegate other = (EventHandlerDelegate)obj;

            return (_instance == other._instance) && _method.equals(other._method);
        }
        else
            return false;
    }

    @Override
    public int hashCode()
    {
        int hash = 5;
        hash = 37 * hash + (_instance != null ? _instance.hashCode() : 0);
        hash = 37 * hash + (_method != null ? _method.hashCode() : 0);
        return hash;
    }

    public void invoke(Object sender, Object args)
    {
        _method.invoke(_instance, sender, arguments);
    }
}

Event class implementing the event registration

public class Event
{
    public Event()
    {
        _handlers = new ArrayList<EventHandlerDelegate>();
        _registrator = new Registrator();
    }

    List<EventHandlerDelegate> _handlers;
    Registrator _registrator;

    public Registrator getRegistrator()
    {
        return registrator;
    }

    public void ClearListeners() { _handlers.clear(); }

    public void Raise(Object sender, Object arguments)
    {
        for(EventHandlerDelegate handler : _handlers)
        {
            try { handler.invoke(sender, arguments); }
            catch (IllegalAccessException error)
            { throw new RuntimeException("This shouldn't have happened ?_?", error); }
            catch (InvocationTargetException error)
            { throw new RuntimeException("Error in event handler", error); }
        }
    }

    public class Registrator
    {
        public boolean addListener(EventHandlerDelegate eventhandler)
        {
            _handlers.add(eventhandler);
        }

        public boolean RemoveListener(EventHandlerDelegate eventhandler)
        {
            handlers.remove(eventhandler);
        }
    }
}

Usage:

class ClassWithEvent
{
    public Event()
    {
        _somethingChangedEvent = new Event();
    }

    private Event _somethingChangedEvent;

    public Registrator getSomethingChangedEvent()
    {
        return _somethingChangedEvent.getRegistrator();
    }
}

{
    ClassWithEvent obj = ...;
    obj.getSomethingChangedEvent().addListener(new EventHandlerDelegate(this, "myHandler"));
}

Note:

  • EventHandlerDelegate is usable on its own and instances can be reused to save on reflection.
  • EventHandlerDelegate can wrap static methods.

TODO:

  • the Event and EventhandlerDelegate could be made generic to have typesafe methods.
\$\endgroup\$

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.