What's the most concise / best way to write this java code?

Question

I'm writing a simple distributed java rmi application and I have a bunch of methods that each need to iterate through a map of client interfaces in order to call various other methods on those interfaces, like so:

public void methodX (arg1, arg2) {
  Iterator<String> itr = clients.keySet().iterator;
  while (itr.hasNext()) {
    String name = itr.next();
    if (!"mod".equals(name)) {
      try {
        clients.get(name).methodXX(arg1, arg2);
      } catch(RemoteException ex) {
        // do something
      }
    }
  }
}

public void methodY (arg1, arg2, arg3) {
  Iterator<String> itr = clients.keySet().iterator;
  while (itr.hasNext()) {
    String name = itr.next();
    if (!"mod".equals(name)) {
      try {
        clients.get(name).methodYY(arg1, arg2, arg3);
      } catch(RemoteException ex) {
        // do something
      }
    }
  }
}

Now I modified these so that instead they each call a single method doAll by passing a new argument called MESSAGE_TYPE, like so:

public void methodX (arg1, arg2) {
  doAll(MESSAGE_TYPE.METHODX, arg1, arg2, null);
}

public void methodY (arg1, arg2, arg3) {
  doAll(MESSAGE_TYPE_METHODY, arg1, arg2, arg3);
}

And the doAll method:

public void doAll(msg_type, arg1, arg2, arg3) {
 Iterator<String> itr = clients.keySet().iterator;
  while (itr.hasNext()) {
    String name = itr.next();
    if (!"mod".equals(name)) {
      try {
        switch(msg_type) {
          case METHODX:
            clients.get(name).methodXX(arg1, arg2);
            break;
          case METHODY:
            clients.get(name).methodYY(arg1, arg2, arg3);
            break;
        }
      } catch(RemoteException ex) {
        // do something
      }
    }
  }
}

Now there are many more methods like this, and so my doAll method needs to take a bunch of args and each methodXX that calls it pass a bunch of nulls to it.

Can I rewrite this so it's more concise? If so, can you provide an example?

Answer 1

For one thing, I'd use the enhanced for loop, and iterate over entries instead of keys, as suggested in the comments:

public void doAll(arg1, arg2, arg3) {
  for (Map.Entry<String,Client> entry : clients.entrySet()) {
    if (!"mod".equals(entry.getKey())) {
      try {
        switch(MESSAGE_TYPE) {
          case METHODX:
            entry.getValue().methodXX(arg1, arg2);
            break;
          case METHODY:
            entry.getValue().methodYY(arg1, arg2, arg3);
            break;
        }
      } catch(RemoteException ex) {
        // do something
      }
    }
  }
}

I think I'd then refactor it to pass in an "action" to call on each client, and use an anonymous inner class from the call sites:

public interface RemoteAction {
  public void execute(Client client) throws RemoteException;
}

public void doAll(RemoteAction action) {
  for (Map.Entry<String,Client> entry : clients.entrySet()) {
    if (!"mod".equals(entry.getKey())) {
      try {
        action.execute(entry.getValue());
      } catch(RemoteException ex) {
        // do something
      }
    }
  }
}

public void methodX (final arg1, final arg2) {
  doAll(new Action() {
    @Override public void execute(Client client) throws RemoteException {
      client.methodX(arg1, arg2);
    }
  });
}

public void methodY (final arg1, final arg2, final arg3) {
  doAll(new Action() {
    @Override public void execute(Client client) throws RemoteException {
      client.methodY(arg1, arg2, arg3);
    }
  });
}

It's not as nice as it would be in a language which supported lambda expressions, but it's nicer than a switch statement.

Answer 2

Use generics

Iterator<String> itr = clients.keySet().iterator;
while (itr.hasNext()) {
   String name = itr.next();

becomes

for(String name: clients.keySet()){

Also, replacing methods with a switch/case is not so good, especially if you need to pass in dummy parameters for unused values. Keep the separate methods.

Answer 3

Depending on the context (sometimes the security manager or proxies gets in the way), introspection and varargs are your friends:

You could have something like:

void callStuff(String methodName, Object ... args) 
{
   for(Client client: clients)
   {
     //...filter client by name, method, etc.
     //...figure out parameter types - you can guess from args or pass another parameter
     Method method = client.getClass().getMethod(methodNamename, parameterTypes);
     method.invoke(client,args);
   } 
}

(disclaimer: code above is untested and not event compiled - and I have no clue if it would work with RMI)

Answer 4

I don't know about making your method more concise, but I have a suggestions about the parameters of the doAll method...

public void doAll(int methodType, Object... arg) 
{
    //snip
    switch(msg_type) 
    {
     case METHODX:
      clients.get(name).methodXX(arg[0], arg[1]);
      break;
     case METHODY:
      clients.get(name).methodYY(arg[0], arg[1], arg[2]);
      break;
    }
    //snip
}

That would allow you to pass a variable number of args to the doAll method, alleviating the need for nulls.