간단한 조회 서비스 로직 리팩토링

배경

오늘 함께 일하시는 FE 분께서 API 응답 데이터가 요구사항과 다르다고 확인을 부탁하셨습니다.

 

요구사항 자체는 간단했습니다. 주문 상태가 WAIT 인 데이터가 포함되면 안되었기에 해당 부분만 필터링을 추가하면 되는 작업이었습니다.

해당 기능에 대한 코드입니다.

// Before
public List<OrderModel> getOrderModels(List<Market> markets, boolean urgent) {
    Map<Long, Market> marketsById = markets.stream()
                                           .collect(Collectors.toMap(Market::getId, Function.identity()));

    List<Order> orders = getWaitingOrders(marketsById.keySet());

    List<OrderModel> OrderModels = Lists.newArrayList();
    for (Order order : orders) {
        Long ownerId = marketsById.get(order.getMarketId()).getOwnerId();
        OrderModels.add(new OrderModel(order.getMarketId(),
                                       order.getId(),
                                       order.getProduct(),
                                       order.getCreatedDateTime(),
                                       order.getPlace(),
                                       order.getStatus(),
                                       order.getMemberId(),
                                       ownerId));
    }

    if (urgent) { // here
        OrderModels.removeIf(order -> order.getStatus() != OrderStatus.WAIT);
    }

    return OrderModels.stream()
                      .sorted(Comparator.comparing(OrderModel::getId))
                      .limit(READ_SIZE)
                      .collect(Collectors.toList());
}

private List<Order> getWaitingOrders(Collection<Long> marketIds) {
    return orderRepository.findWatingOrdersByMarketIds(marketIds);
}

코드를 보시면 if (urgent) {} 부분에서는 WAIT 상태인 데이터를 필터링하고 있으나, 그 외 상황에서는 WAIT 상태 데이터가 포함되고 있었습니다.

 

해당 부분을 간단히 수정할 수 있었지만, 전반적으로 가독성을 개선하고, Order → OrderModel 로의 매핑과 정렬 작업이 함께 이루어지면 좋을 것 같다는 생각이 들었습니다.

그래서 OrderStatus 필터링에 대한 조건을 메서드화 하고, 매핑과 정렬 작업을 하나의 Stream으로 작성하였습니다.

// After
public List<OrderModel> getNotAcceptedOrderModels(List<Market> markets, boolean urgent) {
    Map<Long, Market> marketsById = markets.stream()
                                           .collect(Collectors.toMap(Market::getId, Function.identity()));

    List<Order> orders = getWaitingOrders(marketsById.keySet());

    return orders.stream()
                 .map(order -> new OrderModel(order.getMarketId(),
                                              order.getId(),
                                              order.getProduct(),
                                              order.getCreatedDateTime(),
                                              order.getPlace(),
                                              order.getStatus(),
                                              order.getMemberId(),
                                              marketsById.containsKey(order.getMarketId()) ?
                                              marketsById.get(order.getMarketId()).getOwnerId() :
                                              null))
                 .filter(order -> inTargetStatus(order, urgent))
                 .sorted(Comparator.comparing(OrderModel::getId))
                 .limit(READ_SIZE)
                 .collect(Collectors.toList());
}

private List<Order> getWaitingOrders(Collection<Long> marketIds) {
    return orderRepository.findWatingOrdersByMarketIds(marketIds);
}

private boolean inTargetStatus(OrderModel order, boolean urgent) {
    if (urgent) {
        return order.getStatus() == OrderStatus.TIMEOUT;
    }
    return order.getStatus() != OrderStatus.ACCEPTED;
}

리팩토링 후 코드입니다.

 

Stream이 너무 길어져도 좋지 않을 수 있지만, 반복문이나 Stream이 파편화 되어있으면 관리하기가 어려울 수 있을 것 같습니다.

이를 이용하면 어느 정도는 가독성이 향상될 수 있다고 생각합니다 :)