-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#27] 경매 상품 정렬 기준에 좋아요, 필터링 조건에 경매 상품 상태 추가 #28
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.
고생 많으셨습니다!
피드백 확인 부탁드려요 ~
@@ -51,10 +52,12 @@ public ProductFindResponse findById(@PathVariable Long id) { | |||
@GetMapping | |||
public PagingResponse findAll( |
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.
요기 제네릭 타입도 추가해주면좋을 것 같아요 ~
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.
앗 넵 추가하겠습니다.
@@ -51,10 +52,12 @@ public ProductFindResponse findById(@PathVariable Long id) { | |||
@GetMapping | |||
public PagingResponse findAll( | |||
@RequestParam(value = "keyword", required = false) String keyword, | |||
@RequestParam(value = "Status", required = false) Status status, |
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.
필드명이 대문자로 시작하고있어요 ~
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.
으악 수정하겠습니다..
@RequestParam(value = "pageNo", defaultValue = "0", required = false) int pageNo, | ||
@RequestParam(value = "pageSize", defaultValue = DEFAULT_SIZE, required = false) int pageSize, | ||
@RequestParam(value = "orderBy", required = false) OrderBy order) { | ||
PagingResponse<ProductFindResponse> productPagingResponse = productFacade.findAll(keyword, pageNo, pageSize, | ||
PagingResponse<ProductFindResponse> productPagingResponse = productFacade.findAll(keyword, status, pageNo, |
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.
이럴땐 굳이 변수로 한번 담을 필요 없을 것 같아요~
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하도록 수정하겠습니다.
// 정렬 조건 설정 | ||
OrderSpecifier<?> orderSpecifier = order != null ? order.toOrderSpecifier(product) : product.id.desc(); | ||
OrderSpecifier<?> orderSpecifier = order != null ? order.toOrderSpecifier() : product.id.desc(); |
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.
OrderBy를 notnull로 취급하고 디폴트 값을 갖게 하는게 더 좋지 않을까요 ?
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.
넵 디폴트값을 가지도록 하는게 코드가 더 깔끔할 것 같습니다. 수정하도록 하겠습니다! 감사합니다
.offset(pageNo * pageSize) | ||
.limit(pageSize) | ||
.fetch(); | ||
return products; |
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.
이렇게 바로 리턴되는건 다 변수로 받을 필요 없어보여요 ~
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.
넵 수정하도록 하겠습니다
|
||
if (order == OrderBy.LIKE) { | ||
List<Product> products = jpaQueryFactory.selectFrom(product) | ||
.leftJoin(productLike).on(product.id.eq(productLike.productId)) |
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.
요거혹시 N+1 쿼리 안날아가나요 ?
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 products; | ||
} | ||
|
||
private BooleanExpression containsKeyword(String keyword) { |
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.
요거 전에 만든 ifNotNull 사용하면 되지 않나요 ?
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.
쿼리문 안에 바로 ifNotNull하는 것보다 containsKeyword()를 사용하는게 가독성이 더 좋다고 생각이 들어서
ifNotNull을 containsKeyword()에서 사용하도록 수정했습니다!
LocalDateTime requestTime = LocalDateTime.now(); | ||
if (status != null) { | ||
switch (status) { | ||
case READY: |
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.
👍
근데 요 내요들도 Status 안에 들어갈 수 있을 것 같아요 ~
추가로, Status 이름이 너무 범용적이라 경매상태 를 뜻하는 이름으로 바꿔주면 좋을 것 같아요 ~
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.
넵 수정하도록 하겠습니다.
validateSaveRequest(productSaveRequest, images); | ||
|
||
Product product = productService.enroll(productSaveRequest); | ||
List<ProductImage> productImages = fileService.uploadImages(user, product, images); |
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.
요부분 ProductImageService로 들어가도 되지 않나 하는 생각이 드네요!
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.
이부분은 한번 고민해보도록 하겠습니다.!
} | ||
} | ||
|
||
private void validateSaveRequest(ProductSaveRequest productSaveRequest, List<MultipartFile> images) { |
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.
👍
Quality Gate passedIssues Measures |
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.
고생 많으셨습니다 ~
이 PR은 여기서 머지 해두도록 할게요 ~
|
||
import com.querydsl.core.types.dsl.BooleanExpression; | ||
|
||
public enum ProductCondition { |
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.
👍
} | ||
|
||
private BooleanExpression filterProductStatus(Status status) { | ||
public BooleanExpression filterProductCondition(ProductCondition productCondition) { |
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.
이거 이렇게 now를 사용하면 테스트할때 좀 어려워질텐데 어떻게 해결해 볼 수 있을지 고민해보면 좋을 것 같네요 ~
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.
넵 알겠습니다! 고민해보도록 하겠습니다
case DONE: | ||
return product.closeDate.loe(requestTime); | ||
} | ||
if (productCondition != null) { |
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.
이것도 애초에 null이 아니라 디폴트값을 가질 수 있게 하면 좋을 것 같아요 ~
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 PagingResponse.from(productFindResponses, pageNo); | ||
} | ||
|
||
@Transactional |
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.
읽는데만 필요한 기능이라면 readOnly 속성을 true로 두는게 좋아요~ 왜 그럴지도 한번 찾아보면 더 좋겠네요!
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.
넵 readOnly 속성에 대해서 공부하겠습니다!
[#27] 경매 상품 정렬 기준에 좋아요, 필터링 조건에 경매 상품 상태 추가 #28