jv-rick-and-morty_by-Starikova-hw #145
Conversation
| @Operation(summary = "Get a list of characters ", description = " Get a list of all characters" | ||
| + " whose name contains the search string. " | ||
| + " Parameters: name = String for searching") | ||
| public List<PersonageDto> findByName(@RequestParam(name = "name") String name) { |
There was a problem hiding this comment.
| public List<PersonageDto> findByName(@RequestParam(name = "name") String name) { | |
| public List<PersonageDto> findByName(@RequestParam String name) { |
|
|
||
| @Mapper(config = MapperConfig.class) | ||
| public interface PersonageMapper { | ||
| @Mapping(source = "id", target = "id") |
There was a problem hiding this comment.
| @Mapping(source = "id", target = "id") |
| try { | ||
| HttpResponse<String> response = httpClient | ||
| .send(httprequest, HttpResponse.BodyHandlers.ofString()); | ||
| ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
Dont create Object evry time. Autowire it
| .stream() | ||
| .map(personageMapper::toModel) | ||
| .toList(); |
There was a problem hiding this comment.
Mapstruct supports mapping collections from the box
| } | ||
| return personageMapper | ||
| .toDto(personage.orElseThrow( | ||
| () -> new EntityNotFoundException("Not found any random personage") |
There was a problem hiding this comment.
It is only possible to have an Exception when there are no personages, right? So then change the ex message
| while (personage.isEmpty() && personageRepository.getMaxId() != 0) { | ||
| Long randomId = random.nextLong(personageRepository.getMaxId() + 1); | ||
| personage = personageRepository.findById(randomId); | ||
| } |
There was a problem hiding this comment.
Looks like a bad approach, so will SELECT max(EXTERNAL_ID) FROM Personages ever return 0?
Also it's doesn't look fine to put call to DB in while condition. Can't you just call personageRepository.count()? So you won't need this while block
There was a problem hiding this comment.
I put while loop here for case if the table has some deleted records and so the random number can point to not exist id.
There was a problem hiding this comment.
- How is it possible to maintain deleted records in the table in your implementation, you haven't introduced soft deletion in this task
- You also don't have any deletion logic, so I can't get such an approach.
| @Override | ||
| public PersonageDto getRandomPersonage() { |
There was a problem hiding this comment.
@transactional. You make more than one call to DB
| .findAllByNameContainsIgnoreCase(name) | ||
| .stream() | ||
| .map(personageMapper::toDto) | ||
| .toList(); |
There was a problem hiding this comment.
Also Mapstruct supports mapping collections from the box.
List toDtolist(List personages)
| .stream() | ||
| .map(personageMapper::toModel) | ||
| .toList(); |
| while (personage.isEmpty() && personageRepository.getMaxId() != 0) { | ||
| Long randomId = random.nextLong(personageRepository.getMaxId() + 1); | ||
| personage = personageRepository.findById(randomId); | ||
| } |
There was a problem hiding this comment.
- How is it possible to maintain deleted records in the table in your implementation, you haven't introduced soft deletion in this task
- You also don't have any deletion logic, so I can't get such an approach.
| public PersonageDto getRandomPersonage() { | ||
| Optional<Personage> personage = Optional.empty(); | ||
| if (personageRepository.count() > 0) { | ||
| while (personage.isEmpty()) { |
There was a problem hiding this comment.
| while (personage.isEmpty()) { |
| @Transactional | ||
| public PersonageDto getRandomPersonage() { | ||
| Optional<Personage> personage = Optional.empty(); | ||
| if (personageRepository.count() > 0) { |
There was a problem hiding this comment.
extract personageRepository.count() to a variable, you call .count() twice
|
|
||
| @Entity | ||
| @Data | ||
| @Table(name = "Personages") |
There was a problem hiding this comment.
| @Table(name = "Personages") | |
| @Table(name = "personages") |
|
|
||
| @Repository | ||
| public interface PersonageRepository extends JpaRepository<Personage, Long> { | ||
|
|
| import mate.academy.rickandmorty.dto.external.ResponsePersonageDto; | ||
|
|
||
| public interface PersonageApiClient { | ||
|
|
| @RequiredArgsConstructor | ||
| @Service | ||
| public class PersonageApiClientImpl implements PersonageApiClient { | ||
|
|
| import lombok.Data; | ||
|
|
||
| @Data | ||
| public class ResponseDto { |
There was a problem hiding this comment.
| public class ResponseDto { | |
| public record ResponseDto { |
|
|
||
| @Data | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class ResponseInfoDto { |
There was a problem hiding this comment.
| public class ResponseInfoDto { | |
| public record ResponseInfoDto { |
|
|
||
| @Data | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class ResponsePersonageDto { |
There was a problem hiding this comment.
| public class ResponsePersonageDto { | |
| public record ResponsePersonageDto { |
| import lombok.Data; | ||
|
|
||
| @Data | ||
| public class PersonageDto { |
There was a problem hiding this comment.
| public class PersonageDto { | |
| public record PersonageDto { |
| ResponseDto.class); | ||
| return responseDto; | ||
| } catch (IOException | InterruptedException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Don't forget to add message when you throw the exception
No description provided.