Skip to content

Added working solution #44

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IvanFadieiev
Copy link

No description provided.

Copy link

@andrii-hoienko andrii-hoienko left a comment

Choose a reason for hiding this comment

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

GJ! But some refactoring required

.get("pages")
.toString(), Long.class);
for (int i = 1; i <= pages; i++) {
HttpRequest request = HttpRequest.newBuilder()

Choose a reason for hiding this comment

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

code duplicatioon

private final CharacterMapper characterMapper;
private final CharacterRepository characterRepository;

public void fetchDataFromApi() {

Choose a reason for hiding this comment

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

too much responsibilities for one method, I suggest to introduce method (maybe class) which will be responsible for making request (for example lets name it makeRequest(url) and another one which responsibility will be to call makeRequest and collect responses. Maybe makes sence to create method readInitialResponse and readCharacterResponse for parsing raw data to objects

return characterMapper.toDto(characterRepository
.findById(random.nextLong(characterRepository
.findAll().size())).orElseThrow(()
-> new RuntimeException("Entity was not found")));

Choose a reason for hiding this comment

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

It is bad practice to throw general exception, better to throw NotFoundException or something like this

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.

2 participants