Receiving a JSON string response from a URL

I am using RestTemplate as my HttpClient to execute a URL while the server will return a JSON string as the response. The customer will call this library by passing DataKey object which has userId in it. Earlier I was using AsyncRestTemplate which is part of Spring 4 but in my company they are not supporting Spring 4 in their parent pom so going back to Spring 3 for now.

• Using the given userId, I will find out what are the machines that I can hit to get the data and then store those machines in a LinkedList, so that I can execute them sequentially.

• After that I will check whether the first hostname is in block list or not. If it is not there in the block list, then I will make a URL with the first hostname in the list and execute it and if the response is successful then return the response. But let's say if that first hostname is in the block list, then I will try to get the second hostname in the list and make the url and execute it, so basically, first find the hostname which is not in block list before making the URL.

• Now, let's say if we selected first hostname which was not in the block list and executed the URL and somehow server was down or not responding, then I will execute the second hostname in the list and keep doing this until you get a successful response. But make sure they were not in the block list as well so we need to follow above point.

• If all servers are down or in block list, then I can simply log and return the error that service is unavailable.

I am making a library in which I will have synchronous (getSyncData) and asynchronous (getAsyncData) methods in it.

• getSyncData() - waits until I have a result, returns the result.

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

Below is my DataClient class which will be called by customer and they will pass DataKey object to either getSyncData or getAsyncData method depending on what they

-
I would consider creating an AsyncClient and a SyncClient interface/classes instead of one. I guess users of the current Client class would use only one of the two methods. Furthermore, getSyncData does not use any of the fields of DataClient currently.

Async version:

public class AsyncDataClient {

    private RestTemplate restTemplate = new RestTemplate();
    private ExecutorService service = Executors.newFixedThreadPool(15);

    public Future getAsyncData(DataKey key) {
        DataFetcherTask task = new DataFetcherTask(key, restTemplate);
        Future future = service.submit(task);

        return future;
    }
}

Sync version:

public class SyncDataClient {

    private AsyncDataClient asyncDataClient;

    public SyncDataClient(final AsyncDataClient asyncDataClient) {
        this.asyncDataClient = checkNotNull(asyncDataClient, "asyncDataClient cannot be null");
    }

    public DataResponse getSyncData(DataKey key) {
        ...
        responseFuture = asyncDataClient.getAsyncData(key);
        ...
    }
}

See also: Interface segregation principle

-
In getSyncData the response variable is used for multiple purposes. It could store a valid response and error responses too. I would use separate variables for these purposes for better readability and smaller variable scope:

public DataResponse getSyncData(DataKey key) {
    Future responseFuture = null;

    try {
        responseFuture = asyncDataClient.getAsyncData(key);
        final DataResponse response = responseFuture.get(key.getTimeout(), key.getUnitOfTime());
        return response;
    } catch (TimeoutException ex) {
        final DataResponse response = new DataResponse(DataErrorEnum.CLIENT_TIMEOUT, DataStatusEnum.ERROR);
        responseFuture.cancel(true); // terminating the tasks that have got
                                     // timed out
        return response;
    } catch (Exception ex) {
        return new DataResponse(DataErrorEnum.ERROR_CLIENT, DataStatusEnum.ERROR);
    }
}

It also makes easier to figure out that the only occasion when this method returns null is the try block, when the future returns null.

See also: Effective Java, Second Edition, Item 45: Minimize the scope of local variables

-
In this catch block you loose the cause of the error:

} catch (Exception ex) {
    response = new DataResponse(DataErrorEnum.ERROR_CLIENT, DataStatusEnum.ERROR);
}

An operator would be helpful for (at least) a debug level log message here. It could save you lots of debugging time.

-
DataKey contains at least these methods:

• getTimeout()

• getUnitOfTime()

• getTypeOfFlow()

• getUserId()

• getEntity()

It reminds me a DataRequest object name instead. Consider renaming. For me, DataKey is closer to a cache key class or something like that. Furthermore, getEntity is still smells even in a request class. It might be a third parameter of getAsyncData and the constructor of DataFetcherTask as well.

-
The of hostnames reference could be a simple List instead of

LinkedList hostnames = ...

As far as I see the code does not use any LinkedList specific methods.

See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

-
This:

if (CollectionUtils.isEmpty(hostnames)) {

Could be changed to

if (hostnames.isEmpty()) {

CollectionUtils checks nulls as well, but if it's a null you get a NullPointerException in the for loop earlier.

-
Instead of StringUtils.isEmpty(hostname) I usually prefer StringUtils.isBlank which handles whitespace-only strings too.

-
I don't know how complex is your generateURL method but I would consider moving it to an UrlGenerator method. I would also call it generateUrl for a little bit better readability.

From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:


While uppercase may be more common,
a strong argument can made in favor of capitalizing only the first
letter: even if multiple acronyms occur back-to-back, you can still
tell where one word starts and the next word ends.
Which class name would you rather see, HTTPURL or HttpUrl?

-
You could move this part at the beginning of your method as a guard clause:

// if hostnames are empty, then sending different ERROR ENUM code.
if (hostnames.isEmpty()) {
    dataResponse = new DataResponse(null, DataErrorEnum.PERT_ERROR, DataStatusEnum.ERROR);

For example:

List hostnames = mappings.getListOfHostnames(key.getUserId());
// if hostnames are empty, then sending different ERROR ENUM code.
if (hostnames.isEmpty()) {
    return new DataResponse(null, DataErrorEnum.PERT_ERROR, DataStatusEnum.ERROR);
}

-
Instead of the break statement in the loop you could return immediately:

```
if (response.getStatusCode() == HttpStatus.NO_CONTENT) {
return new DataResponse(response.getBody(), DataErrorEnum.NO_CONTENT, DataStatusEnum.SUCCESS);
} else

StackExchange Code Review Q#87515, answer score: 9