Receiving a JSON string response from a URL

Question

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 want to call. In general some customer will call getSyncData method and some customer might call getAsyncData method.

public class DataClient implements Client {
 private RestTemplate restTemplate = new RestTemplate();
 private ExecutorService service = Executors.newFixedThreadPool(15);
 @Override
 public DataResponse getSyncData(DataKey key) {
 DataResponse response = null;
 Future<DataResponse> responseFuture = null;
 try {
 responseFuture = getAsyncData(key);
 response = responseFuture.get(key.getTimeout(), key.getUnitOfTime());
 } catch (TimeoutException ex) {
 response = new DataResponse(DataErrorEnum.CLIENT_TIMEOUT, DataStatusEnum.ERROR);
 responseFuture.cancel(true); // terminating the tasks that have got timed out
 } catch (Exception ex) {
 response = new DataResponse(DataErrorEnum.ERROR_CLIENT, DataStatusEnum.ERROR);
 }
 return response;
 } 
 @Override
 public Future<DataResponse> getAsyncData(DataKey key) {
 DataFetcherTask task = new DataFetcherTask(key, restTemplate);
 Future<DataResponse> future = service.submit(task);
 return future;
 }
}

DataFetcherTask class:

public class DataFetcherTask implements Callable<DataResponse> {
 private DataKey key;
 private RestTemplate restTemplate;
 public DataFetcherTask(DataKey key, RestTemplate restTemplate) {
 this.key = checkNotNull(key);
 this.restTemplate = checkNotNull(restTemplate);
 }
 // can we simplify this?
 // I tried thinking a lot but not sure how to split this up which follows SRP
 @Override
 public DataResponse call() {
 DataResponse dataResponse = null;
 ResponseEntity<String> response = null;
 MappingsHolder mappings = ShardMapping.getMappings(key.getTypeOfFlow());
 // given a userId, find all the hostnames 
 // it can also have four hostname or one hostname or six hostname as well in the list
 LinkedList<String> hostnames = mappings.getListOfHostnames(key.getUserId());
 for (String hostname : hostnames) {
 // If host name is null or host name is in local block list, skip sending request to this host
 if (StringUtils.isEmpty(hostname) || ShardMapping.isBlockHost(hostname)) {
 continue;
 }
 try {
 String url = generateURL(hostname);
 response = restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class);
 // if the status code is NO_CONTENT, then send that as well
 // otherwise send OK
 if (response.getStatusCode() == HttpStatus.NO_CONTENT) {
 dataResponse = new DataResponse(response.getBody(), DataErrorEnum.NO_CONTENT,
 DataStatusEnum.SUCCESS);
 } else {
 dataResponse = new DataResponse(response.getBody(), DataErrorEnum.OK,
 DataStatusEnum.SUCCESS);
 }
 break;
 // below codes are duplicated looks like
 } catch (HttpClientErrorException ex) {
 HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
 DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
 String errorMessage = httpException.getResponseBodyAsString();
 dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
 return dataResponse;
 } catch (HttpServerErrorException ex) {
 HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
 DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
 String errorMessage = httpException.getResponseBodyAsString();
 dataResponse = new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
 return dataResponse;
 } catch (RestClientException ex) {
 // if it comes here, then it means some of the servers are down so adding it into block list
 ShardMapping.blockHost(hostname);
 }
 }
 // if hostnames are empty, then sending different ERROR ENUM code.
 if (CollectionUtils.isEmpty(hostnames)) {
 dataResponse = new DataResponse(null, DataErrorEnum.PERT_ERROR, DataStatusEnum.ERROR);
 } else if (response == null) { // either all the servers are down or all the servers were in block list
 dataResponse = new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR);
 }
 return dataResponse;
 }
}

My block list keeps getting updated from another background thread every 1 minute. If any server is down and not responding, then I need to block that server by using this -

ShardMapping.blockHost(hostname);

And to check whether any server is in block list or not, I use this -

ShardMapping.isBlockHost(hostname);

I am returning SERVICE_UNAVAILABLE if servers are down or in block list, on the basis of response == null check, not sure whether it's a right approach or not.

I want to know if I am following the Single Responsibility Principle properly here or not and if the above code can be simplified?

Answer 1

1. 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<DataResponse> getAsyncData(DataKey key) {
    DataFetcherTask task = new DataFetcherTask(key, restTemplate);
    Future<DataResponse> 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

2. 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<DataResponse> 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

3. 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.

4. 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.

5. The of hostnames reference could be a simple List<String> instead of

   LinkedList<String> 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

6. 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.

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

8. 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?

9. 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<String> 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);
   }
   

10. 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 {
     return new DataResponse(response.getBody(), DataErrorEnum.OK, DataStatusEnum.SUCCESS);
    }
    

    It also helps to make the scope of dataResponse variable smaller. Actually, you don't need it at all and you could get rid of the null comparison at the end of the method too:

    @Override
    public DataResponse call() {
     ...
     List<String> hostnames = mappings.getListOfHostnames(key.getUserId());
     if (hostnames.isEmpty()) {
     return new DataResponse(null, DataErrorEnum.PERT_ERROR, DataStatusEnum.ERROR);
     }
     for (String hostname: hostnames) {
     ...
     try {
     ...
     ResponseEntity<String> response = restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class);
     ...
     if (response.getStatusCode() == HttpStatus.NO_CONTENT) {
     return new DataResponse(response.getBody(), DataErrorEnum.NO_CONTENT, DataStatusEnum.SUCCESS);
     } else {
     return new DataResponse(response.getBody(), DataErrorEnum.OK, DataStatusEnum.SUCCESS);
     }
     } catch (HttpClientErrorException ex) {
     ...
     return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
     } catch (HttpServerErrorException ex) {
     ...
     return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
     } catch (RestClientException ex) {
     // if it comes here, then it means some of the servers are down so adding it into block list
     ShardMapping.blockHost(hostname);
     }
     }
     // either all the servers are down or all the servers were in block list
     return new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE, DataStatusEnum.ERROR);
    }
    

    Also note the scope change of response.

11. I don't see why you need this casting:

    } catch (HttpClientErrorException ex) {
     HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
    

    and this one:

    } catch (HttpServerErrorException ex) {
     HttpStatusCodeException httpException = (HttpStatusCodeException) ex;
    

    Since HttpStatusCodeException is a superclass of both HttpClientErrorException and HttpServerErrorException the following is the same:

     } catch (HttpClientErrorException ex) {
     HttpStatusCodeException httpException = ex;
     ...
     } catch (HttpServerErrorException ex) {
     HttpStatusCodeException httpException = ex;
    

    Furthermore, HttpStatusCodeException has only these two subclasses in Spring and the body of both catch clauses are the same so you could simply catch only HttpStatusCodeException here:

    } catch (HttpStatusCodeException httpException) {
     DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
     String errorMessage = httpException.getResponseBodyAsString();
     return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
    } catch (RestClientException ex) {
     // if it comes here, then it means some of the servers are down so adding it into block list
     ShardMapping.blockHost(hostname);
    }
    

    Keep in my that anyone can create a new subclass of HttpStatusCodeException so that's might not what you want.

    If you're using Java 7 or later, you could use multi-catch with one catch block:

    } catch (HttpClientErrorException | HttpServerErrorException ex) {
    

    Another solution is extracting out the common code into a method:

     private DataResponse createErrorResponse(HttpStatusCodeException httpException) {
     DataErrorEnum error = DataErrorEnum.getErrorEnumByException(httpException);
     String errorMessage = httpException.getResponseBodyAsString();
     return new DataResponse(errorMessage, error, DataStatusEnum.ERROR);
    }
    

    Usage:

    } catch (HttpClientErrorException ex) {
     return createErrorResponse(ex);
    } catch (HttpServerErrorException ex) {
     return createErrorResponse(ex);
    

12. responseFuture.cancel(true) is in a catch block. I have not checked but I would try to move it into a finally block. If another (non-timeout exception) happens you won't use the future anyway.

13. getUnitOfTime does not suggest any connection with the getTimeout method. I would rename it to getTimeoutUnit.

• Billal BEGUERADJ: You have many elaborated and good answers, why you no longer participate on this website?