[1단계 방탈출 결제 / 배포] 에버(손채영) 미션 제출합니다.

[helenason]

안녕하세요 스티치! 에버입니다.

방탈출 결제 미션 구현 후 리뷰 요청 드립니다.

웹 개발 경험

Spring과 Flask로 오래 전 CRUD 구현 후 배포한 경험이 있습니다.
카카오페이 결제 API를 연동한 경험 또한 있습니다.
하지만 구현하기에 급급하였기에 기초가 부실한 상태로 진행하였어요.

따라서 기초를 튼튼히 다지기 위해 노력하는 중입니다. 참고해주시면 감사하겠습니다!

질문 사항

@Transactional
public ReservationResponse saveReservation(ReservationRequest reservationRequest) {
    return saveReservationWithPayment(reservationRequest, freePayment());
}

private Consumer<ReservationRequest> freePayment() {
    return request -> {};
}

@Transactional
public ReservationResponse saveReservation(ReservationPaymentRequest reservationPaymentRequest) {
    return saveReservationWithPayment(reservationPaymentRequest.toReservationRequest(), reservationRequest -> {
        PaymentRequest paymentRequest = reservationPaymentRequest.toPaymentRequest();
        paymentClient.requestApproval(paymentRequest);
    });
}

private ReservationResponse saveReservationWithPayment(ReservationRequest reservationRequest, Consumer<ReservationRequest> payment) {
    Member member = findMemberById(reservationRequest.memberId());
    ReservationSlot slot = reservationSlotService.findSlot(reservationRequest.toSlotRequest());
    Optional<Reservation> optionalReservation = reservationRepository.findBySlot(slot);

    if (optionalReservation.isPresent()) {
        Reservation reservation = optionalReservation.get();
        Waiting waiting = reservation.addWaiting(member);

        waitingRepository.save(waiting);
        return ReservationResponse.createByWaiting(waiting);
    }

    payment.accept(reservationRequest);

    Reservation savedReservation = reservationRepository.save(new Reservation(member, slot));
    return ReservationResponse.createByReservation(savedReservation);
}

위 코드는 ReservationService 클래스의 코드 일부입니다.

결제 로직이 추가되는 과정에서, 기존 어드민과 유저의 예약 저장을 함께 처리하던 코드에서, 어드민의 예약 저장과 유저의 예약 저장 메서드를 분리하게 되었습니다. 결국, 어드민이 예약을 저장하는 로직과 유저가 예약을 저장하는 로직은 결제 로직을 제외한 모든 코드가 중복됩니다.

해당 코드에서 중복을 제거하기 위해 saveReservationWithPayment 메서드에서 함수형 인터페이스를 payment라는 파라미터로 전달받았습니다. 위 방식이 오직 한 부분을 위한 복잡한 설계라고 보시나요? 스티치의 생각이 궁금해요!

이번 미션 구현 코드

링크

테스트 계정

어드민 계정

ID : admin@admin.com
PW : 1234

유저 계정

ID : user1@user.com
PW : 1234

잘 부탁드립니다 🙇‍♀️

[lxxjn0]

안녕하세요 에버, 이번에 리뷰를 맡게 된 스티치입니다.

에버의 이전 경력이 돋보이는 코드였던 것 같아요! 저도 현재 결제팀에 있는데 저희 팀에서 자주 본 코드 양식들이 있어서 놀랍고도 새로웠습니다 :) 먼저 프로덕션 코드 위주로 피드백 작성해봤는데 한번 확인해보시고 리뷰 반영해주세요.

추가로 함수형 인터페이스로 분리하신 시도는 괜찮아보입니다 👍🏼 메서드로 분리하는 형식을 통해서 이름을 부여해준다면 더욱 가독성을 높일 수 있을 것 같습니다. 한번 참고해주세요.

그리고 기존의 연동 경험들이 보여서 조금 더 어려운 요구사항을 한번 제안드려봤는데 한번 참고해서 개발해보시면 좋을 것 같아요 :)

[helenason]

안녕하세요 스티치!

리뷰 반영하여 요청 드립니다.

이번 리뷰도 잘 부탁드립니다 감사합니다! 🙇‍♀️

[lxxjn0]

피드백 반영하시느라 수고하셨습니다! 기존의 리뷰에 대해서도 추가 코멘트를 간단하게 달아둔 내용도 있으니 함께 확인 부탁드릴게요 👍🏼

[lxxjn0]

파라미터로 함수형 인터페이스를 사용하신 이유가 있을까요? 사실 기존의 코드에서 중복되는 부분을 단순히 메서드 분리만 해도 될 것 같은데 다른 의도가 있으신 건지 궁금합니다.

[helenason]

위 코드는 아래 두 메서드의 중복을 제거하기 위함이었는데요.

private ReservationResponse saveReservation(ReservationRequest reservationRequest, Runnable paymentStrategy) {
    Member member = findMemberById(reservationRequest.memberId());
    ReservationSlot slot = reservationSlotService.findSlot(reservationRequest.toSlotRequest());
    Optional<Reservation> optionalReservation = reservationRepository.findBySlot(slot);
    if (optionalReservation.isPresent()) {
        Reservation reservation = optionalReservation.get();
        Waiting waiting = reservation.addWaiting(member);
        waitingRepository.save(waiting);
        return ReservationResponse.createByWaiting(waiting);
    }
    paymentStrategy.run();
    Reservation savedReservation = reservationRepository.save(new Reservation(member, slot));
    return ReservationResponse.createByReservation(savedReservation);
}

public ReservationResponse saveWaiting(WaitingSaveRequest waitingSaveRequest) {
    Member member = findMemberById(waitingSaveRequest.memberId());
    ReservationSlot slot = reservationSlotService.findSlot(waitingSaveRequest.toSlotRequest());
    Optional<Reservation> optionalReservation = reservationRepository.findBySlot(slot);
    if (optionalReservation.isPresent()) {
        Reservation reservation = optionalReservation.get();
        Waiting waiting = reservation.addWaiting(member);
        waitingRepository.save(waiting);
        return ReservationResponse.createByWaiting(waiting);
    }
    throw new RoomEscapeBusinessException("해당 대기에 대한 예약이 존재하지 않습니다.");
}

중복 코드에 대해서, 반환해야 하는 값들이 많았고, Optional과도 엮어있기 때문에 단순히 메서드로 분리하였을 때에는 메서드가 깔끔하게 추출되지 않았습니다. 깔끔히 추출할 수 있는 부분은 아래 코드 정도였습니다.

Waiting waiting = reservation.addWaiting(member);
waitingRepository.save(waiting);
return ReservationResponse.createByWaiting(waiting);

하지만 이 외 모든 코드는 여전히 중복이었기 때문에 위 세 줄을 메서드로 추출하는 방법보다는 함수형 인터페이스를 사용하는 방향이 낫다고 판단하였습니다!

private ReservationResponse trySave(Member member,
                                    ReservationSlot slot,
                                    Supplier<ReservationResponse> progressWhenReservationNotExist) {
    Optional<Reservation> optionalReservation = reservationRepository.findBySlot(slot);
    if (optionalReservation.isPresent()) {
        Reservation reservation = optionalReservation.get();
        Waiting waiting = reservation.addWaiting(member);
        waitingRepository.save(waiting);
        return ReservationResponse.createByWaiting(waiting);
    }
    return progressWhenReservationNotExist.get();
}

따라서 두 메서드의 차이점인, 예약이 존재하지 않을 때의 로직을 함수형 인터페이스로 주입하였습니다.

하지만 제가 발견하지 못한 더 좋은 메서드 추출 방법이 있을 수 있기 때문에, 발견하셨을 때 언급 주시면 감사하겠습니다! 🥹

[lxxjn0]

해당 부분은 제가 조금 더 코드를 보고 다시 코멘트 남겨두도록 할게요!! 지금의 코드가 잘못된 것은 아니기에 그대로 유지해도 괜찮을 것 같아요! 보다 좋은 메서드 분리가 가능한지는 제가 추가로 코드 읽어보면서 찾아보도록 할게요 :)

[helenason]

스티치의 코멘트 반영하여 리뷰 재요청 드립니다!

이번 리뷰도 잘 부탁드립니다. 감사합니다 😁 👍

[lxxjn0]

에버, 피드백 반영하시느라 수고하셨습니다! 코드를 멋지게 잘 수정해주셔서 다음 단계로 진행해도 좋을 것 같습니다 :)

기존 피드백에 대한 코멘트 추가로 남겨뒀으니 한번 확인만 부탁드리겠습니다 👍🏼 그럼 다음 단계 리뷰에서 만날게요.