Solved Creating Event Annotations with Priority

Discussion in 'Plugin Development' started by Blockhead7360, Oct 8, 2016.

Thread Status:
Not open for further replies.
  1. Offline

    Blockhead7360

    Hello!

    A few weeks ago, I made an EventHandler-type thing for my plugin. It acts the same as @EventHandler, but instead, it is handled through my plugin (different custom events).

    It was working fine, until I realized that it would be much better to have a priority to change the order that the events were sent in.

    Here is the class where I throw the events:
    Code:
        public static Map<Integer, List<Class_METHOD_STORAGE>> invoker = new HashMap<Integer, List<Class_METHOD_STORAGE>>();
       
        public static void go(OtherEvent x) throws Exception{
            for (TestEvents e : TestPlugin.testevents){
                Method[] methods = e.getClass().getMethods();
                for (Method method : methods){
                   
    
                    if (method.isAnnotationPresent(TestEventAnnotation.class)){
                        if (method.getParameters().length == 0) continue;
                        if (method.getParameterTypes()[0].equals(x.getClass())){
                            int p = method.getAnnotation(TestEventAnnotation.class).priority();
                            if (invoker.containsKey(p)){
                                List<Class_METHOD_STORAGE> m = invoker.get(p);
                                Class_METHOD_STORAGE cms = new Class_METHOD_STORAGE(method, e.getClass(), x);
                                m.add(cms);
                                invoker.put(p, m);
                            }else{
                                List<Class_METHOD_STORAGE> m = new ArrayList<Class_METHOD_STORAGE>();
                                Class_METHOD_STORAGE cms = new Class_METHOD_STORAGE(method, e.getClass(), x);
                                m.add(cms);
                                invoker.put(p, m);
                            }
                        }
                    }
                }
               
            }
            invokeAll();
           
        }
        public static void invokeAll() throws IllegalAccessException, IllegalArgumentException, InvocationTargetException, InstantiationException{
            List<Integer> integer = new ArrayList<Integer>();
            for (int i : invoker.keySet()){
                integer.add(i);
            }
            Collections.sort(integer);
            for (int i : integer){
                List<Class_METHOD_STORAGE> m = invoker.get(i);
                for (Class_METHOD_STORAGE cms : m){
                    Method method = cms.getMethod();
                    Class<?> clasz = cms.getClazz();
                    OtherEvent x = cms.getParameter();
                    method.invoke(clasz.newInstance(), x.getClass());
                }
            }
        }
    (By the way, all the "TestEvent" and "TestPlugin" aren't actually in my plugin, I just changed the name for an example because I don't really want to give away what my new plugin is about.)

    Here is what the annotation class looks like:
    Code:
    @Target(ElementType.METHOD)
    @Retention(RetentionPolicy.RUNTIME)
    public @interface TestEventAnnotation {
       
        int priority() default 10;
    }
    The Class_METHOD_STORAGE class is simply just a class where I can store multiple pieces of information in 1 group. As you can see from above, I define a constructor with 3 parameters for the Class_METHOD_STORAGE, and then later on, I take those 3 parameters back out with cms.get<Par>()

    However, in the console, I get an InstantiationException error and a NoSuchMethodException.

    Can anyone help? Thanks.

    P.S. I've never worked with creating my own annotations before, so...
     
  2. Offline

    mythbusterma

    @Blockhead7360

    Why do you need this instead of just using Bukkit's event system? It doesn't really make any sense to me why you wouldn't, as Bukkit will let you work with anything that extends the base class "Event," so you can create your own events and use Bukkit's handling system to fire them, register their handlers, etc.

    Also, the code that you've posted demonstrates that you're out of your league with this sort of stuff.

    You can see how to use Bukkit's system here: http://wiki.bukkit.org/Event_API_Reference
     
    bwfcwalshy likes this.
  3. Offline

    Blockhead7360

    @mythbusterma

    Never mind. I fixed it myself. Instead of using Class<?>, I used instances of the class instead.
     
  4. Offline

    I Al Istannen

    @Blockhead7360
    The question why you are reinventing the wheel remains though ;)

    You could also use a SortedMap (i.e. TreeMap) and just loop over it's entry set. This will eliminate this chunk of code and allows you to change the priority later with a simple comparator.
    Code:
            List<Integer> integer = new ArrayList<Integer>();
            for (int i : invoker.keySet()){
                integer.add(i);
            }
            Collections.sort(integer);
            for (int i : integer){
                List<Class_METHOD_STORAGE> m = invoker.get(i);

    Apart from that, "Class_METHOD_STORAGE" seems like a bad name for a class. Consider refactoring this.

    And I am quite sure taking an Event as the parameter to invokeAll, which is the passed around is going to be better, as you can add values to it. Not just creating a new, empty one.
    Code:
    method.invoke(clasz.newInstance(), x.getClass());
    "if (invoker.containsKey(p)){"
    In Java 8 you can just use "invoker.getOrDefault(new ArrayList<>)" which saves this if and most of the duplicated code.

    There are probably more not optimal solutions in your code. These are just some things I found odd. They may be valid, but with my current knowledge I would do it differently.


    You are probably really best off using Bukkits system. What are your concerns with it? Maybe we can help circumvent them :)
     
  5. Offline

    mythbusterma

    @I Al Istannen @Blockhead7360

    Not to mention the fact that he assumes that it has a zero argument constructor, and creates a new instance each time the event is handled. That is extremely counter-intuitive behavior, and would drive anyone trying to use your plugin up a wall.
     
  6. Offline

    Blockhead7360

    @mythbusterma

    How would it inconvenience someone using my plugin?
     
  7. Offline

    mythbusterma

    @Blockhead7360

    Well first, it uses a non-standard system, second, they can't have a single instance of the listener class, which is what they would expect to be able to do .
     
  8. Offline

    Blockhead7360

  9. Offline

    I Al Istannen

    @Blockhead7360
    Code:
    Class<?> clasz = cms.getClazz();
    method.invoke(<- HERE: clasz.newInstance() ->, x.getClass());
    
    You create a new instance every time it is invoked. You use newInstance, so you assume there is a default constructor. Which doesn't seem to be the case from the console errors.

    And what @mythbusterma probably meant is this:
    You have a Listener class, which has a List. This list contains some player data.
    On each invocation (invokeAll), a new instance of the listener class is created ==> A new list too
    This will result in the listener not being functional
    You may have fixed this in your new version, but we don't know the code for that. But I think you are re-inventing the wheel. While it may be fun and a nice experience, the outcome will probably be not as good as the way Bukkit has done it. The code bukkit uses was (hopefully!) much more carefully though out (no offence) and written by experienced developers. I am guilty of this too though, so I maybe should not be a hypocrite xD
     
  10. Offline

    Blockhead7360

    Ahh I think I know what you guys are saying. In my updated code, I no longer had to create new instances, as I was already using an instance of the class. Does this make sense? Sorry, I forgot to post my final code
     
  11. Offline

    I Al Istannen

    @Blockhead7360
    Yea, makes sense. Allows the usage of one listener class now, but I still don't see any benefit. But this is for you to decide. If it works, is maintainable and makes somewhat sense, I won't judge you :p

    Could you post how you have done it now?
     
  12. Offline

    Blockhead7360

    I can't post the code right now but what I changed was instead of clasz being Class<?>, it is an instance of the actual listener (classes that implement my listener). And then instead of using .newInstance in the invoke method, I don't.
     
  13. Offline

    I Al Istannen

    @Blockhead7360
    I see. Makes sense. I hope you refactored the rest :p [names, maybe a TreeMap instead (to set the execution order) and the other stuff I wrote. Just consider it, no need to adapt.]

    Thanks though :)
     
  14. Offline

    Blockhead7360

    Got it thanks!
     
Thread Status:
Not open for further replies.

Share This Page