-
Notifications
You must be signed in to change notification settings - Fork 343
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
Hw jv2 #320
base: main
Are you sure you want to change the base?
Hw jv2 #320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, keep it up :)
return categoryRepository.findById(id).orElseThrow(() | ||
-> new NoSuchElementException("Can`t find ")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return categoryRepository.findById(id).orElseThrow(() | |
-> new NoSuchElementException("Can`t find ")); | |
} | |
return categoryRepository.findById(id).orElseThrow(() | |
-> new NoSuchElementException("Can`t find category by id: " + id)); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job! Left just a few minor suggestion
import lombok.Data; | ||
import lombok.EqualsAndHashCode; | ||
|
||
@Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try to not use Data annotaion for entities. Better to use Getter Setter EqualsAndHashCode etc.
please check other entities.
List<Product> findAllByPriceBetween(BigDecimal from, BigDecimal to); | ||
|
||
List<Product> findAllByCategoryNameIn(List<String> categoryName); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package mate.academy.springboot.datajpa.service; | ||
|
||
public interface GeneralService<T> { | ||
T create(T model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename this method for better matching what this method do and better code readability
T create(T model); | |
T save(T model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job! Left just a few minor suggestions.
public class CategoryServiceImpl implements CategoryService { | ||
private final CategoryRepository categoryRepository; | ||
|
||
public CategoryServiceImpl(CategoryRepository categoryRepository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the lombok annotations for creating a needed constructor and simplify the code.
public class ProductServiceImpl implements ProductService { | ||
private final ProductRepository productRepository; | ||
|
||
public ProductServiceImpl(ProductRepository productRepository) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the lombok annotations for creating a needed constructor and simplify the code.
} | ||
|
||
@Override | ||
public List<Product> findAllByCategoryNameIn(List<String> categoryName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public List<Product> findAllByCategoryNameIn(List<String> categoryName) { | |
public List<Product> findAllByCategoryNameIn(List<String> categoryNames) { |
|
||
@GetMapping("/by-categories") | ||
public List<ProductResponseDto> getAllByCategories(@RequestParam String[] categoriesId) { | ||
return productService.findAllByCategoryNameIn(List.of(categoriesId)).stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming inconsistency. It is not clear from the naming of variables the method should receive category names or category ids?
} | ||
|
||
@GetMapping("/by-categories") | ||
public List<ProductResponseDto> getAllByCategories(@RequestParam String[] categoriesId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use plural in the end of the variable names instead of in the middle, e.g. categoryIds
or categoryNames
.
Please fix the similar cases in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.