Synchronous and asynchronous behavior for client

I am working on a project in which I am supposed to make synchronous and asynchronous behavior of my client.

The customer will call our client with a userId and we will construct a URL from that userId and make an HTTP call to that URL and we will get a JSON string back after hitting the URL. And after we get the response back as a JSON string, we will send that JSON string back to our customer.

In this case, I need to have synchronous and asynchronous methods. Some customers will call the executeSynchronous method to get the same feature and some customers will call our executeAsynchronous method to get the data back.

Interface:

public interface Client {

    // for synchronous
    public String executeSynchronous(final String userId);

    // for asynchronous
    public Future executeAsynchronous(final String userId);
}

SmartClient which implements the Client interface:

public class SmartClient implements Client {

    ExecutorService executor = Executors.newFixedThreadPool(5);

    // This is for synchronous call
    @Override
    public String executeSynchronous(String userId) {

        String response = null;
        Future handle = getHandle(userId);

        try {
            response = handle.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {

        }

        return response;
    }

    // This is for asynchronous call
    @Override
    public Future executeAsynchronous(String userId) {

        return getHandle(userId);

    }

    private Future getHandle(String userId) {
        Future future = null;

        Task task = new Task(userId);
        future = executor.submit(task);

        return future;

    }
}

Simple class which will perform the actual task:

```
class Task implements Callable {

private final String userId;

public Task(String userId) {
this.userId = userId;
}

public String call() throws Exception {

String url = createURL(userId);

/

Late answer to this question.....

There are a few things which should be considered when implementing a solution like this, and best-practice comes in to play here.


One nit-pick ... before we go further, your interface the methods
executeAsynchronous and executeSynchronous, whereas your
implementation has executeAsync and execute. This is a carelessly
posted question?

What you have, at the moment, works, but is it the best way? I don't think so.

You code has a few features that are important:

• executeSynchronous() - waits until you have a result, returns the result.

• executeAsynchronous() - returns a Future immediately which can be processed after other things are done, if needed.

• it has a private getHandle() method which does the hard work of creating a Callable, and returning a future.

Now, I have tried to list why there are problems with this, but, it's easier to actually show you a different approach, and explain why it is better.... so, here's a different approach, which reverses the logic.

First thing, we are going to make the interface a generic interface... why does it always have to return a String value for each URL? Also, why is it a Client? It is not really a client, but it is a RemoteCall. This is not the full Client, but a part of what it does when it talks to the server.... so, let's rename the Client interface, and make it generic.... Also, while we are at it, you need to declare that you throw some form of exception... my preference is for a new checked exception type, but maybe you can borrow an existing exception. What you have is not best-practice. Here's a decent exception to throw:

class RemoteCallException extends Exception {

    private static final long serialVersionUID = 1L;

    public RemoteCallException(String message, Throwable cause) {
        super(message, cause);
    }

    public RemoteCallException(String message) {
        super(message);
    }

}

Then, with that exception, we can complete the interface:

public interface RemoteCall {

    // for synchronous
    public T executeSynchronous(final String action) throws RemoteCallException;

    // for asynchronous
    public Future executeAsynchronous(final String action);
}

Right, this interface is now a better representation of the work it is supposed to do.

Now, here's a key feature I want you to consider.... at the moment, you may only have one 'action' you need to execute on the server, the part of translating a userid in to a JSOM response. But, I expect there is, or will be, more. Having to build a new implementation for your Client interface for every type of operation is a real PITA, especially when it requires building a new Callable / Task instance.... so, what we do is we create a convenient abstract class, which will do all the synchronous/asynchronous work for us....

Note, it does not implement executeSynchronous(), and it is still fully generic. It uses an executor service that is passed in.

One of the really huge advantages of reversing the logic (there is no Callable involved in the synchronous call) is that the work is done on the calling thread. In your implementation, you have two threads fully occupied, the calling thread waits for work to be done, and a thread in the ExecutorService is actually doing the work. By making the executeSynchronous() call the way I suggest, only one thread is occupied, and the thread that is busy is the same thread that calls executeSynchronous()

public abstract class AbstractRemoteCall implements RemoteCall {

    private final ExecutorService executor;

    public AbstractRemoteCall(ExecutorService executor) {
        this.executor = executor;
    }

    // note, final so it cannot be overridden in a sub class.
    // note, action is final so it can be passed to the callable.
    public final Future executeAsynchronous(final String action) {

        Callable task = new Callable() {

            @Override
            public T call() throws RemoteCallException {
                return executeSynchronous(action);
            }

        };

        return executor.submit(task);
    }
}

OK, so, the above class deals with any asynchronous method calls, and delegates the synchronous calls to the actual implementations.

Now we get to your actual implementation you want to do, the userid -> JSON transformation. It is now as easy as:

```
public class SmartClient extends AbstractRemoteCall {

public SmartClient() {
super(Executors.newFixedThreadPool(5));
}

@Override
public String executeSynchronous(String userId) throws RemoteCallException {

String url = createURL(userId);

// make a HTTP call to the URL
RestTemplate restTemplate = new RestTemplate();

return restTemplate.getForObject(url, String.class);

}

// create a URL
private String createURL(String userId) {
String generateURL = somecode;

return gen

StackExchange Code Review Q#39123, answer score: 11