[#15] 마이페이지 쿠폰 조회 & 사용 가능한 쿠폰 조회 & 쿠폰 사용 #16

codesejin · 2024-04-08T14:09:43Z

관련 Issue : Feat - 마이페이지 쿠폰 조회 & 사용 가능한 쿠폰 조회 & 쿠폰 사용 #15

테이블 증설

쿠폰 사용 로직에 주문 비즈니스 로직이 포함되어야 할것 같아서 테이블 3개를 추가했습니다.

궁금한 점

쿠폰을 사용한 주문 API에서 DB 조회가 많이 일어나고 있는것 같은데, 관련 데이터는 한번에 받아서 DTO에 담아야 할지,
아니면 간단한 SELECT쿼리는 DB 부하에 영향을 크게 주지 않기 때문에 현재로서도 충분한지 궁금합니다.

Subquery, Join 등 코드 리뷰를 더 세심하게 받기 위함

f-lab-maverick

고생하셨습니다. 리뷰 확인해주시고, 수정하신 후 다시 요청해주세요~

f-lab-maverick · 2024-04-09T11:12:18Z

src/main/java/com/flab/offcoupon/controller/MyPageController.java

+    private final MyPageService myPageService;
+    @ResponseStatus(HttpStatus.OK)
+    @GetMapping("/coupons")
+    public ResponseEntity<ResponseDTO<List<AllCouponsByMemberIdResponse>>> getAllCoupons(@RequestParam final long memberId) {


자기자신의 쿠폰 리스트를 확인하는데, 파라미터로 memberId를 받을 필요가 있을까요?
이 유저가 다른 유저의 id를 파라미터로 입력해서 데이터를 조회한다면 어떻게 될까요?

별도의 권한체크가 없으니 parameter manipulation 공격이 성립하겠네요. 이 키워드를 한번 찾아보시고, 취약점을 방어해주세요.

이 부분은 이번 PR에서 수정하지 않고 미루기로했으니, 우선 해당 URL이 오픈되지 않도록 핸들러매핑이 연결되지 않게 @GetMapping("/coupons")을 지워주세요~~

네엡 다음번 PR에서 수정하도록하겠습니다!

f-lab-maverick · 2024-04-09T11:14:18Z

src/main/java/com/flab/offcoupon/controller/OrderController.java

+    @ResponseStatus(HttpStatus.OK)
+    @GetMapping("/available-coupons")
+    public ResponseEntity<ResponseDTO<List<AvailableCouponsByMemberIdResponse>>> getAvailableCoupons(@RequestParam final long memberId,
+                                                                                                     @RequestParam final long productId) {


이 부분도 마찬가지로, 자기자신의 쿠폰 목록을 조회하는 것인데 memberId를 파라미터로 받을 필요가 있을까요?

f-lab-maverick · 2024-04-09T11:21:31Z

src/main/java/com/flab/offcoupon/controller/OrderController.java

+     * @param productId 상품 ID
+     * @return 사용 가능한 쿠폰 목록
+     */
+    @ResponseStatus(HttpStatus.OK)


ResponseStatus는 사용에 주의해야 합니다. Return type이 ResponseEntity이며, 어노테이션 ResponseStatus를 동시에 쓰는 상황에서 두가지 반환값이 서로 다르다면 어떤 status가 반환될까요?

Controller에서 ResponseEntity와 @ResponseStatus를 동시에 사용할때에는 ResponseEntity에서 명시된 상태코드가 우선됩니다. 즉, ResponseEntity에서 명시된 상태코드가 HTTP 응답으로 반환됩니다.

하지만 응답이 2번 발생할 수 있기 때문에 동시에 2개를 사용하는 것은 권장되지 않고, 아래와 같이 (제가 한 실수,,) 응답코드를 Created와 Ok를 둘 다 사용한다면 다른 개발자가 코드를 읽을때 혼동을 줄 수도 있습니다. 따라서 휴먼 에러를 방지하기 위해 왠만하면 Controller에서 @ResponseStatus를 사용하지 않도록 수정하겠습니다!

f-lab-maverick · 2024-04-09T11:26:13Z

src/main/java/com/flab/offcoupon/domain/entity/Product.java

+    private final String title;
+    private final String description;
+    private final long originalPrice; // 원래 가격
+    private final Long salePrice; // null일 경우 전체 할인이 적용되지 않은 것으로 간주


null로 비즈니스 로직을 표현하는 것은 NPE의 가능성을 높입니다.
이런 경우 salePrice를 0으로 기본값을 표기해준다면 어떨까요?

정밀도 향상을 위해 BigDecimal타입으로 변경했고, default 0으로 설정했습니다!

f-lab-maverick · 2024-04-09T11:36:45Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+        List<AvailableCouponsByMemberIdResponse> responseList = new ArrayList<>();
+        filterAvailableCouponsWithMinPrice(productId, availableCoupons, responseList);


Collection을 생성한 후 메서드 내부에서 컬랙션의 상태를 변경시키고 있네요.
이렇게 내부에서 상태를 변경시키는 메서드는 코드를 읽을 때 해당 변수에 대해서 계속해서 트래킹하게 만들기 때문에 피로감을 느끼게 만들고, 메서드의 결과값을 예측할 수 없게 만듭니다.

그렇기 때문에 상태를 변경시키는 것이 아닌, 행위 그 자체를 메서드화한다면 좀 더 깔끔하고 읽기 쉬운 직관적인 코드로 만들 수 있습니다.

Suggested change

List<AvailableCouponsByMemberIdResponse> responseList = new ArrayList<>();

filterAvailableCouponsWithMinPrice(productId, availableCoupons, responseList);

List<AvailableCouponsByMemberIdResponse> responseList = availableCoupons.stream

.filter(coupon -> isOverThenMinPrice(productId))

...

말씀해주신 filter를 사용해보려고 했는데, 할인 가격이 minOrderPrice를 초과하지 않는 범위까지만 누적하는거라 filter 사용이 어려울것 같습니답...

f-lab-maverick · 2024-04-09T11:52:02Z

src/main/java/com/flab/offcoupon/util/DiscountUtils.java

+    /**
+     * 상품의 총 가격을 계산하여 반환하는 메소드입니다.
+     *
+     * @param product  상품 정보
+     * @param quantity 상품 수량
+     * @return 상품의 총 가격
+     */
+    public long calculateTotalPrice(Product product, long quantity) {
+        return getProductPricePerUnit(product) * quantity;
+    }


이 로직또한 비즈니스 로직으로, 유틸리티가 아니기 때문에 Service / Domain Layer에 존재해야하는 로직입니다.

계산하는 로직이 코드 중복될 가능성이 있다고 생각해서 별도의 클래스에 분리하려고 시도했는데, 제가 유틸리티 라는 개념에 대해 미숙했던것 같습니다 수정하겠습니다!

f-lab-maverick · 2024-04-09T11:53:04Z

src/main/java/com/flab/offcoupon/util/DiscountUtils.java

+        long totalDiscountPrice = 0;
+        for (Coupon coupon : couponList) {
+            if (coupon.getDiscountType() == DiscountType.PERCENT) {
+                totalDiscountPrice += getProductPricePerUnit(product) * ((double) coupon.getDiscountRate() / 100);


값을 double type으로 직접 계산하고 있네요. 숫자를 이렇게 원시 타입으로 계산하면 어떤 일이 발생할까요? 원하는 값이 정확하게 나올까요?

double형태로 계산하면 overflow 와 부동 소수점 이슈가 발생합니다. BigDecimal로 변경해서 수정하겠습니다!

f-lab-maverick · 2024-04-09T11:55:16Z

src/main/resources/mapper/CouponIssueMapper.xml

+               ROUND(CASE
+                         WHEN p.sale_price IS NULL THEN
+                                 p.original_price -
+                                 IF(c.discount_type = 'PERCENT', p.original_price * (c.discount_rate / 100),
+                                    c.discount_price)
+                         ELSE
+                                 p.sale_price - IF(c.discount_type = 'PERCENT', p.sale_price * (c.discount_rate / 100),
+                                                   c.discount_price)
+                         END, 0)
+                    AS discounted_price


query에 로직이 있네요~~
정말 불가피한 경우가 아니라면 아래 케이스는 모두 지양해야 합니다.

query에서 분기를 태우는 case-when-then

query에서 값을 계산하는 경우

query에서 비즈니스 로직이 있는 경우

왜냐하면, query는 디버깅하기가 어려우며 수정사항이 발생하였을 경우 유동적으로 변경할 수 없고, 테스트도 짜기 어렵기 때문입니다. 데이터 조회는 심플하게 조회하고, 모든 가공은 어플리케이션에서 진행해주세요~

와! 제가 항상 고민했지만 확신이 없던 내용인데, 이렇게 원인과 케이스별로 정리해주셔서 감사합니다! 애플리케이션 내에서 가공하도록 수정하겠습니다!

f-lab-maverick · 2024-04-09T11:55:45Z

src/main/resources/mapper/CouponIssueMapper.xml

+          AND c.validate_start_date <= #{currentDateTime}
+          AND c.validate_end_date >= #{currentDateTime}


BETWEEN 문을 쓸 수 있엤네요.

네엡 변경했습니다 감사합니다~

f-lab-maverick · 2024-04-09T11:56:30Z

src/main/resources/mapper/CouponMapper.xml

+        select id        as couponId,
+               if(validate_start_date <= #{currentDateTime} and validate_end_date >= #{currentDateTime}, true,
+                  false) as isBetweenValidatePeriod,


이 부분도 쿼리에 로직이 들어간 케이스로 보이네요~

애플리케이션에서 처리하도록 수정했습니다!

쿼리에서 로직 삭제 후 애플리케이션에서 데이터 가공

기존 구현에서 람다 표현식이 외부 변수 accumulatedDiscountPrice를 수정하여 "Variable used in lambda expression should be final or effectively final" 오류가 발생했습니다. AtomicReference 래퍼를 사용하여 accumulatedDiscountPrice 변수를 안전하게 업데이트하도록 수정했습니다.

codesejin

시큐리티 이슈를 아직 해결하지 못해서 parameter manipulation 공격에 대한 내용 제외하고 수정했습니다!

codesejin · 2024-04-15T05:07:51Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+        // 5. 쿠폰 사용 처리
+        couponIssueRepository.updateCouponStatus(request.getCouponIssueId());


아직 메소드 명이 뭐가 적절한지 판단하는게 미숙한것 같습니다! useCoupon 으로 변경하겠습니다 감사합니다!

codesejin · 2024-04-15T05:35:20Z

src/main/resources/mapper/CouponIssueMapper.xml

+               ROUND(CASE
+                         WHEN p.sale_price IS NULL THEN
+                                 p.original_price -
+                                 IF(c.discount_type = 'PERCENT', p.original_price * (c.discount_rate / 100),
+                                    c.discount_price)
+                         ELSE
+                                 p.sale_price - IF(c.discount_type = 'PERCENT', p.sale_price * (c.discount_rate / 100),
+                                                   c.discount_price)
+                         END, 0)
+                    AS discounted_price


와! 제가 항상 고민했지만 확신이 없던 내용인데, 이렇게 원인과 케이스별로 정리해주셔서 감사합니다! 애플리케이션 내에서 가공하도록 수정하겠습니다!

codesejin · 2024-04-15T05:37:02Z

src/main/resources/mapper/CouponIssueMapper.xml

+          AND c.validate_start_date <= #{currentDateTime}
+          AND c.validate_end_date >= #{currentDateTime}


네엡 변경했습니다 감사합니다~

codesejin · 2024-04-15T07:59:59Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+        List<AvailableCouponsByMemberIdResponse> responseList = new ArrayList<>();
+        filterAvailableCouponsWithMinPrice(productId, availableCoupons, responseList);


말씀해주신 filter를 사용해보려고 했는데, 할인 가격이 minOrderPrice를 초과하지 않는 범위까지만 누적하는거라 filter 사용이 어려울것 같습니답...

codesejin · 2024-04-15T10:28:29Z

src/main/resources/mapper/CouponMapper.xml

+        select id        as couponId,
+               if(validate_start_date <= #{currentDateTime} and validate_end_date >= #{currentDateTime}, true,
+                  false) as isBetweenValidatePeriod,


애플리케이션에서 처리하도록 수정했습니다!

codesejin · 2024-04-15T11:05:54Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+    private void filterAvailableCouponsWithMinPrice(long productId, List<AvailableCouponsByMemberIdVo> availableCoupons, List<AvailableCouponsByMemberIdResponse> responseList) {
+        // min_price(상품의 최소 주문 금액)과 비교하여 조건에 맞는 쿠폰만 결과 리스트에 추가합니다.
+        if (!availableCoupons.isEmpty()) {
+            long accumulatedDiscountPrice = 0;
+            for (AvailableCouponsByMemberIdVo availableCoupon : availableCoupons) {
+                // 최소 주문 금액까지 할인 가능한 금액 누적
+                accumulatedDiscountPrice += availableCoupon.discountedPrice();
+                // 할인 금액이 min_price보다 작거나 같으면 결과 리스트에 추가
+                if (accumulatedDiscountPrice <= productRepository.getProductMinOrderPriceById(productId)) {
+                    AvailableCouponsByMemberIdResponse response = new AvailableCouponsByMemberIdResponse(availableCoupon);
+                    responseList.add(response);
+                }
+            }
+        }
+    }
+


인덴트를 줄이기 위해 람다표현식과 AtomicReference를 사용하는걸로 변경했습니다!

f-lab-maverick

고생하셨습니다~ 코멘트 확인해서 수정 후 머지해주세요.

f-lab-maverick · 2024-04-17T14:24:41Z

src/main/java/com/flab/offcoupon/controller/MyPageController.java

+    private final MyPageService myPageService;
+    @ResponseStatus(HttpStatus.OK)
+    @GetMapping("/coupons")
+    public ResponseEntity<ResponseDTO<List<AllCouponsByMemberIdResponse>>> getAllCoupons(@RequestParam final long memberId) {


이 부분은 이번 PR에서 수정하지 않고 미루기로했으니, 우선 해당 URL이 오픈되지 않도록 핸들러매핑이 연결되지 않게 @GetMapping("/coupons")을 지워주세요~~

f-lab-maverick · 2024-04-17T14:37:04Z

src/main/java/com/flab/offcoupon/domain/entity/helper/AvailableCouponInfo.java

+        } else {
+            return calculateDiscountPrice(vo.discountType(), vo.originalPrice(), discountRate, discountPrice);
+        }


else문 없이도 정상 동작 할 것으로 보이네요~ else, else if문은 읽을 때 개발자의 피로감을 늘릴 수 있기 때문에 안쓰는 방법을 연습하는게 좋습니다.

Suggested change

} else {

return calculateDiscountPrice(vo.discountType(), vo.originalPrice(), discountRate, discountPrice);

}

}

return calculateDiscountPrice(vo.discountType(), vo.originalPrice(), discountRate, discountPrice);

넵 early return으로 변경했습니다!

f-lab-maverick · 2024-04-17T14:56:01Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+        for (CouponValidationPeriodVo validationPeriod : isBetweenValidatePeriodVo) {
+            if (!isNowBetweenValidatePeriod(now, validationPeriod)) {
+                throw new CouponUsageInvalidPeriodException(COUPON_USAGE_INVALID_PERIOD
+                        .formatted(validationPeriod.couponId(), validationPeriod.validateStartDate(), validationPeriod.validateEndDate()));
+            }
+        }


이런 부분이 stream의 anymatch를 적용하면 훨씬 깔끔하게 코드가 변경됩니다~

넵 stream API사용하는걸로 변경했습니다!

codesejin

피드백 반영해서 수정했습니다~

codesejin · 2024-04-17T22:45:36Z

src/main/java/com/flab/offcoupon/domain/entity/helper/AvailableCouponInfo.java

+        } else {
+            return calculateDiscountPrice(vo.discountType(), vo.originalPrice(), discountRate, discountPrice);
+        }


넵 early return으로 변경했습니다!

codesejin · 2024-04-17T22:56:53Z

src/main/java/com/flab/offcoupon/service/coupon_use/OrderService.java

+        for (CouponValidationPeriodVo validationPeriod : isBetweenValidatePeriodVo) {
+            if (!isNowBetweenValidatePeriod(now, validationPeriod)) {
+                throw new CouponUsageInvalidPeriodException(COUPON_USAGE_INVALID_PERIOD
+                        .formatted(validationPeriod.couponId(), validationPeriod.validateStartDate(), validationPeriod.validateEndDate()));
+            }
+        }


넵 stream API사용하는걸로 변경했습니다!

codesejin · 2024-04-17T22:57:37Z

src/main/java/com/flab/offcoupon/controller/MyPageController.java

+    private final MyPageService myPageService;
+    @ResponseStatus(HttpStatus.OK)
+    @GetMapping("/coupons")
+    public ResponseEntity<ResponseDTO<List<AllCouponsByMemberIdResponse>>> getAllCoupons(@RequestParam final long memberId) {


네엡 다음번 PR에서 수정하도록하겠습니다!

sonarqubecloud · 2024-04-17T23:00:26Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codesejin and others added 15 commits January 24, 2024 21:11

docs : README.md 수정

f1fe151

docs : Update README.md

02691a4

자동 PR 리뷰를 위한 workflow 파일 추가 by f-lab

8d2ad96

chore : "JPA"에서 "Mybatis"로 변경

c9d9a2f

Subquery, Join 등 코드 리뷰를 더 세심하게 받기 위함

Merge pull request from master

08d2332

docs : Read Me ERD 등 추가

7121a2a

feat : 마이페이지 갖고 있는 쿠폰 조회 & 주문 시 사용 가능한 쿠폰 조회

6c419e5

refactor : 쿠폰 사용 가능 검증 여부 리팩토링

33459ed

feat : 쿠폰 사용 비즈니스 로직 구현

d85e5a9

refactor : list empty체크

c844d64

refactor : 쿠폰 사용 검증 관련 익셉션 처리

53c23e2

refactor : 사용 가능한 쿠폰 목록 조회 리팩토링

c36b34d

refactor : java doc 추가

767f1bb

refactor : 불필요한 코드 제거

d8c047f

refactor : QueryTest @value사용 방법 적용 못함

fca2d3c

codesejin self-assigned this Apr 8, 2024

codesejin added 6 commits April 9, 2024 10:51

refactor : 스키마 수정

3c82ef2

refactor : 스키마 수정

c1b8be5

refactor : 스키마 수정

dc01f8b

chore : properties Value주입 안되서 테스트하느라 직접 명시한거 수정

2b0c15e

refactor : 테스트하느라 @scheduled 주석 처리한거 원복

235238a

refactor : 메소드명, 변수명 명확하게 변경

cfbf82b

codesejin added the enhancement New feature or request label Apr 9, 2024

f-lab-maverick requested changes Apr 9, 2024

View reviewed changes

codesejin added 6 commits April 15, 2024 12:44

refactor : Controller에서 @ResponseStatus삭제

450f9d6

refactor : Product Exception 추가 및 가격 타입 decimal변경

894f7c2

refactor : 메소드 네이밍 변경 - 쿠폰을 사용했다는 명시적인 네이밍

4e3d989

refactor : 주문 시 사용 가능한 쿠폰 조회 기능 리팩토링

784033c

쿼리에서 로직 삭제 후 애플리케이션에서 데이터 가공

refactor : 쿼리에서 로직 지우기

d255cd0

refactor : 소나클라우드 이슈 해결

f1d6941

codesejin commented Apr 17, 2024

View reviewed changes

f-lab-maverick approved these changes Apr 17, 2024

View reviewed changes

codesejin commented Apr 17, 2024

View reviewed changes

refactor : early return & stream API 적용

c0a419d

codesejin merged commit bc8e470 into develop Apr 17, 2024
2 checks passed

		List<AvailableCouponsByMemberIdResponse> responseList = new ArrayList<>();
		filterAvailableCouponsWithMinPrice(productId, availableCoupons, responseList);

		AND c.validate_start_date <= #{currentDateTime}
		AND c.validate_end_date >= #{currentDateTime}

		// 5. 쿠폰 사용 처리
		couponIssueRepository.updateCouponStatus(request.getCouponIssueId());

[#15] 마이페이지 쿠폰 조회 & 사용 가능한 쿠폰 조회 & 쿠폰 사용 #16

[#15] 마이페이지 쿠폰 조회 & 사용 가능한 쿠폰 조회 & 쿠폰 사용 #16

Conversation

codesejin commented Apr 8, 2024 • edited Loading

테이블 증설

궁금한 점

f-lab-maverick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesejin Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesejin Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesejin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-lab-maverick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesejin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 17, 2024

Quality Gate passed

codesejin commented Apr 8, 2024 •

edited

Loading

codesejin Apr 15, 2024 •

edited

Loading

codesejin Apr 15, 2024 •

edited

Loading

codesejin left a comment •

edited

Loading