Prośba o ocenę kodu klasy.

Cześć.

W związku z tym, że pierwszy kod mi się nie podobał i nie potrafiłem go przetestować zrefaktorowałem go. Pytanie czy wyszło to na lepsze ? Na pewno łatwiej to przetestować.

Stara wersja:

class ApiFootballConsumer {

    @Value("${api.apiAuthToken}")
    private String apiToken;

    List<Game> getGameEntityCollection(List<String> urls){
        return createListOfGameEntity(createMatchesExternalApi(urls));
    }

    List<MatchesExternalApi> createMatchesExternalApi(List<String> urls) {
        List<MatchesExternalApi> listOfMatchesExternalApi = new ArrayList<>();
        HttpHeaders headers = new HttpHeaders();
        headers.set("X-Auth-Token", apiToken);
        HttpEntity<String> entity = new HttpEntity<>("parameters", headers);
        RestTemplate restTemplate = new RestTemplate();
        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setObjectMapper(new ObjectMapper());
        restTemplate.getMessageConverters().add(converter);
        urls.forEach(url -> {
            ResponseEntity<MatchesExternalApi> matchesExternalApiResponseEntity = restTemplate.exchange(
                    url,
                    HttpMethod.GET,
                    entity,
                    MatchesExternalApi.class);
            listOfMatchesExternalApi.add(matchesExternalApiResponseEntity.getBody());
        }
        );
        return listOfMatchesExternalApi;
    }

    List<Game> createListOfGameEntity(List<MatchesExternalApi> matchesExternalApi) {
        List<Game> listOfGames = new ArrayList<>();
            matchesExternalApi.forEach(externalMatches -> {
                externalMatches.getMatches().forEach(p -> {
                    listOfGames.add(new Game(
                            p.getId(),
                            new Competition(externalMatches.getCompetition().getId(),
                                            externalMatches.getCompetition().getName()),
                            DateParser.getDateFromJson(p.getUtcDate()),
                            DateParser.getTimeFromJson(p.getUtcDate()),
                            p.getStatus(),
                            new Team(p.getHomeTeam().getId(), p.getHomeTeam().getName()),
                            new Team(p.getAwayTeam().getId(), p.getAwayTeam().getName()),
                            p.getScore().getWinner())
                    );
                    }
                );
            }
            );
        return listOfGames;
    }

}

Nowa wersja:

class ApiFootballConsumer {

    @Value("${api.apiAuthToken}")
    private String apiToken;
    RestTemplate restTemplate = new RestTemplate();


    List<Game> getGameEntityCollection(List<String> urls){
        HttpEntity<String> httpEntity = createHttpEntityAndSetHeader();
        RestTemplate restTemplate = createRestTemplateAndAddConverter();
        List<MatchesExternalApi> list = getListOfMatchesExternalApi(urls,httpEntity,restTemplate);
        return createListOfGameEntity(list);
    }

    HttpEntity<String> createHttpEntityAndSetHeader(){
        HttpEntity<String> httpEntity = new HttpEntity<>("parameters", createHttpHeaderWithApiToken());
        return httpEntity;
    }

    HttpHeaders createHttpHeaderWithApiToken() {
        HttpHeaders headers = new HttpHeaders();
        headers.set("X-Auth-Token", apiToken);
        return headers;
    }

    RestTemplate createRestTemplateAndAddConverter() {
        this.restTemplate.getMessageConverters().add(createMappingJacksonConverter());
        return restTemplate;
    }

    MappingJackson2HttpMessageConverter createMappingJacksonConverter() {
        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setObjectMapper(new ObjectMapper());
        return converter;
    }

    List<MatchesExternalApi> getListOfMatchesExternalApi (List<String> urls, HttpEntity<String> entity, RestTemplate restTemplate){
        List<MatchesExternalApi> listOfMatchesExternalApi = new ArrayList<>();
        urls.forEach(url -> {
                    ResponseEntity<MatchesExternalApi> matchesExternalApiResponseEntity = restTemplate.exchange(
                            url,
                            HttpMethod.GET,
                            entity,
                            MatchesExternalApi.class);
                    listOfMatchesExternalApi.add(matchesExternalApiResponseEntity.getBody());
                }
        );
        return listOfMatchesExternalApi;
    }
    
    List<Game> createListOfGameEntity(List<MatchesExternalApi> matchesExternalApi) {
        List<Game> listOfGames = new ArrayList<>();
            matchesExternalApi.forEach(externalMatches -> {
                externalMatches.getMatches().forEach(p -> {
                    listOfGames.add(new Game(
                            p.getId(),
                            new Competition(externalMatches.getCompetition().getId(),
                                            externalMatches.getCompetition().getName()),
                            DateParser.getDateFromJson(p.getUtcDate()),
                            DateParser.getTimeFromJson(p.getUtcDate()),
                            p.getStatus(),
                            new Team(p.getHomeTeam().getId(), p.getHomeTeam().getName()),
                            new Team(p.getAwayTeam().getId(), p.getAwayTeam().getName()),
                            p.getScore().getWinner())
                    );
                    }
                );
            }
            );
        return listOfGames;
    }

• Jakoś ciężko mi uwierzyć, że wszystko jest package i nic private - nie wiadomo co ta klasa oferuje.
• getGameEntityCollection - nie dość, że zwraca List a nie Collection to jeszcze to słowo Collection nic nie wnosi + Entity również nic nie wnosi (bo nie widzę na razie GameNieEnetity) - nie lepiej getGames, createGames, getMatches
• Spacje po przecinkach
• ExternalApi pojawia się jakoś w nieoczekiwanych miejscach - ciężko zrozumieć co tam przekazujesz (nota bene sama nazwa klasy - jest to konsument futbolu czy konsument API?)
• Dziwaczna inicjalizacja restTemplate - dlaczego w środku wywołania metody, skoro obiekt tworzony przy konstrukcji
• Po co trzymać restTemplate na poziomie klasy, skoro się go przekazuje w wywołaniach metod
• Używanie ArrayListy z nieznanym docelowym rozmiarem
• Osobiście wolałbym widzieć stream, flatMap, map, collect w ostatniej metodzie, podobnie w przedostatniej metodzie

Tak de facto te metody nie mają sensu bo dla danego obiektu zawsze zwracają jedną, tą samą wartość (czyli to jest stała, a nie metoda):

    HttpEntity<String> createHttpEntityAndSetHeader(){
        HttpEntity<String> httpEntity = new HttpEntity<>("parameters", createHttpHeaderWithApiToken());
        return httpEntity;
    }

    HttpHeaders createHttpHeaderWithApiToken() {
        HttpHeaders headers = new HttpHeaders();
        headers.set("X-Auth-Token", apiToken);
        return headers;
    }

Ta metoda nazywa się createRestTemplate... a w rzeczywistości nic w niej nie tworzysz. Dodatkowo, nie ma sensu ze względu na typ zwracany. Czytając sygnaturę metody oczekiwałbym, że dostanę zupełnie nowy obiekt RestTemplate, a w rzeczywistości dostaje zmodyfikowany, już istniejący obiekt:

    RestTemplate createRestTemplateAndAddConverter() {
        this.restTemplate.getMessageConverters().add(createMappingJacksonConverter());
        return restTemplate;
    }

W tej metodzie również za każdym razem tworzysz dokładnie taki sam obiekt:

    MappingJackson2HttpMessageConverter createMappingJacksonConverter() {
        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setObjectMapper(new ObjectMapper());
        return converter;
    }

Nazwy metod getListOfMatchesExternalApi czy createListOfGameEntity są pozbawione sensu. Technikalia, które nie są potrzebne (List, ExternalApi, Entity) zaśmiecają wartościowe rzeczy. Lepszymi nazwami byłyby np. getMatches lub createGames

.foreach powinien służyć do wykonywania skutków ubocznych (jakieś logowanie, może zapis do danych chociaż to można zrobić lepiej). W twoim przypadku lepiej nadaje się mapa, która służy do przekształceń typu F<A> -> F<B>

Nie mogę edytować oryginalnego posta - dlaczego raz używane jest słowo game a raz match? (competition wtedy to też raczej zaciemnia obraz, zgaduję, że może chodzić o jakieś turnieje albo ligi, ale to wtedy są lepsze słowa do opisu tego).

class ApiFootballJsonConsumer {

    @Value("${api.apiAuthToken}")
    private String apiToken;

    List<Game> getGames(List<String> urls){
        HttpEntity<String> httpEntity = createHttpEntityWithHeader();
        RestTemplate restTemplate = createRestTemplateWithConverter();
        return createListOfGameEntity(getMatches(urls,httpEntity,restTemplate));
    }

    HttpEntity<String> createHttpEntityWithHeader(){
        HttpHeaders headers = new HttpHeaders();
        headers.set("X-Auth-Token",apiToken);
        return new HttpEntity<>("parameters",headers);
    }

    RestTemplate createRestTemplateWithConverter() {
        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setObjectMapper(new ObjectMapper());
        RestTemplate restTemplate = new RestTemplate();
        restTemplate.getMessageConverters().add(converter);
        return restTemplate;
    }

    List<MatchesExternalApi> getMatches(List<String> urls, HttpEntity<String> entity, RestTemplate restTemplate){
        return urls.stream()
                .map(url -> {
                    ResponseEntity<MatchesExternalApi> matchesExternalApiResponseEntity = restTemplate.exchange(
                            url,
                            HttpMethod.GET,
                            entity,
                            MatchesExternalApi.class);
                    return matchesExternalApiResponseEntity.getBody();
                })
                .collect(Collectors.toList());
    }

    List<Game> createListOfGameEntity(List<MatchesExternalApi> matchesExternalApi) {
        return matchesExternalApi.stream().flatMap(externalMatches ->
                externalMatches.getMatches().stream().map(p ->
                        (new Game(
                                p.getId(),
                                new Competition(externalMatches.getCompetition().getId(),
                                                externalMatches.getCompetition().getName()),
                                DateParser.getDateFromJson(p.getUtcDate()),
                                DateParser.getTimeFromJson(p.getUtcDate()),
                                p.getStatus(),
                                new Team(p.getHomeTeam().getId(), p.getHomeTeam().getName()),
                                new Team(p.getAwayTeam().getId(), p.getAwayTeam().getName()),
                                p.getScore().getWinner())))).
                collect(Collectors.toList());
    }
}

Game - nazwa mojej encji
MatchesExternalApi - nazwa obiektu z Jsona
Competition - nazwa Ligi w której odbywał się mecz(taka sama w Jsonia jak i u mnie)

W sumie tak jak teraz myślę, nie wiem czemu za wszelką cenę chciałem oddzielić Matches i Games, a nie chciałem oddzielić Competition, to bez sensu.......
Czy zachowanie nazw obiektów z JSONa jest ok ? Nie powinienem ich jakoś rozdzielać od siebie?

@Value na polu to zło - wartości i zależności wstrzykuj przez konstruktor, w ten sposób klasa jest nietestowalna (a raczej byłaby, gdyby pole było prywatne),

ApiFootballConsumer sugeruje, że to implementacja Consumer a nią nie jest (w sumie to nic nie konsumuje...).

createMatchesExternalApi() Nic nie tworzy tylko pobiera - podziel ją na utworzenie klienta HTTP i wywołanie. Osobiście lubię retrofit, ale co kto woli.

Konstruktor Game wygląda koszmarnie - kandydat do jakiejś metody fabrykującej