Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add repository, service, mapper, controller & third-party API client … #58

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

VoitseshukV
Copy link

…implementation

Comment on lines 21 to 30
@Component
@RequiredArgsConstructor
public class DataLoader implements ApplicationRunner {
private final CartoonCharacterClient characterClient;
private final CartoonCharacterService characterService;

public void run(ApplicationArguments args) {
List<CreateCartoonCharacterRequestDto> characters = characterClient.getAllCharacters();
characterService.saveAll(characters);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use Component for this and create inner class?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to get service layer beans. Can't get them directly in a static main method. The option with initialization in ApplicationRunner was considered in one of the lessons. What other options are there?

Copy link
Author

@VoitseshukV VoitseshukV Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new implementation because of its compactness, but I am not sure that it is correct. I added only a fully encapsulated ThirdPartyApiDataLoader class bean to the Application. After its instance is created, its PostConstruct method is called, which performs data loading.

Comment on lines 25 to 26
public CartoonCharacterDto getByName() {
return cartoonCharacterService.getRandom();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct naming of method?

Comment on lines 27 to 52
try {
Long pagesCount = PAGES_COUNT;
List<CreateCartoonCharacterRequestDto> result = null;
for (int i = 0; i < pagesCount; i++) {
HttpRequest httpRequest = HttpRequest.newBuilder()
.GET()
.uri(URI.create(BASE_URL.formatted(i)))
.build();
HttpResponse<String> response = httpClient.send(httpRequest,
HttpResponse.BodyHandlers.ofString());
CartoonCharactersListResponseDto cartoonCharactersListResponseDto =
objectMapper.readValue(response.body(),
CartoonCharactersListResponseDto.class);
pagesCount = cartoonCharactersListResponseDto.info().pages();
if (result == null) {
result = cartoonCharactersListResponseDto.results();
} else {
result.addAll(cartoonCharactersListResponseDto.results());
}
}
if (result == null || result.isEmpty()) {
throw new GettingCharactersListException("Error getting characters list");
}
return result;
} catch (IOException | InterruptedException e) {
throw new GettingCharactersListException("Error getting characters list", e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you divide logic for one helpful method?

Comment on lines 36 to 41
if (characters.isEmpty()) {
throw new EntityNotFoundException("Can't find characters with name: " + name);
}
return characters.stream()
.map(mapper::toDto)
.toList();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic better place to service layer

Copy link

@olekskov olekskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants