[Spring] enum abstract method로 중복을 해결해보자

이번 심화 프로젝트를 진행하다가 튜터님께 피드백 받은 내용이 있다.

사실 이 부분은 정신 놓고 코드 짠거 같아서 부끄럽지만..같은 실수를 하지 않기 위해 정리해본다.

 

🛠️ 변경 전 코드

우선 OwnerOrderController는 가게 사장님이 들어온 주문을 수락 및 거절하는 API를 가지고 있다. 수락이 되었다면 음식조리중, 배달중, 배달완료 순으로 흐를 수 있도록 해야한다. 다음 코드를 보면, 각각의 상태에 따라 API가 다 따로 만들어져있다. 처음에는 상태가 되돌아 갈 수 없도록 하는 것에만 집중해서 예외 처리만 하고 끝냈다.

@RestController
@RequiredArgsConstructor
@RequestMapping("/api")
public class OwnerOrderController {

    private final OwnerOrderService ownerOrderService;

    @Order
    @PatchMapping("/v1/orders/{orderId}/accept")
    public ResponseEntity<OrderStatusResponseDto> acceptOrder(
            @Auth AuthUser owner,
            @PathVariable Long orderId
    ) {
        return ResponseEntity.ok(ownerOrderService.acceptOrder(owner.getId(), orderId));
    }

    @Order
    @PatchMapping("/v1/orders/{orderId}/reject")
    public ResponseEntity<OrderStatusResponseDto> rejectOrder(
            @Auth AuthUser owner,
            @PathVariable Long orderId
    ) {
        return ResponseEntity.ok(ownerOrderService.rejectOrder(owner.getId(), orderId));
    }

    @Order
    @PatchMapping("/v1/orders/{orderId}/cook")
    public ResponseEntity<OrderStatusResponseDto> startCooking(
            @Auth AuthUser owner,
            @PathVariable Long orderId
    ) {
        return ResponseEntity.ok(ownerOrderService.startCooking(owner.getId(), orderId));
    }

    @Order
    @PatchMapping("/v1/orders/{orderId}/deliver")
    public ResponseEntity<OrderStatusResponseDto> startDelivery(
            @Auth AuthUser owner,
            @PathVariable Long orderId
    ) {
        return ResponseEntity.ok(ownerOrderService.startDelivery(owner.getId(), orderId));
    }

    @Order
    @PatchMapping("/v1/orders/{orderId}/complete")
    public ResponseEntity<OrderStatusResponseDto> completeDelivery(
            @Auth AuthUser owner,
            @PathVariable Long orderId
    ) {
        return ResponseEntity.ok(ownerOrderService.completeDelivery(owner.getId(), orderId));
    }
}

 

이런 경우는 문제점이 있다.

만약, API가 점점 늘어나서 몇 백개, 몇 만개가 된다면?

물론 주문하고 배달하기까지의 과정이 그렇게까지 많은 상태로 쪼개지긴 힘들겠지만, 어떤 상태가 추가된다면 또 새로운 API를 추가해야한다. 그리고 다음과 같이 상태 하나하나 따로 호출하게 된다.

음...🤔

PATCH api/v1/orders/1/reject

PATCH api/v1/orders/1/cook

PATCH api/v1/orders/1/deliver

 

...이렇게 API URL만 적다가 끝나버릴수도...

 

 

그리고 OwnerOrderService를 보면 한 눈에 봐도 중복되는 코드가 아주 많다는 것을 볼 수 있다. 중복되는 코드가 눈에 보이면 빠르게 하나로 합칠 수 있다.

@Service
@RequiredArgsConstructor
public class OwnerOrderService {

    private final UserOrderService userOrderService;

    @Transactional
    public OrderStatusResponseDto acceptOrder(Long ownerId, Long orderId) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);

        if (!Objects.equals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }

        if (!OrderStatus.PENDING.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.CANNOT_CANCEL_ORDER.getMessage());
        }

        order.updateOrderStatus(OrderStatus.ACCEPTED);

        return OrderStatusResponseDto.from(order);
    }

    @Transactional
    public OrderStatusResponseDto rejectOrder(Long ownerId, Long orderId) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);

        if (!Objects.equals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }

        if (!OrderStatus.PENDING.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.CANNOT_CANCEL_ORDER.getMessage());
        }

        order.updateOrderStatus(OrderStatus.REJECTED);

        return OrderStatusResponseDto.from(order);
    }

    @Transactional
    public OrderStatusResponseDto startCooking(Long ownerId, Long orderId) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);

        if (!Objects.equals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }

        if (OrderStatus.DELIVERING.equals(order.getOrderStatus())
        || OrderStatus.COMPLETED.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.ALREADY_DELIVERING_OR_COMPLETED.getMessage());
        }

        if (!OrderStatus.ACCEPTED.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_ACCEPTED_BEFORE_COOKING.getMessage());
        }

        order.updateOrderStatus(OrderStatus.COOKING);

        return OrderStatusResponseDto.from(order);
    }

    @Transactional
    public OrderStatusResponseDto startDelivery(Long ownerId, Long orderId) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);

        if (!ObjectUtils.nullSafeEquals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }

        if (OrderStatus.DELIVERING.equals(order.getOrderStatus())
                || OrderStatus.COMPLETED.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.ALREADY_DELIVERING_OR_COMPLETED.getMessage());
        }

        if (!OrderStatus.COOKING.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_COOKING_BEFORE_DELIVERY.getMessage());
        }

        order.updateOrderStatus(OrderStatus.DELIVERING);

        return OrderStatusResponseDto.from(order);
    }

    @Transactional
    public OrderStatusResponseDto completeDelivery(Long ownerId, Long orderId) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);

        if (!ObjectUtils.nullSafeEquals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }

        if (OrderStatus.COMPLETED.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.ALREADY_COMPLETED.getMessage());
        }

        if (!OrderStatus.DELIVERING.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_DELIVERING_BEFORE_COMPLETE.getMessage());
        }

        order.updateOrderStatus(OrderStatus.COMPLETED);

        return OrderStatusResponseDto.from(order);
    }

}

 

다시 봐도 생각없이 짠 코드같아서 스스로도 어이가 없다. 대략 요약하자면 다음과 같은 부분들이 반복된다.

 

▪️유저 아이디를 검증하는 코드

▪️ 현재 들어온 상태 변경을 하기에 적절한 상태인지 확인하는 코드

▪️order.updateOrderStatus()로 주문 상태를 변경하는 코드

▪️ OrderStatusResponseDto을 반환하는 코드

 

변경 전에는 다음과 같이 enum 클래스에서 상태명만 가져와 사용하고 있다.  

@Getter
@RequiredArgsConstructor
public enum OrderStatus {

    PENDING("주문 확인전"),
    CANCELED("주문 취소"),
    ACCEPTED("주문 접수"),
    REJECTED("주문 거절"),
    COOKING("조리중"),
    DELIVERING("배달중"),
    COMPLETED("배달 완료");

    private final String description;

}

 

이제 리팩토링을 해보자!

✨ 리팩토링한 코드

튜터님의 피드백을 정리해보면 다음과 같다.

 

✔️ 중복되는 코드가 너무 많다.

✔️ 상태가 추가되면 API를 계속 추가해야하고, 관리가 어려워진다는 문제가 생긴다. 주문 상태를 파라미터마다 받는 것은 어떨까?

✔️ 각 상태마다 switch문으로 분기할 수 있을 것이다.

✔️ 각 상태별 예외처리를 메서드로 분리해 주문 상태에 따른 처리 로직만 남기면 코드를 간소화할 수 있다.

✔️ 현재 enum 클래스로 상태를 관리하고 있으므로, enum 내에 추상 메서드를 이용해 그 안에서 예외처리를 해볼 수도 있다.

 

먼저, 주문상태를 @RequestParam으로 받도록 변경했다. 다음과 같이 단 하나의 API만 남기고 모두 지웠다.

@RestController
@RequiredArgsConstructor
@RequestMapping("/api")
public class OwnerOrderController {

    private final OwnerOrderService ownerOrderService;

    @Order
    @PatchMapping("/v1/orders/{orderId}")
    public ResponseEntity<OrderStatusResponseDto> order(
            @Auth AuthUser owner,
            @PathVariable Long orderId,
            @RequestParam String status
    ) {
        return ResponseEntity.ok(ownerOrderService.order(owner.getId(), orderId, status));
    }

}

 

그러면 다음과 같이 호출할 수 있다. 주문 상태에 따라 status 값만 바꿔주면 된다.

PATCH /api/v1/orders/1?status=ACCEPTED

 

이제 이거 하나만 있으면 된다..!

 

 

그 다음 OwnerOrderService로 가보면 이전과 달리 아주 심플해진 코드를 볼 수 있다.

 

✔️ 사용자를 검증하는 코드를 private 메서드로 분리해 order() 메서드 내에서는 validateOwner() 한 줄로 적었다.

✔️ OrderStatus enum 클래스 내부에 추상 메서드 update()를 선언해 역시 OrderStatus.valueOf(status).update(order); 한 줄로 적었다.

@Service
@RequiredArgsConstructor
public class OwnerOrderService {

    private final UserOrderService userOrderService;

    @Transactional
    public OrderStatusResponseDto order(Long ownerId, Long orderId, String status) {
        UserOrderEntity order = userOrderService.getOrderById(orderId);
        validateOwner(ownerId, order);
        OrderStatus.valueOf(status).update(order);

        return OrderStatusResponseDto.from(order);
    }

    private void validateOwner(Long ownerId, UserOrderEntity order) {
        if (!Objects.equals(ownerId, order.getStore().getUser().getId())) {
            throw new InvalidRequestException(ErrorOrderMessage.NOT_OWNER_ORDER.getMessage());
        }
    }

}

 

그러면 이제 변경사항이 생겨도 사실상 Controller와 Service 쪽은 건드릴 필요가 없다. OrderStatus 클래스에 각 상태에 따른 예외 처리를 넣어놨기 때문이다. 중복이 되는 부분은 따로 빼서 메서드로 만들고, order를 받아 조건이 맞으면 해당 order의 상태를 수정하는 방식이다.

@Getter
@RequiredArgsConstructor
public enum OrderStatus {

    PENDING("주문 확인전") {
        @Override
        public void update(UserOrderEntity order) {

        }
    },
    CANCELED("주문 취소") {
        @Override
        public void update(UserOrderEntity order) {
            validateOrderCancelable(order);
        }
    },
    ACCEPTED("주문 접수") {
        @Override
        public void update(UserOrderEntity order) {
            validateOrderCancelable(order);
            order.updateOrderStatus(OrderStatus.ACCEPTED);
        }
    },
    REJECTED("주문 거절") {
        @Override
        public void update(UserOrderEntity order) {
            validateOrderCancelable(order);
            order.updateOrderStatus(OrderStatus.REJECTED);
        }
    },
    COOKING("조리중") {
        @Override
        public void update(UserOrderEntity order) {
            validateNotDeliveringOrCompleted(order);
            if (!OrderStatus.ACCEPTED.equals(order.getOrderStatus())) {
                throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_ACCEPTED_BEFORE_COOKING.getMessage());
            }
            order.updateOrderStatus(OrderStatus.COOKING);
        }
    },
    DELIVERING("배달중") {
        @Override
        public void update(UserOrderEntity order) {
            validateNotDeliveringOrCompleted(order);
            if (!OrderStatus.COOKING.equals(order.getOrderStatus())) {
                throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_COOKING_BEFORE_DELIVERY.getMessage());
            }
            order.updateOrderStatus(OrderStatus.DELIVERING);
        }
    },
    COMPLETED("배달 완료") {
        @Override
        public void update(UserOrderEntity order) {
            if (!OrderStatus.DELIVERING.equals(order.getOrderStatus())) {
                throw new InvalidRequestException(ErrorOrderMessage.MUST_BE_DELIVERING_BEFORE_COMPLETE.getMessage());
            }
            order.updateOrderStatus(OrderStatus.COMPLETED);
        }
    };

    private final String description;

    abstract public void update(UserOrderEntity order);

    private static void validateOrderCancelable(UserOrderEntity order) {
        if (!OrderStatus.PENDING.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.CANNOT_CANCEL_ORDER.getMessage());
        }
    }

    private static void validateNotDeliveringOrCompleted(UserOrderEntity order) {
        if (OrderStatus.DELIVERING.equals(order.getOrderStatus())
                || OrderStatus.COMPLETED.equals(order.getOrderStatus())) {
            throw new InvalidRequestException(ErrorOrderMessage.ALREADY_DELIVERING_OR_COMPLETED.getMessage());
        }
    }
}

 

이렇게 리팩토링을 진행하고, 튜터님께 다시 피드백을 받았는데 잘 수정했다고 하셨다. 이 과정을 통해 중복 코드에 대한 경각심을 다시 한 번 일깨웠고, enum 클래스에서 abstract 메서드를 사용하는 방법에 대해 학습해봤다.