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

result #204

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

result #204

wants to merge 3 commits into from

Conversation

ADIGrimm
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job! 🎉 Your implementation meets the task requirements and constraints effectively. The code is well-structured, and all necessary components are correctly configured. Just make sure to include Swagger documentation if it's required elsewhere in your project. Keep up the excellent work! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

public class AppConfig {

@Bean
public RestTemplate restTemplate() {

Choose a reason for hiding this comment

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

you actually don't need to create bean for RestTemplate by this way, so you can remove this config


@Component
public class CharacterMapper {
public CharacterDto toDto(Character character) {

Choose a reason for hiding this comment

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

don't need to implement mapping by yourself, use MapStruct

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
private String internalId;

Choose a reason for hiding this comment

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

Suggested change
private String internalId;
@Column(nullable = false, unique = true)
private String internalId;


@Override
public List<ApiDto> fetchAllCharacters() {
String url = "https://rickandmortyapi.com/api/character?page=";

Choose a reason for hiding this comment

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

it's better to use class-level variable for url
Also, you can use @Value and add url to app properties

import lombok.Data;

@Data
public class ApiDto {

Choose a reason for hiding this comment

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

For this task such Dto will be enough

Example,

public record CharacterResponseDto(
        Map<String, Object> info,
        List<CharacterExternalDto> results) {
}

public record CharacterExternalDto(
        Long id,
        String name,
        String status,
        String gender) {
}

public record CharacterDto(
        Long id,
        Long externalId,
        String name,
        String status,
        String gender
) {}

Copy link

@ahoienko ahoienko left a comment

Choose a reason for hiding this comment

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

Nice, left some comments

<artifactId>mysql-connector-j</artifactId>
<scope>runtime</scope>
</dependency>

Choose a reason for hiding this comment

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

Suggested change

<groupId>org.springframework</groupId>
<artifactId>spring-web</artifactId>
</dependency>

Choose a reason for hiding this comment

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

Suggested change

Character toEntity(CharacterExternalDto characterResponseDto);

default String generateInternalId(CharacterExternalDto dto) {
return dto.getId() != null ? String.valueOf(dto.getId()) : "default-id";

Choose a reason for hiding this comment

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

Suggested change
return dto.getId() != null ? String.valueOf(dto.getId()) : "default-id";
return dto.getId() != null ? String.valueOf(dto.getId()) : DEFAULT_ID;

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 24 to 25
public static final String URL = "https://rickandmortyapi.com/api/character?page=";
public static final Long CHARACTER_MAX = 826L;

Choose a reason for hiding this comment

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

Suggested change
public static final String URL = "https://rickandmortyapi.com/api/character?page=";
public static final Long CHARACTER_MAX = 826L;
private static final String URL = "https://rickandmortyapi.com/api/character?page=";
private static final Long CHARACTER_MAX = 826L;

Choose a reason for hiding this comment

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

Not resolved


@Override
public CharacterDto getRandomCharacter() {
Random random = new Random();

Choose a reason for hiding this comment

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

What is the reason to create this object each time method is called?

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 44 to 57
List<CharacterExternalDto> allCharacters = new ArrayList<>();
int page = 1;
CharacterResponseDto response;
do {
ResponseEntity<CharacterResponseDto> apiResponse = restTemplate
.getForEntity(URL + page, CharacterResponseDto.class);
response = apiResponse.getBody();
if (response != null && response.getResults() != null) {
allCharacters.addAll(response.getResults());
}
String nextPageUrl = (String) response.getInfo().get("next");
page++;
} while (response != null && response.getInfo().get("next") != null);
return allCharacters;

Choose a reason for hiding this comment

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

move string literals to constants

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 44 to 57
List<CharacterExternalDto> allCharacters = new ArrayList<>();
int page = 1;
CharacterResponseDto response;
do {
ResponseEntity<CharacterResponseDto> apiResponse = restTemplate
.getForEntity(URL + page, CharacterResponseDto.class);
response = apiResponse.getBody();
if (response != null && response.getResults() != null) {
allCharacters.addAll(response.getResults());
}
String nextPageUrl = (String) response.getInfo().get("next");
page++;
} while (response != null && response.getInfo().get("next") != null);
return allCharacters;

Choose a reason for hiding this comment

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

Add some line breaks

Choose a reason for hiding this comment

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

Not resolved

return allCharacters;
}

@PostConstruct

Choose a reason for hiding this comment

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

PostConstruct annotation is used to indicate 'init' method, the porpose of init method is to configure something in bean, code that you wrote here has nothing to deal with bean configuration. If you want to make some operation on program start please check ApplicationReadyEvent or use CommandLineRunner

Choose a reason for hiding this comment

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

Not resolved

@@ -0,0 +1,3 @@
databaseChangeLog:
- include:
file: db/changelog/changes/01-create-characters-table.yaml

Choose a reason for hiding this comment

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

Suggested change
file: db/changelog/changes/01-create-characters-table.yaml
file: db/changelog/changes/01-create-characters-table.yaml

Choose a reason for hiding this comment

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

Not resolved

added/removed lines in CharacterServiceImpl, db.changelog-master, pom
added constants in CharacterServiceImpl, CharacterMapper
Copy link

@ahoienko ahoienko left a comment

Choose a reason for hiding this comment

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

Please resolve comments before pushing new solution

Character toEntity(CharacterExternalDto characterResponseDto);

default String generateInternalId(CharacterExternalDto dto) {
return dto.getId() != null ? String.valueOf(dto.getId()) : "default-id";

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 24 to 25
public static final String URL = "https://rickandmortyapi.com/api/character?page=";
public static final Long CHARACTER_MAX = 826L;

Choose a reason for hiding this comment

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

Not resolved


@Override
public CharacterDto getRandomCharacter() {
Random random = new Random();

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 44 to 57
List<CharacterExternalDto> allCharacters = new ArrayList<>();
int page = 1;
CharacterResponseDto response;
do {
ResponseEntity<CharacterResponseDto> apiResponse = restTemplate
.getForEntity(URL + page, CharacterResponseDto.class);
response = apiResponse.getBody();
if (response != null && response.getResults() != null) {
allCharacters.addAll(response.getResults());
}
String nextPageUrl = (String) response.getInfo().get("next");
page++;
} while (response != null && response.getInfo().get("next") != null);
return allCharacters;

Choose a reason for hiding this comment

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

Not resolved

return allCharacters;
}

@PostConstruct

Choose a reason for hiding this comment

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

Not resolved

Comment on lines 44 to 57
List<CharacterExternalDto> allCharacters = new ArrayList<>();
int page = 1;
CharacterResponseDto response;
do {
ResponseEntity<CharacterResponseDto> apiResponse = restTemplate
.getForEntity(URL + page, CharacterResponseDto.class);
response = apiResponse.getBody();
if (response != null && response.getResults() != null) {
allCharacters.addAll(response.getResults());
}
String nextPageUrl = (String) response.getInfo().get("next");
page++;
} while (response != null && response.getInfo().get("next") != null);
return allCharacters;

Choose a reason for hiding this comment

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

Not resolved

@@ -0,0 +1,3 @@
databaseChangeLog:
- include:
file: db/changelog/changes/01-create-characters-table.yaml

Choose a reason for hiding this comment

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

Not resolved

@ADIGrimm
Copy link
Author

Sorry, I forgot to push

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.

4 participants