주문 코드 리팩토링

최근 우아한 객체지향, KSUG 채널의 스프링캠프 2023 [Session 3] 대규모 엔터프라이즈 시스템 개선 경험기를 보고난 뒤 많은 영감이 떠올라 가장 최근에 했던 프로젝트를 리팩토링 했다. 사실 리팩토링 이라기 보다는 코드를 다시 작성했다고 보는 것이 맞는 것 같다.

 

사람마다 생각이 다르고 시스템마다 요구사항, 환경이 다르기 때문에 지금의 코드를 작성하면서 한 생각은 어떤 코드에서는 좋지 못할수도 있다. 그럼에도 나는 유지 보수성을 목표로 코드를 작성했다.

 

여기서 말한 유지 보수성이란 두려움 없이, 주저함 없이, 저항감 없이 코드를 변경할 수 있는 능력을 의미한다.

위 문구는 조영호님의 말로, 유지보수의 본질을 가장 명확하게 설명하고 있으며, 나의 마음에 가장 와닿았다.

 

---

자, 이제 코드를 개선해보자.

 

내가 주로 고민한 부분은 다음과 같다.

• id 참조 vs 객체 참조
• 트랜잭션의 분리
• 절차적 프로그래밍 vs 객체지향 프로그래밍
• 패키지 구조

이제 시스템의 요구사항을 살펴보자

 

주문 플로우와 기존의 시스템에 대해 설명하자면 다음과 같다.

 

아, 말하기 앞서 프로젝트는 "1인 가구를 위한 반찬가게" 라는 프로젝트이다.

 

주문 플로우: 

1. 장바구니에 상품을 담는다. (주문은 무조건 장바구니를 거친 후 주문을 해야하며 여러 가게의 상품을 같은 장바구니에 담기 가능)

2. 각 상품 별 옵션을 선택한다.(하나의 상품은 여러가지 옵션을 가질 수 있다. 곱빼기, 야채 x)

3. 주문을 생성하고 결제를 요청한다.

4. 결제가 완료되면 배송이 생성 되며 주문의 책임이 끝난다.

 

위 과정을 보면 다른 가게의 상품을 장바구니에 담을 수 있다는 점을 제외하고는 배달의 민족의 주문 플로우와 유사하다.

 

시스템:

• 주문이 저장될 때, Kafka라는 메시지 브로커를 통해 이벤트가 발행되고, 해당 이벤트는 Elasticsearch 검색 엔진에 주문 정보가 저장되도록 처리했다.
  -> Kibana는 Elasticsearch와 통합이 잘되기 때문에 Kibana의 통계 기능을 이용하기 하며, 주문 조회 성능 개선을 위해 NoSQL을 적용했다.
• 주문이 저장될 때의 이벤트 발행은 AOP를 통해 저장 메소드가 종료될 때 메시지가 발행되도록 했다.
• 배송 생성에 대한 책임도 주문 생성시 같은 트랜잭션 내에서 작동하도록 했다.

 

기존의 주문 엔티티를 보면 아래와 같다.

 

@Entity
@Getter
public class Orders {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "order_id")
    private Long id;

    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "user_id")
    private User user;

    @OneToMany(mappedBy = "order", cascade = CascadeType.ALL, fetch = FetchType.LAZY)
    private List<OrderItem> orderItems = new ArrayList<>();

    @OneToOne(cascade = CascadeType.ALL, fetch = FetchType.LAZY, orphanRemoval = true)
    @JoinColumn(name = "delivery_id")
    private Delivery delivery;

    @Enumerated(EnumType.STRING)
    private OrderStatus status;

    private LocalDateTime orderDate;

    @Embedded
    private Payment payment;

    public void initializeUser(User user) {
        this.user = user;
        user.getOrders().add(this);
    }

    public void addOrderItem(OrderItem orderItem) {
        orderItems.add(orderItem);
        orderItem.initializeOrder(this);
    }

    public static Orders createOrder(User user, List<OrderItem> orderItems, Payment payment, Delivery delivery) {
        Orders order = new Orders();
        order.initializeUser(user);
        orderItems.forEach(entity -> order.addOrderItem(entity));
        order.initializeDelivery(delivery);
        order.updateOrderStatus(OrderStatus.ORDER);
        order.updateOrderDate();
        order.updatePayment(payment);

        return order;
    }

    public void updateOrderStatus(OrderStatus status) {
        this.status = status;
    }

    private void updatePayment(Payment payment) {
        this.payment = payment;
    }

    private void updateOrderDate() {
        this.orderDate = LocalDateTime.now();
    }

    public void initializeDelivery(Delivery delivery) {
        this.delivery = delivery;
        delivery.initializeOrder(this);
    }

    public void cancelDelivery() {
        this.delivery = null;
    }
}

 

먼저 엔티티 코드를 보면 순수 도메인 로직과 jpa 때문에 생성해야 하는 로직이 섞여 있어 별로 좋아보이지는 못한다. (이 코드는 내가 약 5달 전 작성한 코드인데 지금 보니 너무 못짰다는 생각이 든다.)

 

무엇보다 delivery, Order, OrderItem(내부로 들어가면 Item과 연결되어 있다.), 객체가 양방향으로 연관되어 있어 의존관계가 서로 순환한다는 문제가 있다.

 

객체참조 vs id 참조

그렇다면 의존 관계가 순환되면 어떤 문제가 발생할까?

 

그 전에, 의존에 대해 간략히 정의하자면 나의 경우 의존을 "알고 있다" 라는 단어로 치환해서 생각한다. 

위 상황을 예로 들자면 delivery 객체가 order 객체를 알고 있고, order 객체가 delivery 객체를 알고 있다.

 

즉, 한 쪽에서 변경이 일어나면 원래 알고 있던 객체가 아닌 변경된 객체를 갖고 코드를 작성해야 하기 때문에 변경된 코드에 맞춰서 로직을 재작성해야 할 가능성이 커진다. 또한 객체를 조회할 때 해당 객체가 참조하고 있는 객체들이 계속해서 따라와 성능적으로도 약간의 문제가 있을 것이다.

 

위와 같은 단점에도 불구하고 객체를 조회할 때 객체 참조가 주는 편리함은 강력하기 때문에 어떠한 경우에 객체참조를 하면 좋을지 고민해봤다.

• 생명 주기가 같을 경우
• 비즈니스 로직 상 계속해서 같이 조회가 되는 경우

일단은 무조건 id를 통해 참조하고 위 조건에 부합하면 객체 참조를 하며 의존 관계를 끊어냈다. 이 경우, order와 orderItems는 무조건 붙어다니기 때문에 객체참조를 했고 delivery는 참조를 끊어줬다.

 

트랜잭션 분리

 

@Transactional
    public OrderIndexDto order(String userEmail, Payment payment) {
        User user = userRepository.findByEmail(userEmail).orElseThrow(() -> new SessionExpiredException());
        OrderExceptionCheckFactory.checkAddress(user);
        Cart cart = cartRepository.findCartByUserEmail(userEmail).orElseThrow(() -> new CartNotFoundException());
        List<Items> items = cart.getCartItems().stream().map(cartItem -> cartItem.getItem()).collect(Collectors.toList());
        Orders order = Orders.createOrder(user, convertCartItemToOrderItem(items), payment, Delivery.createDelivery(user.getAddress()));
        user.resetCart();

        return OrderIndexDto.from(order);
    }

    private List<OrderItem> convertCartItemToOrderItem(List<Items> items) {
        List<OrderItem> orderItems = new ArrayList<>();
        for (Items item : items) {
            optimisticLockQuantityFacade.subtractQuantity(item.getId());
            orderItems.add(OrderItem.createOrderItem(item));
        }
        return orderItems;
    }

 

위 코드를 보면 하나의 메소드 내에서 회원 조회, 장바구니 조회, 상품 조회, 상품 재고 감소, 배송 생성을 한번에 하고 있는 것을 볼 수 있다. 트랜잭션을 위와 같이 길게 가져가면 성능적, 디버깅, 가독성 모두 좋지 못할 것이다.

 

그렇다면 위 코드를 어떻게 분해하면 좋을까?

 

고민한 결과 메소드를 주요 기능, 보조 기능으로 관점을 나눠서 봤다.

 

위 코드의 핵심적인 기능은 "주문"이다. 장바구니 초기화, 상품 재고 감소, 배송 생성 등 이러한 부분들은 모두 주문이라는 이벤트가 발생했기 때문에 일어날 수 있는 서브 기능으로 바라봤다. 따라서 주문 생성 트랜잭션, 배송 생성 트랜잭션, 장바구니 비우기를 이벤트 기반으로 분리했다. 이 때 일관성을 어떻게 지킬 지에 대한 고민을 하게 됐는데, 이 부분은 로직상 예외가 얼마나 발생할지, 이벤트를 받는 코드에서 예외를 잘 다룰 수 있을지를 고민하며 트랜잭션을 분리했다.

 

즉, 주문은 주문이라는 이벤트를 발행하고 책임을 끝내고, 이벤트를 구독하고 있는 곳에서 부가적인 로직을 처리하며 예외까지 다루도록 했다.

 

절차적 프로그래밍 vs 객체지향 프로그래밍

검증 로직을 작성하며 고민한 부분이다.

 

다음의 검증 상황을 살펴보자

• 주문 시 주문한 메뉴가 가게에 존재하는지 검증한다.
• 주문 시 상품 재고가 부족하지 않은지 검증한다.
• 주문 시 주문 옵션이 유효한 옵션인지 검증한다.
• 주문 취소 시 배송이 출발했는지 검증한다.

실제 검증 로직은 더욱 복잡할 것이다.

위 로직을 객체지향 적으로 작성한다면 order -> orderItems.validate -> menu.validate -> menuOption.validate 와 같은 형식으로 자기가 갖고있는 객체의 검증 로직을 하나씩 호출해 나갈 것이다. 하지만 이런식으로 작성하게 된다면 코드의 흐름을 알기 어려울 뿐만 아니라, 나도 모르는 새 패키지 간에 의존성이 순환 할 수 있다. 주문을 생성할 때 메뉴를 검증하고 주문을 취소할 때 배송을 검증하는 것은 당연하다고 볼 수 있지만 이런식으로 객체를 계속해서 호출하다 보면 주문과 배송, 주문과 메뉴 사이에 의존 관계가 생길 수 있다. 또한 실제 서비스에서는 검증 로직이 더욱 복잡하며 변경될 가능성이 있을텐데 이러한 코드를 서비스 클래스에 작성하는 것은 유지보수성으로 부터 멀어지는 길이라 생각한다.

 

따라서 나는 이런 연쇄적으로 호출해야 하는 코드는 하나의 클래스에 전부 넣었다. 이렇게 하면 복잡한 검증 로직을 관리하기 쉬워지고 의존성 또한 검증 클래스 하나에만 주입되어 의존성이 순환할 가능서도 없어진다. 또한 검증 로직이 추가됐을 경우 메소드만 추가하면 될 것이다.

하나의 클래스에 메뉴 검증, 배송 검증, 주문 옵션 검증 등 다른 관계에 있는 도메인의 검증 로직들을 넣은 것은 객체지향 보다는 절차적 프로그래밍 방식에 가깝다고 할 수 있지만, 현재의 상황에서 유지보수성에는 이 절차적인 방법이 더 가깝다고 생각한다. 이렇듯 절차적인 방식으로 의존성을 역전 할 수 있다.

 

객체지향적인 코드를 작성하는 이유는 결국 유지보수성에 강한 코드를 작성하기 위해서라고 생각하는데, 때로는 절차적인 방법이 유지보수성에 가까울 수 있다. 즉, 객체지향을 위한 객체지향이 아닌 유지 보수성을 위한 객체지향을 추구하는 것이 더욱 괜찮은 코드를 작성하게 해주는 것 같다.

 

패키지 구조

다음으로 패키지 구조는 어떻게 하면 좋을지 생각했다.

앞서 영상 링크와 다른 아키텍처를 보며 영감을 얻었으며 아직까지는 이 방식이 가장 좋다고 생각이 들어 다음과 같은 구조로 작성했다.

 

이런식으로 패키지를 구성하면 뭐가 좋을까?

• 먼저 유지보수성에 용이하다. 예를 들어 요구사항이 JPA가 아닌 API 기반으로 변경된다면, 어댑터 구현체만 갈아 끼워주면 된다.
• 이벤트 코드와 어플리케이션 코드를 완전히 분리할 수 있다.
• 어플리케이션 외부의 코드가 변경되더라도 기존의 로직은 잘 지켜질 것이다.(도메인이 어떤 외부 객체에도 의존하고 있지 않다.)

 

흐름을 보면 아래와 같다. 

1. 컨트롤러에서 서비스 코드를 호출한다.

2. 서비스 코드는 어댑터를 통해 영속성 영역에 접근한다. (헥사고날 아키텍처의 output 포트와 유사한 역할을 한다. 어댑터란 네이밍이 맞는지 고민되지만 일단 어댑터라고 작성했다.)

3. 영속성 영역은 다시 서비스 코드에 응답한다.

4. 2, 3의 과정을 통해 트랜잭션이 종료되면 도메인은 부가적인 기능을 처리하기 위해 이벤트를 발행하며 컨트롤러에 응답한다.

 

번외

1. 해당 도메인과 관련한 이벤트를 다른 곳에서 받게 된다면 이벤트 버스를 통해 이벤트를 받아 어플리케이션 로직에 접근한다.

2. 이후 과정은 앞서 설명한 2~4의 과정과 동일하다.

 

 

여기서 고민은 역시 이벤트 발행이다. 이벤트를 받은 클래스에서 예외가 발생되면 컨트롤러에서 호출한 서비스 코드는 컨트롤러에 ok라는 응답을 보내면 안되지 않을까? 역시 정답은 상황에 따라 다르다로 결론지었다. 비즈니스 로직상 강하게 결합되어야 하는 트랜잭션은 이벤트 기반이 아닌 같은 메소드 내에 묶어주는 것이 좋다고 생각한다.

 

하지만 주문이 완료됐을 때 배송을 생성해야 한다는 요구사항을 생각해보자. 물론 트랜잭션을 묶어주는 것도 괜찮을 수 있지만 과연 배송 생성에 오류가 발생한다고 해서 주문을 다시 해야하는 것이 맞을까? 내가 사용자였다면 좋은 경험이라고 생각하진 않을 것이다. 즉, 이벤트를 발행하고 나머지 처리는 Order는 모르게 하는 것이 더욱 괜찮을 것이다. 또한 delivery 생성에 예외가 발생했을 경우 예외의 복구도 delivery에서 처리하는 것이 좋아 보인다.

 

완벽하진 않지만 나의 고민을 풀어낸 코드를 살펴보자

@Service
@RequiredArgsConstructor
public class OrderService {

    private final OrderAdapter orderRepository;
    private final OrderCartMapper orderCartMapper;
    private final OrderValidator orderValidator;

    @Transactional
    public OrderInfo saveOrder(Cart cart) {
        var order = orderCartMapper.mapFrom(cart);
        order.ordered(orderValidator);
        orderRepository.save(order);

        return OrderInfo.from(order);
    }

    @Transactional
    public void payOrder(Long orderId, Payment payment) {
        var order = orderRepository.findById(orderId).orElseThrow(IllegalArgumentException::new);
        order.pay(payment);
        orderRepository.save(order);
    }

    @Transactional
    public OrderInfo cancelOrder(Long orderId) {
        var order = orderRepository.findById(orderId).orElseThrow(IllegalArgumentException::new);
        order.cancel(orderValidator);
        return OrderInfo.from(order);
    }

    @Transactional(readOnly = true)
    public List<OrderInfo> findAll(Long userId, Pageable pageable) {
        return orderRepository.findAllByUserId(userId, pageable)
                .stream()
                .map(OrderInfo::from)
                .toList();
    }

    @Transactional(readOnly = true)
    public OrderInfo findById(Long id) {
        return orderRepository.findById(id)
                .map(OrderInfo::from)
                .orElseThrow(IllegalArgumentException::new);
    }
}

서비스 코드는 이전과 비교했을 때 책임은 덜어지고 흐름은 알기 쉬워졌다.

 

@Entity
@Getter
@Table(name = "orders")
public class Order extends AbstractAggregateRoot<Order> {
    public enum OrderStatus {ORDERED, PAYED, CANCELLED, DELIVERING, DELIVERED}

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "order_id")
    private Long id;

    @Column(name = "user_id")
    private Long userId;

    @OneToMany(cascade = CascadeType.ALL)
    private List<OrderItem> orderItems = new ArrayList<>();

    @Enumerated(EnumType.STRING)
    @Column(name = "status")
    private OrderStatus status;

    @Column(name = "order_date")
    private ZonedDateTime orderDate;

    @OneToOne(fetch = FetchType.LAZY, cascade = CascadeType.PERSIST)
    @JoinColumn(name = "payment_id")
    private Payment payment;

    Order() {
    }

    public Order(Long userId, List<OrderItem> orderItems) {
        this.userId = userId;
        this.orderItems.addAll(orderItems);
        this.orderDate = ZonedDateTime.now(ZoneId.of("Asia/Seoul"));
    }

    public void ordered(OrderValidator orderValidator) {
        orderValidator.validateStockQuantity(this);
        this.status = OrderStatus.ORDERED;
    }

    public void cancel(OrderValidator orderValidator) {
        orderValidator.validateCancel(this);
        this.status = OrderStatus.CANCELLED;
    }

    public void pay(Payment payment) {
        this.status = OrderStatus.PAYED;
        this.payment = payment;
        registerEvent(new OrderPayedEvent(this));
    }


    public void deliver() {
        this.status = OrderStatus.DELIVERING;
    }

    public Money calculateTotalPrice() {
        return orderItems.stream()
                .map(OrderItem::calculatePrice)
                .reduce(Money.ZERO, Money::plus);
    }

}

domain 클래스도 jpa 때문에 지저분했던 코드도 어느정도 정리됐으며 POJO에 가까운 객체가 되었다. AbstractAggregateRoot는 해당 도메인이 트랜잭션이 커밋된 후 이벤트를 발행하게 해주는 편리한 클래스다. 필요하다면 커스텀해서 사용할 수 있다.

 

@Component
@RequiredArgsConstructor
public class DeliveryCreatedEvent {

    private final DeliveryAdapter deliveryRepository;

    @Async
    @EventListener
    @Transactional
    public void handle(OrderPayedEvent orderPayedEvent) {
        var order = orderPayedEvent.getOrder();
        order.deliver();
        var user = orderPayedEvent.getUser();

        deliveryRepository.save(Delivery.createDelivery(order.getId(), user.getAddress()));
    }
}

발행한 이벤트는 각 도메인의 알맞은 패키지에서 소비하며 요구사항에 맞게 작성하면 된다. 메시지 큐에 메시지를 발행하는 부분도 위와 다를 바 없을 것이다. (현재 이벤트 스트리밍 플랫폼을 이용할 정도의 분리된 서비스는 아니기 때문에 카프카와 관련한 내용은 제거할 생각이다.)

 

코드의 강력한 유지보수성을 확보하는 것은 어려운 일이며, 완벽한 코드를 추구하다 보면 기능 구현에 집중하지 못할 수도 있다. 이런 상황은 때로는 서비스 제공보다는 단순히 개발만을 위한 작업처럼 느껴질 수 있다. 나의 경우  이러한 함정에 빠지지 않기 위해 기능 구현은 최대한 빠르게 하고자 노력하며, 리팩토링 기간을 길게 잡고 다양한 생각을 하는 것 같다. 왜 객체지향 이라는 것을 해야하는지, 내가 왜 코드를 작성하고 있는지 생각하며 목적에 맞는 코드를 작성하자.