-
Notifications
You must be signed in to change notification settings - Fork 240
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 rick and morty hw #30
base: main
Are you sure you want to change the base?
Conversation
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.
Good job! Let's improve your solution!
public interface CharacterMapper { | ||
CharacterDto toDto(Character character); | ||
|
||
Character toEntity(CharacterResponseDto characterResponseDto); |
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.
Character toEntity(CharacterResponseDto characterResponseDto); | |
Character toCharacter(CharacterResponseDto characterResponseDto); |
@Table(name = "characters") | ||
@Data | ||
public class Character { | ||
@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.
@GeneratedValue(strategy = GenerationType.IDENTITY)
public class Character { | ||
@Id | ||
private long id; | ||
private Long externalId; |
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.
Could externalId, name and status be null?
import mate.academy.rickandmorty.model.Character; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
|
||
public interface CharacterRepository extends JpaRepository<Character, Long> { |
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.
Annotation Repository is missing
|
||
@Override | ||
public CharacterDto getRandomCharacter() { | ||
Long id = RANDOM.nextLong(characterRepository.count()); |
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 could use query here to find random character
@Component | ||
@RequiredArgsConstructor | ||
public class CharacterClient { | ||
private static final String URL = "https://rickandmortyapi.com/api/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.
this property should be stored in application properties and referenced via @value
.map(characterMapper::toEntity) | ||
.toList(); | ||
list.forEach(c -> c.setExternalId(c.getId())); | ||
characterRepository.saveAll(list); |
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 could define a RestTemplate bean and then use its method getForObject here
this way your code will be less verbose and more readable
characterRepository.saveAll(list); | ||
} | ||
} catch (Exception e) { | ||
throw new DataProcessingException("Can't load 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.
I didn't find GlobalExceptionHandler where you handle this exception
public interface CharacterService { | ||
CharacterDto getRandomCharacter(); | ||
|
||
List<CharacterDto> getCharacterByNameContaining(String name); |
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.
List<CharacterDto> getCharacterByNameContaining(String name); | |
List<CharacterDto> getCharacterByName(String name); |
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.
Good job! Let's make your solution better!
description = "Endpoints for receiving Rick and Morty characters from DB") | ||
@RequiredArgsConstructor | ||
@RestController | ||
@RequestMapping(value = "api/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.
@RequestMapping(value = "api/characters") | |
@RequestMapping("/characters") |
api
is a prefix of the module (you don’t need to specify each of your controllers with it)
description = "Returns a list of all characters " | ||
+ "whose name contains the search string") | ||
@GetMapping(value = "/search") | ||
public List<CharacterDto> searchCharactersByName(@RequestParam String name) { |
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.
It is better to add @ParameterObject @PageableDefault Pageable pageable
|
||
@Repository | ||
public interface CharacterRepository extends JpaRepository<Character, Long> { | ||
List<Character> findCharacterByNameContaining(String name); |
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.
List<Character> findCharacterByNameContaining(String name); | |
List<Character> findCharacterByNameContaining(String name, Pageable pageable); |
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.
Great 😊
No description provided.