-
Notifications
You must be signed in to change notification settings - Fork 250
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
Implemented task #49
Implemented task #49
Conversation
|
||
@Tag(name = "Rick and Morty API") | ||
@RestController | ||
@RequestMapping("/api") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd expand path in order to be able to add other endpoints later
@RequestMapping("/api") | |
@RequestMapping("/api/characters") | |
long count(); | ||
|
||
Character save(Character character); | ||
|
||
List<Character> findAll(); | ||
|
||
Optional<Character> findById(Long id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these methods are present by default in JpaRepository, no need to duplicate
long count(); | |
Character save(Character character); | |
List<Character> findAll(); | |
Optional<Character> findById(Long id); |
@PostConstruct | ||
private void init() { | ||
List<Character> characters = client.getCharacters(); | ||
repository.saveAll(characters); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move method to CharacterClientImpl
This service mainly works to process our users' requests and initialization is logically out of this scope
@PostConstruct | |
private void init() { | |
List<Character> characters = client.getCharacters(); | |
repository.saveAll(characters); | |
} |
character.setInternalId(null); | ||
return repository.save(character); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot understand why we reset id and call save for character we just retrieved from DB
public List<Character> getAll() { | ||
return repository.findAll(); | ||
} | ||
|
||
@Override | ||
public List<CharacterDto> getCharactersByName(String name) { | ||
List<Character> characters = repository.findByNameContaining(name); | ||
List<CharacterDto> characterDtos = mapper.map(characters); | ||
return characterDtos; | ||
} | ||
|
||
@Override | ||
public Character saveRandomCharacter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should unify if you service returns DTOs or models
public List<Character> getCharacters() { | ||
return characterService.getAll(); | ||
} | ||
|
||
@Operation(summary = "Find characters by specific name.") | ||
@GetMapping("/search") | ||
public List<CharacterDto> getCharactersByName(String name) { | ||
return characterService.getCharactersByName(name); | ||
} | ||
|
||
@Operation(summary = "Generate and save random existing character.") | ||
@GetMapping("/random") | ||
public Character saveRandomCharacter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use dtos and hide internal implementation of classes
@Operation(summary = "Get all available characters.") | ||
@GetMapping | ||
public List<CharacterDto> getCharacters() { | ||
return characterService.getAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.