[🚀 사이클2 - 미션 (예약 변경/취소와 에러 처리)] 영기(김기영) 미션 제출합니다. by Giyoul · Pull Request #430 · woowacourse/spring-roomescape-member

Giyoul · 2026-05-13T08:44:21Z

인사말

아루 안녕하세요!
빠르게 코드를 가지고 돌아왔습니다!
cycle1 마지막에 주신 피드백 내용과 제가 다음 사이클로 미루겠다고 말씀드렸던 리팩토링 사항들 또한 전부 적용해서 가져왔으니, 확인해주시면 감사드리겠습니다!
잘 부탁드립니다 :)

체크 리스트

미션의 필수 요구사항을 모두 구현했나요?
Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
애플리케이션이 정상적으로 실행되나요?

베이스 코드 선택 체크

이전 미션의 내 코드에서 시작
이전 미션의 페어의 코드에서 시작

어떤 부분에 집중하여 리뷰해야 할까요?

래퍼 DTO 설계 방식에 대한 고민 질문

현재 각 응답마다 별도 클래스를 두는 방식으로 구현했는데, 다른 방식과 비교해 어떤 방향이 더 적절한지 여쭤보고 싶습니다.

방식 1. 별도 클래스 (현재)

public record ReservationsResponse(List reservations) {}
public record ReservationTimesResponse(List times) {}
{"reservations": [...]}
{"times": [...]}

방식 2. 제네릭 공통 클래스

public record ListResponse(List items) {}
{"items": [...]} // 필드명이 획일화됨

방식 3. Inner class로 묶기

public class CollectionResponse {
  public record Reservations(List reservations) {}
  public record Times(List times) {}
}

방식 1은 JSON 필드명이 명확하고 응답별 확장이 자유롭지만 파일이 많아지고, 방식 2는 코드가 줄지만 필드명 표현력이 떨어지며, 방식 3은 파일 수를 줄이면서 필드명은 유지할 수 있지만 타입명이 길어지는 트레이드오프가 있습니다.
이번 미션에서 저는 1번 방식을 선택했고, 평소에 프로젝트를 할 때는 3번 방식을 선호하는 편입니다.
이 부분은 항상 고민을 하지만, 정답이 없다고 느껴져서 어려운 것 같습니다.
물론 이 부분이 팀 내 컨벤션일 것 같긴 한데, 아루는 개인 개발하실떄 어느 방향을 선호하시는지 궁금해지는 것 같습니다!

확장성을 위해 List를 직접 반환하던 모든 엔드포인트를 래퍼 객체로 감싸도록 수정 - AdminReservationsResponse, ReservationsResponse - ReservationTimesResponse, ThemesResponse - AvailableReservationTimesResponse

- reservation 테이블에 created_at 컬럼 추가 - Reservation.create()로 신규 생성 시 과거 날짜 검증 - Reservation.restore()로 DB 복원 시 검증 없이 재구성 - ReservationDao.save()가 Reservation을 받아 ID 포함 Reservation 반환 - 서비스에서 검증용 객체를 따로 생성하던 구문 제거

- DAO 테스트를 EmbeddedDatabaseBuilder로 전환 — Spring Boot 컨텍스트 제거 - DaoTest 추상 클래스 삭제 - Tautological test 제거 (Mock 출력을 그대로 검증하는 서비스 테스트) - data.sql 하드코딩 의존 테스트 개선 (isNotEmpty, before-1 패턴) - ThemeControllerTest 잘못된 엔드포인트 수정 (/themes → /themes/popular)

@Valid

- 날짜+시간 기준 과거 예약 불가 (LocalDateTime 비교로 변경) - created_at 컬럼 DATE → TIMESTAMP로 변경 - 이미 지난 예약 취소 불가 - @Valid 적용 — ReservationRequest, ReservationTimeRequest, ThemeRequest - ReservationDao.findById() 추가

- ErrorResponse(code, message) DTO 추가 - GlobalExceptionHandler 보강 - MethodArgumentNotValidException → 400 INVALID_INPUT - HttpMessageNotReadableException → 400 INVALID_FORMAT - NoResourceFoundException → 404 NOT_FOUND - Exception fallback → 500 SERVER_ERROR

- ReservationUpdateRequest(date, timeId) DTO 추가 - ReservationDao.update() 추가 - ReservationService.update() — 존재 여부, 과거 날짜, 중복 예약 검증

- PastReservationException → 422 (과거 날짜 예약/취소/변경) - ReservationConflictException → 409 (중복 예약) - GlobalExceptionHandler에 422, 409 핸들러 추가

- 예약 목록에 변경 버튼 추가 - 날짜·시간 선택 모달 구현 - PATCH /reservations/{id} 호출 - code 기반 사용자 친화적 에러 메시지 처리 - 삭제 에러도 code 기반으로 개선

donghoony

영기 안녕하세요~ 이번 사이클 빠르게 진행해주신 덕분에 재미있는 이야기를 많이 나눠볼 수 있을 것 같네요 😎 개발자마다 성향이 다르겠지만, 저는 객체지향으로부터 뻗어나가는 설계를 좋아하는 편이라 몇 가지 코멘트를 남겨보았어요. 다양한 관점을 살펴보고 이야기하는 시간이 되었으면 좋겠습니다.

요구사항은 전체적으로 잘 만족하고 있고, 몇 가지 보완할 부분에 대해서 리뷰를 남겨두었으니 코멘트 확인해주세요~

donghoony · 2026-05-13T15:27:51Z

+import java.util.List;
+
+public record AdminReservationsResponse(List<AdminReservationResponse> reservations) {
+}


이 모양이 어떤 것을 의미하는지 알아보고, 전체적으로 적용해볼까요?

POSIX표준이고, cat 같은 명령어 쳤을때에 파일이 붙어버리는 문제가 발생할 수 있습니다.
이 부분은 전 리뷰어 분들한테도 공통적으로 지적받았던 부분이고, 그래서 EOF에 엔터 삽입 intellij 자동기능도 설정해두었는데, 뭔가 충돌이 있는지 자꾸 지워지네요ㅠㅠ
피알 날리기 전에도 마지막으로 모든 파일 들어가서 option + command + L로 파일 포멧 정리도 하고 EOF에 엔터 안쳐져있으면 수동으로 하나하나 추가했는데, 어김없이 지워져 있네요...

일단 파일 다 확인하고 수동으로 엔터 다시 추가하도록 수정해보겠습니다...

donghoony · 2026-05-13T15:30:27Z

@@ -0,0 +1,7 @@
+package roomescape.exception;
+
+public class PastReservationException extends RuntimeException {


이 예외는 어디에서 발생하나요? Dao에서도, Controller에서 발생해도 어색하지 않은 패키지 위치로 보여요. 발생하는 곳에서 예외를 정의하면 어떨까요?

매번 패키징이 어려운 것 같네요..
PastReservationException은 Reservation 도메인 객체에서, ReservationConflictException은 ReservationService에서 발생합니다.
말씀대로 각각 domain과 service 패키지로 이동하면 예외가 발생 맥락과 가까워지고 별도의 exception 패키지도 제거할 수 있겠네요.
수정하겠습니다.

donghoony · 2026-05-13T15:31:55Z

+
+    @Transactional
+    public Reservation update(long id, LocalDate date, long timeId) {
+        Reservation reservation = reservationDao.findById(id)


여기에는 final이 붙지 않았네요? 이유가 있었나요? 메서드 내에서 변수를 재할당하지 않는다는 컨벤션을 모두가 공유하고 있다면 제거하는 방향으로 통일하는 것도 좋은 방법 중 하나라고 생각해요~

음... 이건 그저 '내 코드 구려 병'이 도져서 생긴 문제인 듯 합니다..
필드는 final 유지, 로컬 변수는 제거로 구분하겠습니다.

donghoony · 2026-05-13T15:34:34Z

+        if (LocalDateTime.of(date, newTime.getStartAt()).isBefore(LocalDateTime.now())) {
+            throw new PastReservationException("과거 날짜로는 예약할 수 없습니다.");
+        }


현재 Controller-Service-Dao의 계층으로 나눠 주셔서, 도메인이라는 개념이 조금 얇아질 수 있습니다. 도메인 지식과 계층 구조는 서로 상반되는 것이 아니라서 함께 어우러질 수 있는데요.

과거 날짜로 예약할 수 없다는 예약에게 있어서 절대적인 불변식에 해당하지 않을까요?

만약 그렇다면, 현재 시간이 예약에 존재하지 않으니 Service에서 진행하는 것은 자연스러워 보입니다.

다만, 예약에게는 예약 시각뿐만 아니라, 예약을 진행한 시각도 중요한 정보 아닐까요?

검증을 서비스가 아닌 도메인단으로 이동하면 어떤 점이 이점으로 다가올지 고민해보셔도 좋겠습니다. (예약 시간, 예약에 대한 검증의 경우에는 도메인 혼자서는 해결할 수 없으므로 Service 계층의 도움을 받아야 하겠습니다)

save()에서는 Reservation.create()가 과거 날짜를 검증하고 있어서 도메인이 제 역할을 하고 있는데, update()와 delete()에서는 동일한 불변식을 서비스가 직접 체크하고 있었습니다.
이거를 도메인 책임이 일관되지 않게 서비스로 새어나간거로 볼 수 있겠네요

그러면 Reservation에 withUpdated(date, newTime, now)와 validateCancellable(now) 같은 메서드를 추가해서 과거 날짜 검증을 도메인으로 이동하겠습니다.
충돌 체크는 DB 조회가 필요하니 서비스에 유지하고, 나머지 서비스는 순수 오케스트레이션만 담당하는 방향으로 수정하겠습니다.

약간 굳이 한 계층 더 들어가는 느낌이라 이렇게 하지 않았었는데, 책임 분리와 중복 코드 제거의 관점에서 생각해보니 이게 더 좋게 느껴지는 것 같네요

donghoony · 2026-05-13T15:44:28Z

+        if (reservationDao.existsByDateAndTimeIdAndThemeId(date, timeId, reservation.getTheme().getId())) {
+            throw new ReservationConflictException("이미 예약된 시간입니다.");
+        }


(이전 코멘트와 바로 위 코멘트로부터 이어집니다!)

만약 이번 미션이 블랙잭과 같은 콘솔 기반의 객체지향 미션이었다면, 그리고 DB 연결 또한 없었더라면 객체 설계를 어떻게 진행하셨을 것 같나요?

예를 들어보자면, 중복 예약에 대한 검증을 위해 테마의 하루 예약을 관리하는 일급 컬렉션이 등장할 수 있습니다.

Dao의 상호작용은 DB라는 저수준에 종속되는 느낌이 듭니다. 테이블의 언어로 상호작용하는 것처럼 보이죠. 영기가 Repository와 Dao를 구분해주신 만큼 Repository에서는 해당 도메인을 영속화하는 과정이 담길 수 있어요. 그렇다면 서비스에서는 해당 일급 컬렉션으로 소통하고, 저장하는 일련의 과정을 만들어낼 수도 있습니다. 이를 저장하기 위해서 DB 컬럼을 일치시킬 수도 있지만, 성능상 이를 포기하는 경우도 있고요. (이 경우 매핑의 책임을 고민해보는 것도 좋은 공부가 됩니다)

물론 구현해주신 방향도 요구사항을 충분히 만족하지만, 도메인을 탐구하고 객체의 책임을 도출해내는 과정으로부터 설계가 진행된다면 더 단단한 코드를 작성할 수 있다고 생각해요! 반영하시지 않아도 되고 하나의 관점으로 바라봐주셨으면 좋겠습니다 🙂

"도메인을 먼저 탐구하고 객체의 책임을 도출한 뒤 설계"라는 말씀이 생각의 여지를 많이 던져주는 것 같습니다...!
생각해보니 DB 구조가 도메인 설계를 주도하고 있었던 것 같습니다.

DB 없이 설계한다면 ThemeDaySchedule 같은 일급 컬렉션이 중복 예약 검증의 책임을 가져갈 수 있겠네요.
현재는 DB 조회(existsByDateAndTimeIdAndThemeId)가 그 역할을 대신하다 보니 도메인이 얇아진 상황이었습니다.

Repository가 도메인 객체와 DB 컬럼 사이의 매핑을 담당하고, Service는 일급 컬렉션과 도메인 언어로 소통하는 구조가 더 단단한 설계임을 이해했습니다!

donghoony · 2026-05-13T15:59:26Z

 @RestControllerAdvice
 public class GlobalExceptionHandler {

    @ExceptionHandler(IllegalArgumentException.class)


IllegalArgumentException을 400으로 처리하고, e.getMessage()를 노출해도 괜찮을까요? IllegalArgumentException이 의도치 않은 다른 곳에서 발생할 위험은 없을까요?

IllegalArgumentException을 광범위하게 잡으면 라이브러리나 프레임워크 내부에서 발생한 예외까지 400으로 처리되고 내부 메시지가 노출될 위험이 있겠네요.
도메인 전용 커스텀 예외(InvalidInputException 등)를 만들어서 우리 코드에서 의도적으로 던지는 경우만 잡도록 분리하겠습니다!

donghoony · 2026-05-13T16:02:43Z

dto 클래스가 많아지면서 담고있는 내용이 커지고 있네요. 패키지를 적절하게 나눠보면 어떨까요?

현재 controller.dto 하나에 Reservation, Theme, ReservationTime 관련 DTO가 15개 모여있는 상황이라 도메인별 서브 패키지로 분리하는 방향이 적합하다고 생각했습니다.
controller/dto/reservation/, controller/dto/time/, controller/dto/theme/ 형태로 나누면 각 도메인별로 Request/Response를 한 눈에 볼 수 있고, 기존 domain/ 패키지 구분 기준과도 일관성을 유지할 수 있을 것 같습니다.
혹시 생각하신 다른 기준이 있으시면 말씀해 주시면 반영하겠습니다.

한 가지 걸리는 점은, 현재 프로젝트 구조를 보면:

roomescape/ controller/ ← 계층 dao/ ← 계층 domain/ ← 계층 exception/ ← 계층 service/ ← 계층

전체가 계층 기반(horizontal slicing) 으로 나뉘어 있습니다.
여기서 dto/ 안에서만 도메인 기준을 섞으면 일관성이 깨집니다.
dao/나 service/는 도메인 sub-package 없이 flat한데 dto/만 domain-based가 됩니다.

그래서, 현재의 철학을 따르려면 Request / Response 분리하는게 맞아 보이는데,

controller/dto/ request/ ← 입력 response/ ← 출력

아루는 dto/ 이 아래에 도메인별로 패키지를 나누는 것과, 위에서 말한 것 중에 어느게 더 괜찮은 방법이라고 보시나요?

donghoony · 2026-05-13T16:03:24Z

    id       BIGINT NOT NULL AUTO_INCREMENT,
    start_at TIME   NOT NULL UNIQUE,
    PRIMARY KEY (id)


여러 start_time이 DB상으로 존재할 수 있을 것 같네요.

방이 하나인 방탈출 카페 맥락이라면 UNIQUE (date, time_id)로 변경해야 시간당 한 팀만 예약이 가능해질 것 같습니다.
서비스의 중복 체크 로직도 함께 수정하겠습니다

donghoony · 2026-05-13T16:04:15Z

+### 에러 응답 형식
+
+```json
+{ "code": "RESERVATION_CONFLICT", "message": "이미 예약된 시간입니다." }


code가 필요한 이유는 무엇인가요? 실제로 프론트에서 해당 code를 보고 분기가 이뤄지나요? 서버에서 불필요한 정보를 내려주고 있는 것은 아닐까요?

user.html에서 code 필드를 실제로 분기에 사용하고 있었습니다.
취소·변경 API 응답에서 PAST_RESERVATION과 RESERVATION_CONFLICT를 구분해 각각 다른 안내 메시지를 표시하는 용도입니다.

다만 프론트가 code에 의존하는 대신 HTTP 상태 코드(422, 409)만으로 분기할 수 있도록 리팩토링하는 것도 가능하고, 그렇다면 code 필드는 제거할 수 있을 것 같습니다.
현재 구조를 유지할지 의견 주시면 반영하겠습니다!

donghoony · 2026-05-13T16:06:26Z

+        Reservation reservation = reservationDao.findById(id)
+                .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 예약입니다."));
+        if (LocalDateTime.of(reservation.getDate(), reservation.getTime().getStartAt()).isBefore(LocalDateTime.now())) {
+            throw new PastReservationException("이미 지난 예약은 취소할 수 없습니다.");


어떤 경우에 기본 예외를 사용하고, 어떤 경우에는 만들어주신 커스텀 예외를 사용하셨나요?

돌아보니 명확한 기준 없이, HTTP 상태 코드가 400이 아닐 때만 커스텀 예외를 만드는 패턴이었습니다.
의도한 설계가 아니었습니다.

더 나은 기준은 "도메인 규칙 위반은 모두 커스텀 예외, IllegalArgumentException은 프로그래밍 오류에만"이라고 생각합니다.
"존재하지 않는 예약입니다" 같은 케이스도 ReservationNotFoundException 같은 도메인 예외로 표현하는 게 맞고, IllegalArgumentException은 예상치 못한 상황에만 남겨두는 방향으로 정리하겠습니다.

Giyoul added 13 commits May 12, 2026 15:18

docs: cycle2 리팩토링 목록 추가

64efbed

refactor: update front api call format

da03c86

docs: cycle 2 : 구현 기능 목록 추가

ddd9b12

docs: update cycle2 step2 checklist

a2c199a

feat(reservation): add PATCH /reservations/{id} for date and time update

594586d

- ReservationUpdateRequest(date, timeId) DTO 추가 - ReservationDao.update() 추가 - ReservationService.update() — 존재 여부, 과거 날짜, 중복 예약 검증

feat(error): apply correct HTTP status codes with custom exceptions

cd4f973

- PastReservationException → 422 (과거 날짜 예약/취소/변경) - ReservationConflictException → 409 (중복 예약) - GlobalExceptionHandler에 422, 409 핸들러 추가

feat(ui): add reservation update modal in user.html

b201a7a

- 예약 목록에 변경 버튼 추가 - 날짜·시간 선택 모달 구현 - PATCH /reservations/{id} 호출 - code 기반 사용자 친화적 에러 메시지 처리 - 삭제 에러도 code 기반으로 개선

docs: 예약 변경 화면 업데이트

80a7dfe

donghoony requested changes May 13, 2026

View reviewed changes

		@@ -0,0 +1,7 @@
		package roomescape.exception;

		public class PastReservationException extends RuntimeException {

Conversation

Giyoul commented May 13, 2026

인사말

체크 리스트

베이스 코드 선택 체크

어떤 부분에 집중하여 리뷰해야 할까요?

래퍼 DTO 설계 방식에 대한 고민 질문

Uh oh!

donghoony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants