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 with 2 methods #182

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

Created API with 2 methods #182

wants to merge 5 commits into from

Conversation

Trayvor
Copy link

@Trayvor Trayvor commented Aug 11, 2024

No description provided.


@Data
public class CharacterDto {
@NotBlank

Choose a reason for hiding this comment

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

Suggested change
@NotBlank
@NotNull

public class CharacterDto {
@NotBlank
private Long id;
@NotBlank

Choose a reason for hiding this comment

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

same

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
@Column(nullable = false, name = "external_id")

Choose a reason for hiding this comment

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

Suggested change
@Column(nullable = false, name = "external_id")
@Column(nullable = false, unique = true)


@Override
public String getKey() {
return "name";

Choose a reason for hiding this comment

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

should be constant

Random random = new Random();
Long randomId = random.nextLong(characterRepository.count() - MIN_ID + 1) + MIN_ID;
Character character = characterRepository.findById(randomId).orElseThrow(() ->
new CharacterNotFoundException("Can`t find random character in database"));

Choose a reason for hiding this comment

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

Suggested change
new CharacterNotFoundException("Can`t find random character in database"));
new EntityNotFoundException("Can`t find random character in database"));

characterDtoList.addAll(characterResponseDto.getResults());
nextPageUrl = characterResponseDto.getInfo().getNext();
} catch (URISyntaxException | IOException | InterruptedException e) {
throw new FetchingDataException(e, "Can`t fetch data about characters. URL: "

Choose a reason for hiding this comment

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

Suggested change
throw new FetchingDataException(e, "Can`t fetch data about characters. URL: "
throw new CharacterExternalLoadException("Can`t fetch data about characters. URL: " + URL, e);

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!

name: status
type: varchar(256)
constraints:
nullable: false
Copy link

Choose a reason for hiding this comment

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

add empty line at EOF


@Override
public CharacterDto getRandomCharacter() {
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.

create one random per class instance, not per each method invocation

@Override
public CharacterDto getRandomCharacter() {
Random random = new Random();
Long randomId = random.nextLong(characterRepository.count() - MIN_ID + 1) + MIN_ID;
Copy link

Choose a reason for hiding this comment

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

Suggested change
Long randomId = random.nextLong(characterRepository.count() - MIN_ID + 1) + MIN_ID;
Long randomId = random.nextLong(characterRepository.count() - 1);

sorry i don't get it

Comment on lines 62 to 65
return characterRepository.findAll(characterSpecification)
.stream()
.map(characterMapper::toDto)
.toList();
Copy link

Choose a reason for hiding this comment

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

btw you can declare mapping of list to list in mapstruct's interface

Comment on lines 29 to 31
List<Character> characters = externalCharacterList.stream()
.map(characterMapper::toModel)
.toList();
Copy link

Choose a reason for hiding this comment

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

same, try to do it in interface

@Service
@RequiredArgsConstructor
public class CharacterServiceImpl implements CharacterService {
public static final int MIN_ID = 1;
Copy link

Choose a reason for hiding this comment

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

not needed, but why public?


@Component
public class NameSpecificationProvider implements SpecificationProvider<Character> {
public static final String SQL_WILDCARD = "%";
Copy link

Choose a reason for hiding this comment

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

this is not going to change in decades, okay to hardcode :)
or if you want a constant, create a string format with placeholder then

public Specification<Character> getSpecification(String param) {
String pattern = SQL_WILDCARD + param + SQL_WILDCARD;
return (root, query, criteriaBuilder) ->
criteriaBuilder.like(criteriaBuilder.lower(root.get("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
criteriaBuilder.like(criteriaBuilder.lower(root.get("name")),
criteriaBuilder.like(criteriaBuilder.lower(root.get(NAME_KEY)),

@Trayvor Trayvor requested a review from okuzan August 20, 2024 18:47
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.

Well done!

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