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

First solution of jv-rick-and-morty #118

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

QbaSekowski
Copy link

No description provided.

Copy link

@nowickimj nowickimj left a comment

Choose a reason for hiding this comment

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

Your code doesn't compile successfully - fix it before requesting a review.

Copy link

@okuzan okuzan left a comment

Choose a reason for hiding this comment

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

Good job so far! Make sure to retrieve all the characters, keeping pagination in mind

Comment on lines 28 to 33
HttpResponse<String> response = httpClient.send(
request, HttpResponse.BodyHandlers.ofString());

return objectMapper.readValue(
response.body(), CharacterResponseDataDto.class);

Copy link

Choose a reason for hiding this comment

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

are you sure you receive all 826 characters with this implementation? Remember about pagination. You need some kind of a loop to check if there's a next page
image

long randomId = random.nextLong(characterRepository.count());
Character character = characterRepository.findById(randomId)
.orElseThrow(() ->
new RuntimeException("Cannot find a character by id: " + randomId));
Copy link

Choose a reason for hiding this comment

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

it's better to throw more specific exceptions where possible, let's go for EntityNotFound or NoSuchElement exception here

Comment on lines 11 to 12
@Query(value = "SELECT c FROM Character c WHERE c.name LIKE CONCAT('%', :name, '%')")
List<Character> findAllByNameContainingIgnoreCase(String name);
Copy link

Choose a reason for hiding this comment

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

Consider removing the @query annotation for this method as Spring Data JPA's query derivation from method names can automatically generate the required query

Copy link

@nowickimj nowickimj left a comment

Choose a reason for hiding this comment

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

Few minor comments, good work with the rest :)

@@ -5,7 +5,6 @@

@SpringBootApplication
public class Application {

Choose a reason for hiding this comment

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

Avoid unnecessary changes in your PRs.

import lombok.Data;

@Data
public class CharacterExternalResponseDto {

Choose a reason for hiding this comment

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

You might use record instead of regular class for DTOs. Additionally, you won't require Lombok - @Data produces lots of boilerplate code.

import lombok.Data;

@Data
public class CharacterResponseDataDto {

Choose a reason for hiding this comment

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

Same here, you can use record.

void contextLoads() {
}

@Test

Choose a reason for hiding this comment

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

Unnecessary change.

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