[refactor] 주문 금액 계산 추상화

[scars97]

기존 로직

@Transactional
override fun placeOrder(orderCommand: OrderCommand): Order {

    // 주문 금액 계산 후, 쿠폰 여부에 따라 할인 적용.
    order.calculateOriginPrice(items)
    order.couponId?.let {
        applyCouponTo(order)
    }

}

private fun applyCouponTo(order: Order) {
    val userCoupon = couponPort.findUserCouponBy(order.couponId!!, order.userId)
    couponPort.commandUserCoupon(userCoupon.use())

    order.calculateDiscountPrice(userCoupon.coupon)
}

calculateOriginPrice 와 calculateDiscountPrice 가 굳이 나눠져야할까?
두 메서드 모두 order의 책임으로 주어졌기때문에 주문 금액 계산이라는 하나의 책임으로 묶어도 좋을 것 같다.

---

리팩토링 과정 및 문제 상황

하나의 책임으로 묶기 위해 order에 calculatePrice 라는 메서드를 생성하고
calculateOriginPrice 와 calculateDiscountPrice로 전달하는 파라미터들을 그대로 calculatePrice에 전달.
applyCouponTo 메서드에서는 사용자가 보유하고 있는 쿠폰을 반환한다.

@Transactional
override fun placeOrder(orderCommand: OrderCommand): Order {

    order.calculatePrice(items, applyCouponTo(order))

}

private fun applyCouponTo(order: Order): Coupon {
    val userCoupon = couponPort.findUserCouponBy(order.couponId!!, order.userId)
    couponPort.commandUserCoupon(userCoupon.use())

    return userCoupon.coupon
}

하지만 이렇게 작성한다면 쿠폰이 있는 경우에만 정상 동작할 것이다.
쿠폰 인스턴스를 파라미터로 전달해야하지만 쿠폰이 없는 경우 어떻게 해결할 수 있을까?

---

해결방법 1 : null 전달

단순하게 처리한다면 null로 전달해서 처리할 수 있다.

private fun applyCouponTo(order: Order): Coupon {
    if(order.couponId == null) {
        return null
    }

    // ...
}

// calculatePrice 에 작성된 할인 금액 계산 로직
if(coupon != null) {
    coupon.calculateDiscount(originPrice)
}

하지만 로직마다 null 체크가 추가되어야 하고, 의도가 명확하지 않다.

해결방법 2 : NONE 쿠폰 타입 추가

현재 쿠폰 타입에는 RATE(할인율), AMOUNT(고정금액) 가 있다.
order에서 할인 금액을 계산하기 위해 coupon.calculateDiscount(originPrice)를 호출하는데,
coupon이 가지고 있는 타입에 따라 할인 금액을 계산하고 반환한다.

null을 전달하기보다 NONE 이라는 쿠폰 타입을 추가해서 의도를 명확하게 하면 어떨까

class Coupon(
    // ...
) {
    enum class DiscountType {
        RATE {
            override fun calculateDiscount(price: BigDecimal, discount: Long): BigDecimal { ... }
        },
        AMOUNT {
            override fun calculateDiscount(price: BigDecimal, discount: Long): BigDecimal { ... }
        },
        NONE {
            override fun calculateDiscount(price: BigDecimal, discount: Long): BigDecimal { ... }
        };

        abstract fun calculateDiscount(price: BigDecimal, discount: Long): BigDecimal
    }

쿠폰이 없는 경우, NONE 타입을 가지는 쿠폰을 생성하여 전달한다.

private fun applyCouponTo(order: Order): Coupon {
    if(order.couponId == null) {
        return Coupon.none()
    }

    // ...
}

null 전달보다 조금 더 객체지향적으로 리팩토링이 가능하지 않을까 생각된다.

[scars97]

@claude pr에 올라온 코드를 리뷰해주세요.

[claude]

Claude finished @scars97's task —— View job

---

코드 리뷰 완료 ✅

진행 상황:

• 변경된 파일들 분석
• 리팩토링 접근법 검토
• 코드 품질 및 설계 결정 평가
• 종합적인 리뷰 피드백 제공

---

📋 종합 평가

이번 리팩토링은 주문 금액 계산 로직을 하나의 책임으로 통합한 좋은 개선 작업입니다. 전반적으로 코드의 응집도가 높아졌고, Null Object Pattern을 활용한 해결책이 인상적입니다.

✅ 잘 구현된 부분

1. 책임의 명확한 통합

OrderService.kt:31에서 기존의 분리된 calculateOriginPrice()와 calculateDiscountPrice() 메서드를 calculatePrice()로 통합한 것은 좋은 결정입니다.

// Before: 두 개의 분리된 호출
order.calculateOriginPrice(items)
order.couponId?.let { applyCouponTo(order) }

// After: 하나의 통합된 책임
order.calculatePrice(items, applyCouponTo(order))

2. Null Object Pattern의 우아한 구현

Coupon.kt:27-31, 36-39에서 NONE 타입과 Coupon.none() 팩토리 메서드를 추가한 것이 매우 좋습니다:

NONE {
    override fun calculateDiscount(price: BigDecimal, discount: Long): BigDecimal {
        return BigDecimal.ZERO
    }
}

companion object {
    fun none(): Coupon = Coupon(0L, "none", DiscountType.NONE, 0L, 0, 0L)
}

이 접근법은:

• null 체크를 제거하여 코드를 더 깔끔하게 만듦
• 의도를 명확히 표현 (쿠폰이 "없다"는 상태를 객체로 표현)
• 다형성을 활용하여 일관된 인터페이스 제공

3. 테스트 케이스의 포괄적 커버리지

OrderTest.kt:32-65에서 NONE 타입을 포함한 모든 쿠폰 타입에 대한 테스트를 작성한 것이 훌륭합니다.

🔍 개선 제안사항

1. 메서드 명명 고려사항

applyCouponTo() 메서드명이 실제 동작과 약간 다를 수 있습니다:

// 현재: applyCouponTo - "쿠폰을 적용한다"는 의미
private fun applyCouponTo(order: Order): Coupon

// 제안: getCouponFor 또는 resolveCouponFor - "쿠폰을 가져온다/해결한다"는 의미

실제로는 쿠폰을 "적용"하는 것이 아니라 쿠폰 객체를 "반환"하는 역할이므로, 메서드명이 더 명확할 수 있습니다.

2. Null Safety 개선

OrderService.kt:44에서 non-null assertion(!!)을 사용하고 있습니다:

val userCoupon = couponPort.findUserCouponBy(order.couponId!!, order.userId)

이미 null 체크를 했지만, 더 안전한 접근을 고려해볼 수 있습니다:

private fun applyCouponTo(order: Order): Coupon {
    val couponId = order.couponId ?: return Coupon.none()
    
    val userCoupon = couponPort.findUserCouponBy(couponId, order.userId)
    couponPort.commandUserCoupon(userCoupon.use())
    
    return userCoupon.coupon
}

3. NONE 쿠폰의 의미론적 일관성

Coupon.none()에서 생성하는 쿠폰의 필드값들을 검토해보세요:

fun none(): Coupon = Coupon(0L, "none", DiscountType.NONE, 0L, 0, 0L)

• id = 0L: null 또는 다른 특별한 값이 더 적절할 수 있음
• title = "none": 더 명확한 설명이나 빈 문자열 고려

4. 도메인 규칙 명확화

할인 계산 로직에서 할인 금액이 원가보다 클 경우에 대한 처리가 없습니다. 비즈니스 요구사항에 따라 이를 고려해볼 수 있습니다.

🏆 설계 관점에서의 평가

1. Single Responsibility Principle: ✅ 잘 적용됨
2. Open/Closed Principle: ✅ enum을 활용한 전략 패턴으로 확장 가능
3. Liskov Substitution Principle: ✅ NONE 타입도 동일한 인터페이스 지원
4. Null Object Pattern: ✅ 매우 잘 구현됨

📝 결론

이 리팩토링은 객체지향 설계 원칙을 잘 따른 품질 높은 구현입니다. 특히 Null Object Pattern을 활용하여 복잡성을 줄이고 코드의 가독성을 크게 개선했습니다. 제안한 몇 가지 개선사항들은 코드를 더욱 견고하게 만들 수 있는 방향이지만, 현재 구현도 충분히 훌륭합니다.

---