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

Created API #133

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

Created API #133

wants to merge 2 commits into from

Conversation

mhorianin
Copy link

No description provided.

@Tag(name = "Character management", description = "Endpoints for managing characters")
@RequiredArgsConstructor
@RestController
@RequestMapping("/character")

Choose a reason for hiding this comment

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

Better to use plural as endpoint represents that we have collection of characters

Suggested change
@RequestMapping("/character")
@RequestMapping("/characters")

Comment on lines 27 to 30
@GetMapping("/by-name")
@Operation(summary = "Get characters by name",
description = "Returns a list of all characters with the given name")
public List<CharacterDto> getCharactersByName(@RequestParam String name) {

Choose a reason for hiding this comment

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

Filtering usually continues getAll operation. So .../characters is getAll and .../characters?name=someName is a filtered option, no need in additional URL suffix like .../characters/by-name?name=someNam

Suggested change
@GetMapping("/by-name")
@Operation(summary = "Get characters by name",
description = "Returns a list of all characters with the given name")
public List<CharacterDto> getCharactersByName(@RequestParam String name) {
@GetMapping
@Operation(summary = "Get characters by name",
description = "Returns a list of all characters with the given name")
public List<CharacterDto> getCharactersByName(@RequestParam String name) {

OriginResponseDto origin,
OriginResponseDto location,
String image,
ArrayList<String> episode,

Choose a reason for hiding this comment

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

No need to use particular List implementation

Suggested change
ArrayList<String> episode,
List<String> episode,

import java.util.ArrayList;

public record ResultsResponseDataDto(
ArrayList<CharacterResponseDataDto> results,

Choose a reason for hiding this comment

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

Suggested change
ArrayList<CharacterResponseDataDto> results,
List<CharacterResponseDataDto> results,


@RequiredArgsConstructor
@Component
public class SaveCharacters {

Choose a reason for hiding this comment

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

Verbs shouldn't be used in class names

Suggested change
public class SaveCharacters {
public class CharactersInitializer {

private final RickAndMortyClient rickAndMortyClient;

@PostConstruct
public void saveCharacters() {

Choose a reason for hiding this comment

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

method doesn't represent full specter of actions it's doing

Suggested change
public void saveCharacters() {
public void fetchCharactersToDb() {

public void saveCharacters() {
List<CharacterResponseDataDto> characterResponseDataDto =
rickAndMortyClient.getCharacters();
characterService.saveAll(characterResponseDataDto);

Choose a reason for hiding this comment

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

I'd move logic from characterService.saveAll here.
It doesn't seems for me that work with characterResponseDataDto fits rest of methods characterService performs and may perform in future

@mhorianin mhorianin requested a review from olekskov March 21, 2024 10:32
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