Как следовать принципу единой ответственности в моем исполнителе HttpClient?

Я использую RestTemplate как мой HttpClient для выполнения URL-адреса, и сервер вернет обратно строку json в качестве ответа. Клиент будет вызывать эту библиотеку, передав DataKey объект, в котором есть userId.

  • Используя заданный userId, я узнаю, какие машины, на которые я могу попасть, получить данные, а затем сохраните эти машины в LinkedList, чтобы я мог выполнять их последовательно.
  • После этого я проверю, находится ли первое имя хоста в списке блоков или нет. Если его нет в списке блоков, тогда я сделаю URL с первым именем хоста в списке и запустим его, и если ответ будет успешным, верните ответ. Но скажем, если это первое имя хоста находится в списке блоков, тогда я попытаюсь получить второе имя хоста в списке и сделать url и выполнить его, так что в основном сначала найти имя хоста, которое не находится в списке блоков до сделав URL.
  • Теперь, скажем, если мы выбрали первое имя хоста, которого не было в списке блоков, и он выполнил URL-адрес, и каким-то образом сервер был отключен или не ответил, тогда я буду выполнять второе имя хоста в списке и продолжать делать это, пока вы не получите успешный ответ. Но убедитесь, что они также не были в списке блоков, поэтому нам нужно следовать за точкой выше.
  • Если все серверы опущены или находятся в списке блоков, я могу просто зарегистрировать и вернуть ошибку, которая недоступна службе.

Ниже мой класс DataClient, который будет вызываться клиентом, и передаст объект DataKey методу getData.

public class DataClient implements Client {

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

    public Future<DataResponse> getData(DataKey key) {
        DataExecutorTask task = new DataExecutorTask(key, restTemplate);
        Future<DataResponse> future = service.submit(task);

        return future;
    }
}

Ниже мой класс DataExecutorTask:

public class DataExecutorTask implements Callable<DataResponse> {

    private DataKey key;
    private RestTemplate restTemplate;

    public DataExecutorTask(DataKey key, RestTemplate restTemplate) {
        this.restTemplate = restTemplate;
        this.key = key;
    }

    @Override
    public DataResponse call() {
        DataResponse dataResponse = null;
        ResponseEntity<String> response = null;

        MappingsHolder mappings = ShardMappings.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
        List<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 (ClientUtils.isEmpty(hostname) || ShardMappings.isBlockHost(hostname)) {
                continue;
            }
            try {
                String url = generateURL(hostname);
                response = restTemplate.exchange(url, HttpMethod.GET, key.getEntity(), String.class);

                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
                ShardMappings.blockHost(hostname);
            }
        }

        if (ClientUtils.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;
    }
}

Мой список блоков продолжает обновляться из другого фонового потока каждые 1 минуту. Если какой-либо сервер отключен и не отвечает, тогда мне нужно заблокировать этот сервер, используя это -

ShardMappings.blockHost(hostname);

И чтобы проверить, находится ли какой-либо сервер в списке блоков или нет, я использую это -

ShardMappings.isBlockHost(hostname);

Я возвращаю SERVICE_UNAVAILABLE, если серверы опущены или в списке блоков, на основе response == null check, не уверены, правильный ли подход или нет.

Я не придерживаюсь принципа единой ответственности здесь, я думаю, вообще. Может ли кто-нибудь представить пример, как лучше всего использовать принцип SRP здесь.

После долгих размышлений мне удалось извлечь класс хостов, как показано ниже, но не уверен, что лучший способ использовать это в моем классе DataExecutorTask .

public class Hosts {

    private final LinkedList<String> hostsnames = new LinkedList<String>();

    public Hosts(final List<String> hosts) {
        checkNotNull(hosts, "hosts cannot be null");
        this.hostsnames.addAll(hosts);
    }

    public Optional<String> getNextAvailableHostname() {
        while (!hostsnames.isEmpty()) {
            String firstHostname = hostsnames.removeFirst();
            if (!ClientUtils.isEmpty(firstHostname) && !ShardMappings.isBlockHost(firstHostname)) {
                return Optional.of(firstHostname);
            }
        }
        return Optional.absent();
    }

    public boolean isEmpty() {
        return hostsnames.isEmpty();
    }
}

Ответы

Ответ 1

Ваше беспокойство действительно. Во-первых, посмотрим, что делает исходный исполнитель данных:

First, it is getting list of hostnames
Next, it loops through every hostnames that do the following things:
    It checks whether the hostname is valid to send request.
    If not valid: skip. 
    Else continue.
        Generate the URL based on hostname
        Send the request
        Translate the request response to domain response
        Handle exceptions
If the hostnames is empty, generate an empty response
Return response

Теперь, что мы можем сделать, чтобы следовать за SRP? Как я вижу, мы можем сгруппировать эти операции в некоторые группы. Я вижу, что эти операции можно разделить на:

HostnameValidator:        checks whether the hostname is valid to send request
--------------
HostnameRequestSender:    Generate the URL
                          Send the request
--------------
HttpToDataResponse:       Translate the request response to domain response
--------------
HostnameExceptionHandler: Handle exceptions

То есть, один подход к де-паролю вашей операции и следовать за SRP. Существует и другой подход, например, для упрощения операций:

First, it is getting list of hostnames
If the hostnames is empty, generate an empty response
Next, it loops through every hostnames that do the following things:
    It checks whether the hostname is valid to send request
    If not valid: remove hostname
    Else: Generate the URL based on hostname
Next, it loops through every valid hostnames that do the following things:
    Send the request
    Translate the request response to domain response
    Handle exceptions
Return response

Затем его также можно разбить на:

HostnameValidator:        checks whether the hostname is valid to send request
--------------
ValidHostnameData:        Getting list of hostnames
                          Loops through every hostnames that do the following things:
                              Checks whether the hostname is valid to send request
                              If not valid: remove hostname
                              Else: Generate the URL based on hostname
--------------
HostnameRequestSender:    Send the request
--------------
HttpToDataResponse:       Translate the request response to domain response
--------------
HostnameExceptionHandler: Handle exceptions

Конечно, есть и другой способ сделать это. И я оставляю детали реализации пустым, потому что есть много способов его реализовать.