Model simulation using Java annotations

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 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 gears = new ArrayList();
ArrayList levers = new ArrayList();

int i;
for (i = 0; i clazz : listOfAgents.keySet())
{
List actions = new ArrayList();
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 (

-

private final Map, List> listOfAgents =
    new HashMap, List>();

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

private final Map, List> listOfAgents =
    new HashMap<>();

or Guava's newHashMap.

private final Map, List> listOfAgents =
    newHashMap();

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

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

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

-
Instead of maps like

Map>

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

-

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.

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

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

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

public void start() {
    for (final Class clazz: agentMap.keySet()) {
        final List actionMethods =
            getActionMethods(clazz);

        for (final Agent agent: agentMap.get(clazz)) {
            runAgentActions(agent, actionMethods);
        }
    }
}

private List getActionMethods(Class clazz) {
    final List actions = new ArrayList();
    for (final Method method: clazz.getDeclaredMethods()) {
        if (isActionMethod(method)) {
            actions.add(method);
        }
    }
    return actions;
}

private void runAgentActions(Agent agent, List 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.

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

StackExchange Code Review Q#42332, answer score: 3