Problem
I had to write a generic wrapper that enables me to pass variable number of arguments for execution of some methods and repeat it up to a few times when it fails because of network connection problems.
What I came up is this class and a helper method. Well I can’t say it’s perfect – it is sufficient for my need, but is there anything that I could have done better ?
The main idea is behind command class
public abstract class Command<R, A1, A2, A3> {
protected final SomeService someService;
private Bundle<A1, A2, A3> bundle;
public Command(SomeService someService) {
this.someService = someService;
}
public void setBundle(Bundle<A1, A2, A3> bundle) {
this.bundle = bundle;
}
public Bundle<A1, A2, A3> getBundle() {
return bundle;
}
public abstract R execute(Bundle<A1, A2, A3> bundle) throws Exception;
}
I have to implement an execute method, which holds some logic which will be performed during runtime of the application. But, it may take up to three arguments and return any type.
An auxiliary class that helps to pass arguments to the Command
class is the Bundle
class.
public class Bundle<A1, A2, A3> {
A1 arg1;
A2 arg2;
A3 arg3;
public Bundle(A1 arg1, A2 arg2, A3 arg3) {
this.arg1 = arg1;
this.arg2 = arg2;
this.arg3 = arg3;
}
public A1 getArg1() {
return arg1;
}
public A2 getArg2() {
return arg2;
}
public A3 getArg3() {
return arg3;
}
public static <T1> Bundle<T1, Void, Void> bundleOf(T1 arg1) {
return new Bundle<>(arg1, null, null);
}
public static <T1, T2> Bundle<T1, T2, Void> bundleOf(T1 arg1, T2 arg2) {
return new Bundle<>(arg1, arg2, null);
}
public static <T1, T2, T3> Bundle<T1, T2, T3> bundleOf(T1 arg1, T2 arg2, T3 arg3) {
return new Bundle<>(arg1, arg2, arg3);
}
}
It creates a generic parameter container that is passed to the Command
s
execute
method.
Finally, the method that we currently need, must repeat some operation up to three times (because of often disconnects), so I came up with this methods based on some SO questions:
private <R, A1, A2, A3> R executeCommand(Command<R, A1, A2, A3> command) {
R result = null;
for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
try {
return command.execute(command.getBundle());
} catch (Exception e) {
logger.error("Exception during performing operation, retrying for the " + attempt + " time", e);
sleepForNMinutes(attempt);
loginToApp();
}
}
return result;
}
Instantation of the Command
class:
Command<Person, String, Void, Void> retrievePersonByNetwork;
retrievePersonByNetwork= new Command<Person, String, Void, Void>(someService) {
public Person execute(Bundle<String, Void, Void> bundle) throws Exception {
return this.someService.retrievePerson(bundle.getArg1());
}
};
Execution:
Bundle<String, Void, Void> bundle = Bundle.bundleOf("userName");
retrievePersonByNetwork.setBundle(bundle);
Person person = executeCommand(retrievePersonByNetwork);
Solution
private <R, A1, A2, A3> R executeCommand(Command<R, A1, A2, A3> command) {
R result = null;
for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
try {
return command.execute(command.getBundle());
} catch (Exception e) {
logger.error("Exception during performing operation, retrying for the " + attempt + " time", e);
sleepForNMinutes(attempt);
loginToApp();
}
}
return result;
}
You never set result
outside the initial declaration. So you could just get rid of the declaration and return null
at the end.
You retry on any exception, but the only retry condition that you mention is a disconnect. Why not create a DisconnectException
and only catch that? The current code will retry things like command
being null
or a divide by zero error, even though those will never succeed.
You sleep and log into the app after the final attempt. Why? What happens if the log in action fails? What happens if the exception isn’t caused by something that breaks the login session?
You sleep for 0 minutes after the first attempt.
You log that you are retrying for the 0, 1, or 2 time. That’s incorrect. Particularly on the last time, when you don’t retry. The grammar also doesn’t quite work.
private <R, A1, A2, A3> R executeCommand(Command<R, A1, A2, A3> command) {
int attempt = 0;
while (true) {
try {
return command.execute(command.getBundle());
} catch (DisconnectException e) {
logger.error("Exception while performing operation, attempt " + ++attempt, e);
}
if (attempt >= MAX_ATTEMPTS) {
return null;
}
sleepForNMinutes(attempt);
loginToApp();
}
}
This version does give up using a for
loop to track the attempts. This allows us to move the end condition to the middle of the loop rather than the beginning. This may or may not be what you want. But if you do, this is one way you could do it.