Skip to content

Comments

[BE] 상품과 판매 상품 간 엔티티 관계 오류 수정 및 상품 리펙토링#310

Merged
KWAK-JINHO merged 27 commits intodevelopfrom
be/refactor/294
Apr 17, 2025
Merged

[BE] 상품과 판매 상품 간 엔티티 관계 오류 수정 및 상품 리펙토링#310
KWAK-JINHO merged 27 commits intodevelopfrom
be/refactor/294

Conversation

@KWAK-JINHO
Copy link
Contributor

@KWAK-JINHO KWAK-JINHO commented Apr 15, 2025

🚀 어떤 기능을 구현했나요 ?

  • 카테고리 필터링 시 null 값 처리 개선
  • 상품 목록 동적 쿼리에서 BooleanBuilder → BooleanExpression으로 개선
  • @transactional@scheduled의 책임 분리 (오늘의 특가 기능)
  • fetchCount() 메서드 fetchOne()메서드로 개선

🔥 어떤 문제를 마주했나요 ?

  • or 조건 필터링 시 예기치 않은 결과 발생 (불필요한 필터 추가)

✨ 어떻게 해결했나요 ?

  • Expressions.allOf메서드 통해서 해결

📝 어떤 부분에 집중해서 리뷰해야 할까요?

  • 각 객체가 적절한 책임 분리가 이루져 있는지

- Map<String, Object>의 반환타입을 SizeOption DTO로 수정

🔗 Resolves: #294
- null일경우 0반환하게 수정

🔗 Resolves: #294
- embedded로 분리를 통해 가독성 향상

🔗 Resolves: #294
- List.of로 변경하여 불변하게 전달

🔗 Resolves: #294
- 구현계층 삭제
- keyword 빈 값으로 받을 수 있게 변경
- 서비스 레이어 분리

🔗 Resolves: #294
- 필터 메서드 호출 전 null 체크 추가

🔗 Resolves: #294
- or 조건으로 카운트개수만 만족해도 필터에 추가 되던 현상 and 조건으로 수정하여 해결

🔗 Resolves: #294
- products 서비스로 위치변경

🔗 Resolves: #294
- BooleanBuilder -> BooleanExpression 으로 변경

🔗 Resolves: #294
- 카테고리id null로 들어올때 처리

🔗 Resolves: #294
import java.time.LocalDateTime;

@Embeddable
@Getter
Copy link
Contributor

Choose a reason for hiding this comment

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

코드 컨벤션 지키기..!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙀

import lombok.NoArgsConstructor;

@Embeddable
@Getter
Copy link
Contributor

Choose a reason for hiding this comment

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

피라미드 형식 지켜야할것같슴다!

// private final CategoryRedisService categoryRedisService;

@Override
public PagedModel<ProductResponse> getProductsByCategory(Long categoryId, int page) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 조회기능은 완전 없어진건가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

상품 조회 기능이 카테고리 디렉토리에 있는것이 이상해서 상품 디렉토리로 가져왔습니다!

implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310'

// Apple Silicon native DNS resolver
implementation 'io.netty:netty-resolver-dns-native-macos:4.1.100.Final:osx-aarch_64'
Copy link
Contributor

Choose a reason for hiding this comment

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

이게 뭔가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macOS 환경에서 Netty가 DNS를 해석하지 못하고 생기는 경고가 있었습니다. 실행에는 문제 없었으나 프로그램 실행시 나오는 경고를 해결하기 위해서 추가한 의존성입니다.

Comment on lines +104 to +112
def generated = 'build/generated/sources/annotationProcessor/java/main'

tasks.withType(JavaCompile).configureEach {
options.getGeneratedSourceOutputDirectory().set(file(generated))
}

clean {
delete file(generated)
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

이게 뭔가요? 설명해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Querydsl을 사용함에 따라 QClass 파일이 생기는데 IDE 설정마다 해당파일이 생기는 경로가 다르게 설정됩니다. 따라서 해당경로가 생기는 위치를 통일하기 위해서 추가한 설정입니다.
clean은 gradle clean시 해당 디렉토리를 명시적으로 삭제하기 위해서 추가했습니다.

).orElse(0L);
}

private BooleanExpression buildProductConditions(final ProductRequest request, final List<Long> categoryIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

BooleanExpression은 처음 봅니다! 무슨 역할을 하는 메서드인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BooleanExpression은 Querydsl에서 제공하는 조건식 클래스입니다. WHERE 절에 사용되는 조건을 표현하기 위해 사용됩니다. 기존에 사용되었던 BooleanBuilder과 다르게 null 조건은 자동으로 조합에서 빠지기 때문에 if문을 없앨 수 있다는 큰 강점을 가지고 있습니다.

Comment on lines +32 to +40
product.getProduct().getProductInfo().getName(),
product.getOption() != null ? product.getOption().getOptionValue() : "기본옵션",
paymentPrice,
orderPrice,
discountPrice,
quantity,
paymentPrice * quantity,
product.getProduct().getMainImage(),
product.getProduct().getBrand(),
product.getProduct().getImage().getMainImage(),
product.getProduct().getProductInfo().getBrand(),
Copy link
Contributor

Choose a reason for hiding this comment

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

embeded로 info 만들었다면 보내실때도 객체 dto로 묶어서 보내면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

일단 DTO를 만들어 놓겠습니다.. 추후에 원하시는 분들께서는 사용해주시면 좋을것 같습니다.. 모든 파일을 까볼수가 없기때문에 😭

.from(product)
.where(buildProductConditions(productRequest, categoryIds))
.fetchOne()
).orElse(0L);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 메서드는 무슨 역할을 하나요? orElse로 왜 0L를 반환하나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 메서드는 페이징처리를 위한 total 개수를 세기 위한 메서드 입니다. fetchOne 메서드는 null을 반환하기 때문에 적절한 처리를 해주었습니다.

Comment on lines +136 to +145
saleProduct.getProduct().getProductInfo().getName(),
saleProduct.getOption() != null ? saleProduct.getOption().getOptionValue() : "기본옵션",
saleProduct.getProduct().getDiscountPrice(),
saleProduct.getProduct().getOriginPrice(),
saleProduct.getProduct().getOriginPrice() - saleProduct.getProduct().getDiscountPrice(),
saleProduct.getProduct().getProductInfo().getDiscountPrice(),
saleProduct.getProduct().getProductInfo().getOriginPrice(),
saleProduct.getProduct().getProductInfo().getOriginPrice() -
saleProduct.getProduct().getProductInfo().getDiscountPrice(),
quantity,
saleProduct.getProduct().getDiscountPrice() * quantity,
saleProduct.getProduct().getMainImage(),
saleProduct.getProduct().getBrand(),
saleProduct.getProduct().getProductInfo().getDiscountPrice() * quantity,
saleProduct.getProduct().getImage().getMainImage(),
saleProduct.getProduct().getProductInfo().getBrand(),
Copy link
Contributor

Choose a reason for hiding this comment

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

위와 동일합니다.

public interface CategoryService {

PagedModel<ProductResponse> getProductsByCategory(Long categoryId, int page);
// PagedModel<ProductResponse> getProductsByCategory(Long categoryId, int page);
Copy link
Contributor

Choose a reason for hiding this comment

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

나중에 필요한 것일까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

삭제했습니다! 감사합니당

Comment on lines 18 to 22
for (SizeOption option : productOptions) {
String optionValue = option.optionValue();
String[] colorAndSize = optionValue.split("/");

if (colorAndSize.length == 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분을 sizeOption의 역할로 두면 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은의견 감사합니다. 수정했습니다!

Comment on lines 13 to +16
ProductOption productOption = new ProductOption(
(Long) option.get("saleProductId"),
(String) option.get("optionValue"),
(int) option.get("optionExtra")
option.saleProductId(),
option.optionValue(),
option.optionExtra()
Copy link
Contributor

Choose a reason for hiding this comment

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

Option을 안넣고 필드로 넣으신 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option객체를 생성해서 직접 값을 주입하지 않았냐고 여쭤보시는게 맞을까요?
객체 생성을하지 않고 값 전달을 위해 필요한 필드만 뽑아 내는것이 좋다고 생각했습니다.

Comment on lines +56 to +57
int orderPrice = saleProduct.getProduct().getProductInfo().getOriginPrice();
int paymentPrice = saleProduct.getProduct().getProductInfo().getDiscountPrice();
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 이 부분이 메소드 체이닝의 문제라고 생각듭니다. 무언가가 변경되었을때 다른 곳에서도 변경이 되니까요..
이 경우에는 어쩔 수 없지만 추후에 필드가 추가되서 변경이 가해지면 사이드 임펙트가 더 커질텐데 어떻게 하는게 맞을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

getProductInfo에 필드가 많아져서 또한번 embed로 만든다면?

Copy link
Contributor Author

@KWAK-JINHO KWAK-JINHO Apr 15, 2025

Choose a reason for hiding this comment

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

저도 이번에 embed를 분리하면서 생긴 사이드 이펙트를 겪고 주신 의견에 매우 큰 공감을 합니다..
현재 위의 OrderDetail의 from과 같은 메서드들은 디미터의 법칙을 준수하지 못하고 있다고 생각합니다. saleProduct의 내부구조를 OrderDetail은 몰라야 하지 않을까요? 하지만 설계단계부터 명확한 책임분리를 잡으면서 가지 못했기때문에 어쩔 수 없는 현상이라고 생각합니다..

Comment on lines 31 to 37
new String[] {
product.getImage().getImage1(),
product.getImage().getImage2(),
product.getImage().getImage3(),
product.getImage().getImage4()
},
product.getImage().getDetailImage(),
Copy link
Contributor

Choose a reason for hiding this comment

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

List를 사용하지 않은 이유랑 embed 같이 객체로 안 만드신 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 그렇네요.. List로 수정했습니다. 위에서 만든 embed를 사용해서 반환했습니다.
클라이언트 요구사항에 맞게 이미지는 List으로 반환했습니다.


try {
productDiscountService.updateDailyDeals();
log.info("오늘의 특가 뽑기가 잘 완료되었습니다. 박수!!");
Copy link
Contributor

Choose a reason for hiding this comment

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

박수

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🍾🥂🎉

Comment on lines +83 to +86
case 0 -> priceBuilder.or(productInfo.discountPrice.between(0, 25000));
case 25000 -> priceBuilder.or(productInfo.discountPrice.between(25001, 50000));
case 50000 -> priceBuilder.or(productInfo.discountPrice.between(50001, 100000));
case 100000 -> priceBuilder.or(productInfo.discountPrice.gt(100000));
Copy link
Contributor

Choose a reason for hiding this comment

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

확장을 고려한다면 enum을 사용해 보는 것도 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

추후 요구사항이 변경될 가능성이 있어 임시로 작성한 코드입니다.

@KWAK-JINHO KWAK-JINHO merged commit df56af0 into develop Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants