init commit #151
init commit #151
Conversation
|
|
||
| } | ||
| } | ||
|
|
|
|
||
| while (url != null) { | ||
| String jsonResponse = restTemplate.getForObject(url, String.class); | ||
| JsonNode rootNode = objectMapper.readTree(jsonResponse); |
There was a problem hiding this comment.
What is the reason to readTree? Is it possible to map to an object?
There was a problem hiding this comment.
I choose this aproach because its clear for me to create object from which then I'll fetch info by key, and then manipulate with it. Then I get by key="results" and map to Character.class
| private final CharacterRepository characterRepository; | ||
|
|
||
| @PostConstruct | ||
| public void fetchData() throws JsonProcessingException { |
There was a problem hiding this comment.
Bad method name, I believe fetch data method should return something
| private final CharacterRepository characterRepository; | ||
|
|
||
| @PostConstruct | ||
| public void fetchData() throws JsonProcessingException { |
There was a problem hiding this comment.
Please catch JsonProcessingException and rethrow it as runtime
| private final ObjectMapper objectMapper; | ||
| private final CharacterRepository characterRepository; | ||
|
|
||
| @PostConstruct |
There was a problem hiding this comment.
PostConstruct often used for bean configuration, the code that you wrote in not related to bean configuration, so I suggest to use different approach for this, you can use CommandLineRunner interface(or bean) or use event listeners to run code on startup
|
|
||
| @Override | ||
| public Character generateRandomCharacter() { | ||
| return Optional.of(characterRepository.findById(new Random().nextLong(CHARACTERS_AMOUNT))) |
There was a problem hiding this comment.
Do you really want to create Random object each time this method is called
| @Service | ||
| @RequiredArgsConstructor | ||
| public class CharacterServiceImpl implements CharacterService { | ||
| private static final int CHARACTERS_AMOUNT = 826; |
There was a problem hiding this comment.
What if some characters will be added or removed?
| import java.io.IOException; | ||
| import mate.academy.rickandmorty.model.Character; | ||
|
|
||
| public class CharacterDeserializer extends JsonDeserializer<Character> { |
There was a problem hiding this comment.
I think it is redundant, object mapper could deserialize your objects without additional configuration
There was a problem hiding this comment.
It does, but it maps id from Json to Entity.id. I thought JsonProperty annotation will solve this problem, but had mapping exception with msg like (I want to set json.id to entity.id, but your annotation said that i should set json.id to other field)
There was a problem hiding this comment.
That is why you need to create an externalDto to map response from API and then remap it to entity and save it, such approach will be more flexible because we have a separate class to represent characters in external API. In general better to avoid custom deserialization logic, because it is often confusing. Flow when we map external response as is and then work with it as we want is preferable, I could describe in more details on QnA
| spring.datasource.username=root | ||
| spring.datasource.password=root |
| @RequiredArgsConstructor | ||
| public class DataFetcher { | ||
|
|
||
| private static final String CHARACTER_URL = "https://rickandmortyapi.com/api/character"; |
There was a problem hiding this comment.
I would prefer to make this url configurable via propertioes
| return Optional.of(characterRepository.findById(id)) | ||
| .get() | ||
| .orElseThrow(() -> new IllegalStateException("Character not found with id: " + id)); | ||
| } |
There was a problem hiding this comment.
as far as I remember characterRepository.findById(id) returns optional. So you make Optional.of(Optional returned from repository).get (//Unboxing optional that you create) .orElseThrow (//Working with optional returned from repo)
| import java.io.IOException; | ||
| import mate.academy.rickandmorty.model.Character; | ||
|
|
||
| public class CharacterDeserializer extends JsonDeserializer<Character> { |
There was a problem hiding this comment.
That is why you need to create an externalDto to map response from API and then remap it to entity and save it, such approach will be more flexible because we have a separate class to represent characters in external API. In general better to avoid custom deserialization logic, because it is often confusing. Flow when we map external response as is and then work with it as we want is preferable, I could describe in more details on QnA
| try { | ||
| JsonNode rootNode = objectMapper.readTree(jsonResponse); | ||
| List<Character> pageCharacters = deserializeCharacters(rootNode); | ||
| characterRepository.saveAll(pageCharacters); |
There was a problem hiding this comment.
I suggest to fetch all characters first and than save them all to not make request to db several times
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class DataFetcher { |
There was a problem hiding this comment.
| public class DataFetcher { | |
| public class DataFetchService{ |
| @Value("${characters.url}") | ||
| private String characterUrl; | ||
|
|
||
| public void populateCharactersDB() { |
There was a problem hiding this comment.
| public void populateCharactersDB() { | |
| public List<Characters> fetch() { |
| @Component | ||
| @RequiredArgsConstructor | ||
| public class CustomApplicationRunner implements ApplicationRunner { | ||
| private final DataFetcher dataFetcher; | ||
|
|
||
| @Override | ||
| public void run(ApplicationArguments args) { | ||
| dataFetcher.populateCharactersDB(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| @Component | |
| @RequiredArgsConstructor | |
| public class CustomApplicationRunner implements ApplicationRunner { | |
| private final DataFetcher dataFetcher; | |
| @Override | |
| public void run(ApplicationArguments args) { | |
| dataFetcher.populateCharactersDB(); | |
| } | |
| } | |
| @Component | |
| @RequiredArgsConstructor | |
| public class DataPopulator implements ApplicationRunner {//Not best name, you can think about better one | |
| private final DataFetcher dataFetcher; | |
| private final Repo repo | |
| @Override | |
| public void run(ApplicationArguments args) { | |
| characters = dataFetcher.populateCharactersDB(); | |
| repo.saveAll(characters) | |
| } | |
| } |
No description provided.