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 simple app for working with products and its categories #319

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

Conversation

viprko
Copy link

@viprko viprko commented Aug 16, 2023

No description provided.

Copy link

@Gamemod13 Gamemod13 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, you can improve it if you want

@RequestParam BigDecimal priceTo) {
return productService.getAllByPriceBetween(priceFrom, priceTo).stream()
.map(dtoMapper::toDto)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

You can use short option

Suggested change
.collect(Collectors.toList());
.toList();

public List<ProductResponseDto> getAllByCategoryIds(@RequestParam Set<Long> categoryIds) {
List<Product> allByCategoryIds = productService.getAllByCategoryIds(categoryIds);
return allByCategoryIds.stream()
.map(dtoMapper::toDto).collect(Collectors.toList());

Choose a reason for hiding this comment

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

Suggested change
.map(dtoMapper::toDto).collect(Collectors.toList());
.map(dtoMapper::toDto).toList();

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you are right. But I'm not using toList() in homework cos this method appears in Java 16 but most of our homework uses Java 11. I missed that this task is using Java 17. Also, I had some problems before with List immutability after using the "toList()" method, so...


@Data
public class CategoryResponseDto {
private Long id;

Choose a reason for hiding this comment

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

Do you need send id in response?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I want to return the ID of the saved element


@Data
public class ProductResponseDto {
private Long id;

Choose a reason for hiding this comment

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

The same, do you realy need send id in Response?

Comment on lines +13 to +14
@Getter
@Setter

Choose a reason for hiding this comment

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

You can use one annotation Data

Suggested change
@Getter
@Setter
@Data

Copy link
Author

Choose a reason for hiding this comment

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

I read some topics, that were specified in the common mistakes file, about avoiding @DaTa annotation in Entity classes. I do not agree with all suggestions, however, I do find myself aligning with a majority of them, that's why I don't use @DaTa annotation here

Copy link

@sarakhmen sarakhmen left a comment

Choose a reason for hiding this comment

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

That's great you're using technologies studied during the group project :)) Good job!

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