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

Did the task -Rick and Morty- #146

Closed
wants to merge 3 commits into from
Closed

Did the task -Rick and Morty- #146

wants to merge 3 commits into from

Conversation

aksoli666
Copy link

+created API with two methods:

  1. get randomly character
  2. get character by part of name
    +used public API

+created API with two methods:
1) get randomly character
2) get character by part of name
+used public API

@SpringBootApplication
@ComponentScan("mate.academy.rickandmorty.mapper")

Choose a reason for hiding this comment

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

Your spring application is already scanning this package, so this line is redundant

Suggested change
@ComponentScan("mate.academy.rickandmorty.mapper")

Copy link

Choose a reason for hiding this comment

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

not fixed, doesn't it work without it?

try {
HttpResponse<String> response = httpClient.send(httpRequest,
HttpResponse.BodyHandlers.ofString());
CharacterResponseDto dataDto = objectMapper

Choose a reason for hiding this comment

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

Suggested change
CharacterResponseDto dataDto = objectMapper
CharacterResponseDto characterDto = objectMapper

Comment on lines 7 to 10
@Setter
@Getter
@ToString
public class InfoResponseDto {

Choose a reason for hiding this comment

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

You can use @DaTa with Dtos instead of these annotations

@Getter
@ToString
public class InfoResponseDto {
private String nextUrl;

Choose a reason for hiding this comment

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

Have you checked the result? I don't think next deserializes into nextUrl

private final Random random;

@Override
public InternalCharacterDto getRandomlyCharacter() {

Choose a reason for hiding this comment

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

@transactional. You make more than one call to DB

@GetMapping("/random")
@Operation(summary = "Get randomly character", description = "Return a character by random id")
public InternalCharacterDto getRandomlyCharacter() {
return characterService.getRandomlyCharacter();

Choose a reason for hiding this comment

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

Suggested change
return characterService.getRandomlyCharacter();
return characterService.getRandomCharacter();

@GetMapping
@Operation(summary = "Filter by the given name",
description = "Returns a list of all characters whose name contains the search string")
public List<InternalCharacterDto> getCharacterByName(String name) {

Choose a reason for hiding this comment

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

Suggested change
public List<InternalCharacterDto> getCharacterByName(String name) {
public List<InternalCharacterDto> getCharactersByName(@RequestParam String name) {

private final ObjectMapper objectMapper;

@Override
public void getAndSaveCharacters() {

Choose a reason for hiding this comment

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

image
image
You need to tell Spring that you want to fetch all the data once, at the start of the application. Now your application never calls this method. So no data is fetched


@Override
public List<InternalCharacterDto> getCharacterByName(String name) {
return characterRepository.findAllByName(name)

Choose a reason for hiding this comment

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

Suggested change
return characterRepository.findAllByName(name)
return characterRepository.findAllByNameIn(name)

image
Let's say you have Rick in DB. If I call getCharacterByName("ck"), I want it to return Rick, Now your method will return Rick only if I call getCharacterByName("Rick")

}

@Override
public List<InternalCharacterDto> getCharacterByName(String name) {

Choose a reason for hiding this comment

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

Suggested change
public List<InternalCharacterDto> getCharacterByName(String name) {
public List<InternalCharacterDto> getCharactersByName(String name) {

+used @DaTa for DTOs
+getRandomCharacter -> @transactional now

@Repository
public interface CharacterRepository extends JpaRepository<Character, Long> {
List<Character> findAllByNameContaining(String partialName);

Choose a reason for hiding this comment

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

Suggested change
List<Character> findAllByNameContaining(String partialName);
List<Character> findAllByNameContainingIgnoreCase(String partialName);

Comment on lines +28 to +30
@Override
public void fetchAndSaveCharacters() {
HttpClient httpClient = HttpClient.newHttpClient();

Choose a reason for hiding this comment

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

renaming to fetch doesn't help Spring understand that that it should call this method at the beginning of the application bootstrapping. I hope you are familiar with bean's lifecycle, there's a method init(), look up what the annotation in Spring helps you put this method in init() method.

Comment on lines 35 to 37
.stream()
.map(characterMapper::toDto)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

List<InternalCharacterDto> toDtoList(List<Character> characters)

Comment on lines 54 to 56
Iterable<Character> characters = externalCharacterDtoList.stream()
.map(characterMapper::toModel)
.toList();

Choose a reason for hiding this comment

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

Mapstruct supports mapping collections from the box

}
} while (url != null);

Iterable<Character> characters = externalCharacterDtoList.stream()

Choose a reason for hiding this comment

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

Suggested change
Iterable<Character> characters = externalCharacterDtoList.stream()
List<Character> characters = externalCharacterDtoList.stream()

+method find all by name now ignore case
+fixed some issues
Comment on lines +34 to +35
characterRepository.findAllByNameContainingIgnoreCase(partialName));

Copy link

Choose a reason for hiding this comment

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

Suggested change
characterRepository.findAllByNameContainingIgnoreCase(partialName));
characterRepository.findAllByNameContainingIgnoreCase(partialName));


@SpringBootApplication
@ComponentScan("mate.academy.rickandmorty.mapper")
Copy link

Choose a reason for hiding this comment

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

not fixed, doesn't it work without it?

@aksoli666 aksoli666 closed this by deleting the head repository Jun 21, 2024
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