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

implement get random character #135

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

Conversation

hlibmurphy
Copy link

No description provided.

Copy link

@kozhukhovsky kozhukhovsky left a comment

Choose a reason for hiding this comment

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

We need to fix the build. And also, first, when you launch the application, you need to load all the characters into the database. And when accessing your controller, you should go to the database and pull out data, and not make a request to someone else’s API every time.

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!

public class ExternalCharacterResponseDto {
private InfoResponseDto info;
private List<CharacterResponseDto> results;

Copy link

Choose a reason for hiding this comment

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

Suggested change

@Override
public CharacterDto getRandomCharacterDto() {
long size = characterRepository.count();
Random random = new Random();
Copy link

Choose a reason for hiding this comment

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

better to not recreate random once per method call, but once per class/instance creation

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