-
Notifications
You must be signed in to change notification settings - Fork 5
[review] f1-washpedia #144
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
base: refactor/review
Are you sure you want to change the base?
Changes from 11 commits
ddd61fb
c2a17d0
f1d0672
07cb76d
c4e1569
19a24d0
c90f43c
82db412
e0fcb63
1fc0f4e
060196a
5af31d2
44918ee
4d7d388
789b0cb
927b0e7
3afa661
dd9617a
e90ba9f
c4daa01
d3cd247
a4773a2
71676fa
242c2f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,12 @@ | |
| public class CommonCodeController { | ||
|
|
||
| private final CommonCodeService commonCodeService; | ||
|
|
||
| // | ||
| @GetMapping("/{codeName}") | ||
| public ResponseEntity<ApiResponse<List<CommonCodeDto>>> getCommonCode (@PathVariable String codeName){ | ||
|
|
||
| // | ||
| List<CommonCodeDto> codes = commonCodeService.getCodes(codeName); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 반환타입을 저장하는 변수를 만드는것도 좋지만 사용처가 없다면 바로 return 문에 넣어주는것은 어떨까요 ?return |
||
|
|
||
| return ApiResponse.toResponseEntity(GET_COMMON_CODE_SUCCESS, codes); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,14 @@ public class CommonCodeService { | |
|
|
||
| public List<CommonCodeDto> getCodes(String codeName) { | ||
|
|
||
| // | ||
| return commonCodeRepository.findAllByUpperNameAndIsUsed(codeName,true) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 항상 true만 반환하는데 spring data jpa 에서 제공하는 true를 그냥 써도 될것같습니다! 참고 사이트 전달드립니다 |
||
| .stream() | ||
| .map(CommonCodeDto::from) | ||
| .toList(); | ||
| } | ||
|
|
||
| // | ||
| public List<CommonCodeDto> getCodesMapper(String codeName) { | ||
|
|
||
| return commonCodeMapper.findAllByUpperNameAndIsUsed(codeName,true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons | |
| boolean result = true; | ||
| String requestToken = request.getHeader("Authorization"); | ||
|
|
||
| // | ||
| if (requestToken == null || requestToken.isEmpty()) { throw new BusinessException(AcceptInterceptorErrorCode.DOSE_NOT_EXIST_REQUEST_TOKEN); } | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Objects.isNull 을 활용하여 null에 대해 명시적으로 관리하면 좋을것 같습니다! 또한 2가지 조건에대해서도 처리 가능합니다! |
||
|
|
||
| if (!validRequestToken(requestToken)) { throw new BusinessException(AcceptInterceptorErrorCode.FAILED_VALID_REQUEST_TOKEN); } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,22 @@ | |
| @RestController | ||
| @RequestMapping | ||
| @RequiredArgsConstructor | ||
|
|
||
| // | ||
| public class MainContoller { | ||
chan99k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| private final ProductService productService; | ||
| private final MainService mainService; | ||
|
|
||
| // | ||
| @GetMapping("/banner") | ||
| ResponseEntity<ApiResponse<BannerDto>> getBanner() { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API는 복수형으로 작성하시는걸 추천드립니다 |
||
|
|
||
| return ApiResponse.toResponseEntity(BannerBusinessCode.GET_BANNER_DATA_SUCCESS, mainService.getBanner()); | ||
| } | ||
|
|
||
|
|
||
| @GetMapping("/recommend-products") | ||
| // | ||
| ResponseEntity<ApiResponse<Page<RecommendProductsDto>>> getRecommendProducts(Pageable pageable) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pageable를 바로사용해도 좋지만 클래스로 만들어서 Custom하게 사용할 수 있도록 해보시는것도 좋아보입니다! |
||
| Page<RecommendProductsDto> recommendProductList = productService.getRecommendProductList(pageable); | ||
|
|
||
|
|
@@ -37,6 +42,8 @@ ResponseEntity<ApiResponse<Page<RecommendProductsDto>>> getRecommendProducts(Pag | |
| } | ||
|
|
||
| @GetMapping("/products/rank") | ||
|
|
||
| // | ||
| ResponseEntity<ApiResponse<Page<ProductDto>>> getProducts( | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Controller에서 반환하는 Response타입명을 DTO를 제거하고 Response를 붙여 보시는건 어떠세요? |
||
| @RequestParam(name = "sortType", defaultValue = "viewCnt-order") Sort sortType, Pageable pageable) { | ||
| Page<ProductDto> productDtos = sortType.sort(productService, pageable); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import org.springframework.data.domain.Pageable; | ||
|
|
||
|
|
||
| // | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort가 Enum인데 패키지의 위치가 controller에 있습니다. 패키지의 위치 조절이 필요해 보입니다 . |
||
| public enum Sort { | ||
|
|
||
| VIEW_COUNT_PRODUCT_ORDER("viewCnt-order") { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,13 @@ | |
| public interface ProductRepository extends JpaRepository<Product, Long> { | ||
| Page<Product> findByProductNameContaining(String keyword, Pageable pageable); | ||
|
|
||
| // | ||
| Page<Product> findAllByOrderByViewCountDesc(Pageable pageable); | ||
|
|
||
| Page<Product> findTop5ByOrderByProductNameDesc(Pageable pageable); | ||
|
|
||
| Page<Product> findAllBySafetyStatusEquals(SafetyStatus safetyStatus, Pageable pageable); | ||
|
|
||
| 거 | ||
| Page<Product> findAllByOrderByCreatedAtDesc(Pageable pageable); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 미사용 메서드들은 제거해보시는거 어떨까요 |
||
|
|
||
| @Query(value = "SELECT p FROM Product p WHERE p.productName = :productName " | ||
|
|
||
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.
만약 버저닝을 해야한다면 어떻게 관리할 수 있을까요?
URL로도 관리할 수 있지만 header를 통해서도 관리하는 방법에 대해 보시면 좋을것 같습니다!