-
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
added integration with rick-and-morty API #4
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
pom.xml
Outdated
<groupId>org.springframework.boot</groupId> | ||
<artifactId>spring-boot-devtools</artifactId> | ||
<scope>runtime</scope> | ||
<optional>true</optional> |
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.
why do you need this?
public record CharacterInfoDto( | ||
@JsonProperty("id") Long externalId, String name, String status, String gender |
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.
weird formatting:
public record CharacterInfoDto( | |
@JsonProperty("id") Long externalId, String name, String status, String gender | |
public record CharacterInfoDto( | |
@JsonProperty("id") | |
Long externalId, | |
String name, | |
String status, | |
String gender |
|
||
import java.util.List; | ||
|
||
public record 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.
a general recommendation on record formatting
if your record has 1-2 fields, you can:
public record SomeRecord(String field1, Integer field2) {
if your record has more than 2 fields:
public record SomeRecord(String field1,
int field2,
Double field3,
boolean field4) {
or alrternative:
public record SomeRecord(
String field1,
int field2,
Double field3,
boolean field4) {
@Column(nullable = false) | ||
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.
should this be unique?
|
||
@Service | ||
public class CharactersClientService { | ||
private static final String BASE_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 can be stored in your app properties, and then be injected here via @Value
@Service | ||
public class CharactersClientService { | ||
private static final String BASE_URL = "https://rickandmortyapi.com/api/character"; | ||
private final HttpClient httpClient = HttpClient.newHttpClient(); |
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.
consider defining it as a bean
also you may find using RestTemplate
easier
public class CharactersClientService { | ||
private static final String BASE_URL = "https://rickandmortyapi.com/api/character"; | ||
private final HttpClient httpClient = HttpClient.newHttpClient(); | ||
private final ObjectMapper objectMapper = new ObjectMapper(); |
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.
web starter already brings autoconfiguration, that has a bean of ObjectMapper
- no need to create additional one
import org.springframework.stereotype.Service; | ||
|
||
@Service | ||
public class CharactersClientService { |
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.
may be simplified to CharactersClient
} | ||
return characters; | ||
} catch (URISyntaxException | IOException | InterruptedException e) { | ||
throw new RuntimeException("Retrieving characters failed", e); |
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.
consider creating your custom exception
|
||
@Override | ||
public CharacterDto getRandomCharacter() { | ||
List<Character> characters = characterRepository.findAll(); |
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 is a bit incorrect
you need to get a random character, however you load all the characters from the DB
think of how you can load only 1 random character via repository :)
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 don't have an idea how to do this in one SQL query, so I thought of using a random page with a size=1
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.
LGTM
|
||
@Override | ||
public List<CharacterDto> searchCharacters( | ||
String name, @PageableDefault(size = 5, sort = "id") 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.
there is no need to use @PageableDefault
here
No description provided.