Model simulation using Java annotations

Question

A couple of days ago I posted my code that models systems made up of moving parts. I got several great tips and ideas and here is my latest version. My actions are now methods annotated with @Action (instead of separate classes as in my original code). My Model class contains a HashMap of Class, ArrayList<? extends Agent> so each ArrayList contains agents of the same type.

In myModel.start() method I get annotated methods for each class and invoke them on each ArrayList member. The code looks more promising than the original one, but my main concern is that I had to use reflection to invoke actions on each model agent as I read that reflection is costly.

I would also like to hear your opinions on my usage of generics as I am new to the concept.

import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; public class GenericsTest { public static void main(String[] args) throws NoSuchFieldException, SecurityException { Model machine = new Model(); ArrayList<Gear> gears = new ArrayList<Gear>(); ArrayList<Lever> levers = new ArrayList<Lever>(); int i; for (i = 0; i < 1000; i++) { Gear gear = new Gear(); gear.setDiameter(i); gears.add(gear); Lever lever = new Lever(); lever.setLength(i + 2000); levers.add(lever); } machine.addAgentList(Gear.class, gears); machine.addAgentList(Lever.class, levers); machine.start(); } } class Model { public void start() { for (Class<? extends Agent> clazz : listOfAgents.keySet()) { List<Method> actions = new ArrayList<Method>(); for (Method m : clazz.getDeclaredMethods()) { if (m.isAnnotationPresent(Action.class)) { actions.add(m); } } for (Agent agent : listOfAgents.get(clazz)) { for (Method action : actions) { try { action.invoke(agent, new Object[] {}); } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { e.printStackTrace(); } } } } } public Map<Class<? extends Agent>, ArrayList<? extends Agent>> getAgentLists() { return listOfAgents; } public void addAgentList(Class<? extends Agent> cls, ArrayList<? extends Agent> agents) { listOfAgents.put(cls, agents); } private final Map<Class<? extends Agent>, ArrayList<? extends Agent>> listOfAgents = new HashMap<Class<? extends Agent>, ArrayList<? extends Agent>>(); } interface Agent { } @Retention(RetentionPolicy.RUNTIME) @Target(value = ElementType.METHOD) @interface Action { } /* * Agent classes */ class Gear implements Agent { private int diameter; public int getDiameter() { return diameter; } public void setDiameter(int diameter) { this.diameter = diameter; } @Action public void rotate() { System.out.println("rotating ... " + this.toString() + " " + this.diameter); } @Action public void replace() { System.out.println("replacing ..." + this.toString() + " " + this.diameter); } } class Lever implements Agent { private int length; public int getLength() { return length; } public void setLength(int length) { this.length = length; } @Action public void move() { System.out.println("moving ..." + this.toString() + " " + this.length); } } 

I will also try improve my original FactoryClass idea making Action an interface and post the code in another post.

EDIT

Changed the code to improve my list declarations:

import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; public class GenericsTest { public static void main(String[] args) throws NoSuchFieldException, SecurityException { Model machine = new Model(); List<Gear> gears = new ArrayList<Gear>(); List<Lever> levers = new ArrayList<Lever>(); int i; for (i = 0; i < 1000; i++) { Gear gear = new Gear(); gear.setDiameter(i); gears.add(gear); Lever lever = new Lever(); lever.setLength(i + 2000); levers.add(lever); } machine.addAgentList(Gear.class, gears); machine.addAgentList(Lever.class, levers); machine.start(); } } class Model { public void start() { for (Class<? extends Agent> clazz : listOfAgents.keySet()) { List<Method> actions = new ArrayList<Method>(); for (Method m : clazz.getDeclaredMethods()) { if (m.isAnnotationPresent(Action.class)) { actions.add(m); } } for (Agent agent : listOfAgents.get(clazz)) { for (Method action : actions) { try { action.invoke(agent, new Object[] {}); } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { e.printStackTrace(); } } } } } public Map<Class<? extends Agent>, List<? extends Agent>> getAgentLists() { return listOfAgents; } public void addAgentList(Class<? extends Agent> cls, List<? extends Agent> agents) { listOfAgents.put(cls, agents); } private final Map<Class<? extends Agent>, List<? extends Agent>> listOfAgents = new HashMap<Class<? extends Agent>, List<? extends Agent>>(); } interface Agent { } @Retention(RetentionPolicy.RUNTIME) @Target(value = ElementType.METHOD) @interface Action { } /* * Agent classes */ class Gear implements Agent { private int diameter; public int getDiameter() { return diameter; } public void setDiameter(int diameter) { this.diameter = diameter; } @Action public void rotate() { System.out.println("rotating ... " + this.toString() + " " + this.diameter); } @Action public void replace() { System.out.println("replacing ..." + this.toString() + " " + this.diameter); } } class Lever implements Agent { private int length; public int getLength() { return length; } public void setLength(int length) { this.length = length; } @Action public void move() { System.out.println("moving ..." + this.toString() + " " + this.length); } } 

Answer 1

1. private final Map<Class<? extends Agent>, List<? extends Agent>> listOfAgents = new HashMap<Class<? extends Agent>, List<? extends Agent>>(); 

   The map creation would be readable with Java 7's diamond operator

   private final Map<Class<? extends Agent>, List<? extends Agent>> listOfAgents = new HashMap<>(); 

   or Guava's newHashMap.

   private final Map<Class<? extends Agent>, List<? extends Agent>> listOfAgents = newHashMap(); 

   (I like the final keyword here, I saved me from searching code which could change the reference.)

2. According to Code Conventions for the Java Programming Language , 3 - File Organization field declarations should be before methods. (Model.listOfAgents).

3. I've seen earlier the listOfAgents style variables but I've found agents or agentList more easier to work with. Maybe because if more than one variable starts with listOf it's hard to differentiate them. Hm, I've just noticed that it's actually a map. Then, the list is a little bit misleading. I think agents or agentMap could be better.

4. Instead of maps like

   Map<X, List<Y>> 

   use a Multimap. Guava has great implementations. (Doc, Javadoc) It's easier to read. (The same is true for new ArrayList<Method>()).

5. System.out.println("moving ..." + this.toString() + " " + this.length); 

   You don't need this. here. Modern IDEs use highlighting to separate local variables from instance variables.

6. I'd rename Method m to method for better readability. If you just look at the actions.add(m) line you don't have to search its type to figure out what it is, it's more obvious.

7. I'd move the isAnnotationPresent check to a separate method with an additional check:

   private boolean isActionMethod(Method method) { if (!method.isAnnotationPresent(Action.class)) { return false; } if (method.getParameterTypes().length != 0) { return false; } return true; } 

   It prevents runtime errors when the action method has parameters.

8. Extracting out another methods would increase the abstarction level of the code:

   public void start() { for (final Class<? extends Agent> clazz: agentMap.keySet()) { final List<Method> actionMethods = getActionMethods(clazz); for (final Agent agent: agentMap.get(clazz)) { runAgentActions(agent, actionMethods); } } } private List<Method> getActionMethods(Class<? extends Agent> clazz) { final List<Method> actions = new ArrayList<Method>(); for (final Method method: clazz.getDeclaredMethods()) { if (isActionMethod(method)) { actions.add(method); } } return actions; } private void runAgentActions(Agent agent, List<Method> actionsMethods) { for (final Method actionMethod: actionsMethods) { try { actionMethod.invoke(agent, new Object[] {}); } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { e.printStackTrace(); } } } 

   It provides an overview in the start method about what it does (without the details) and you can still check them if you are interested in.

9. If you don't switch to Multimap iterate over Map.entrySet() instead of keySet and get. It's faster.