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

solution #189

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

solution #189

wants to merge 3 commits into from

Conversation

GlebPashko
Copy link

No description provided.

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 overall!

return new Random().nextLong(characterRepository.count());
}
}

Copy link

Choose a reason for hiding this comment

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

Suggested change

}

private long getRandomId() {
return new Random().nextLong(characterRepository.count());
Copy link

Choose a reason for hiding this comment

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

make random a private final class field to avoid needless recreation

private final CharacterService characterService;

@Operation(summary = "Get one random character")
@GetMapping("/randomly")
Copy link

Choose a reason for hiding this comment

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

Suggested change
@GetMapping("/randomly")
@GetMapping("/random")

makes more sense for RESTful naming

Comment on lines 27 to 28
@GetMapping("/search")
public List<CharacterEntity> searchByName(String name) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
@GetMapping("/search")
public List<CharacterEntity> searchByName(String name) {
@GetMapping
public List<CharacterEntity> searchByName(String name) {

make name a request parameter

@GlebPashko GlebPashko requested a review from okuzan September 16, 2024 05:33
Copy link

@liliia-ponomarenko liliia-ponomarenko 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! 🔥 A few recommendations, please correct them and take them into account in the next solutions 😉

<configuration>
<configLocation>${maven.checkstyle.plugin.configLocation}</configLocation>
<consoleOutput>true</consoleOutput>
<sourceDirectories>src/main/java/mate/academy/rickandmorty/mapper/CharacterMapper.java</sourceDirectories>

Choose a reason for hiding this comment

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

Why do you apply checkstyle plugin only to one class? Let's apply it to the src directory;)


@SpringBootApplication
public class Application {

private final CharacterService characterService;

@Autowired

Choose a reason for hiding this comment

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

JFYI this annotation is optional ;)

@Getter
@Setter
@Entity
public class CharacterEntity {

Choose a reason for hiding this comment

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

Suggested change
public class CharacterEntity {
public class Character {

url = dataDto.getInfo().getNext();
}
} catch (IOException | InterruptedException e) {
throw new RuntimeException(e);

Choose a reason for hiding this comment

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

Let's add an informative message ;)

spring.jpa.hibernate.ddl-auto=create-drop
spring.jpa.show-sql=true
spring.jpa.open-in-view=false

Choose a reason for hiding this comment

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

Suggested change

Leave only one empty line at the end of the file ;)

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