Какой самый краткий / лучший способ написать этот код Java?

Question

Я пишу простое распределенное Java-приложение RMI, и у меня есть куча методов, каждый из которых должен перебирать карту клиентских интерфейсов для вызова различных других методов на этих интерфейсах, например:

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

Теперь я изменил их так, чтобы вместо этого каждый из них вызывал один метод doAll, передавая новый аргумент с именем MESSAGE_TYPE, например:

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);
}

И метод doAll:

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

Теперь есть еще много подобных методов, и поэтому мой метод doAll должен принимать несколько аргументов, и каждый вызывающий его метод XX передает ему множество нулей.

Могу ли я переписать это, чтобы оно было более кратким? Если да, можете ли вы привести пример?

Answer 1

Во-первых, я бы использовал расширенный цикл for и перебрал записи вместо ключей, как это предлагается в комментариях:

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

Я думаю, что тогда я бы реорганизовал его, чтобы передать "действие" для вызова каждого клиента и использовать анонимный внутренний класс из сайтов вызовов:

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);
    }
  });
}

Это не так хорошо, как было бы на языке, который поддерживает лямбда-выражения, но это лучше, чем оператор switch.

Answer 2

Не знаю, как сделать ваш метод более лаконичным, но у меня есть предложения по поводу параметров метода doAll ...

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
}

Это позволит вам передавать переменное число аргументов в метод doAll, устраняя необходимость в значениях NULL.

Answer 3

В зависимости от контекста (иногда мешают менеджер безопасности или прокси), интроспекция и varargs ваши друзья:

Вы можете иметь что-то вроде:

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);
   } 
}

(отказ от ответственности: приведенный выше код не проверен и не скомпилирован по событию - и я понятия не имею, будет ли он работать с RMI)

Answer 4

Используйте дженерики

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

становится

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

Кроме того, замена методов переключателем / регистром не очень хороша, особенно если вам необходимо передать фиктивные параметры для неиспользуемых значений. Сохраняйте отдельные методы.