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

mvg01 · 2026-05-13T11:17:29Z

개요

안녕하세요 기론, 리뷰이 무빙입니다.

이번에도 리뷰 핑퐁 기간을 충분히 확보하려고 "일단 동작하는 구현"을 우선해서 빠르게 PR을 올렸습니다. 그러다 보니 이전 미션보다 코드 일관성이 떨어지는 부분이 있을 것 같습니다.

이번 리뷰의 목표는, 다음 미션에서 페어가 제 코드를 읽고 의도를 빠르게 파악할 수 있는 수준까지 끌어올리는 것입니다. 개념적으로 궁금한 부분은 아래에, 코드 단위에서 궁금한 부분은 인라인 코멘트로 남겨두었습니다.

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

체크 리스트

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

베이스 코드 선택 체크

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

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

1. mockData.sql에 대한 테스트 의존성

진행하다 보니 컨트롤러 테스트들이 mockData.sql에 강하게 의존하게 된다는 걸 느꼈습니다. 다양한 케이스를 테스트하기 위해 mockData.sql에 데이터를 추가하면, 그때마다 기존 테스트들이 깨지곤 했습니다. mockData.sql을 수정하는 게 두려워질 정도였습니다. 이 데이터가 바뀌어도 테스트가 자연스럽게 따라갈 수 있는 방법이 있을까요? 이 불편함을 해결하는 방법론이나 도구가 있을 것 같은데, 지금 시점에선 잘 모르겠습니다.

2. ErrorCode Enum 활용

토론 조원들과 함께 예외 응답에 담길 내용을 정의했고, 아래 형식을 따랐습니다.

에러 응답 형식 (토론 그룹에서 정한 형식)

{
  "code": "에러_코드",
  "path": "/요청_URI",
  "message": "사용자에게 보여줄 메시지",
  "action": "해결 방법 안내 (없으면 null)"
}

이 판단이 정답인지 아닌지는 고려하지 않았고, 각자의 값이 필요하다고 생각한 이유는 다음과 같습니다.

code: 프론트에서 예외를 쉽게 분기할 수 있게 하기 위해서 제공합니다.
path: 예외가 발생한 URI
message: 프론트랑 클라이언트에게 자연어로 전달할 상세한 설명
action: 그래서 이 에러를 해결하기 위해 프론트 혹은 클라이언트가 무엇을 해야할지에 대한 설명 (nullable)

그리고 코드를 작성하기 전에 직접 이런식으로 요청에 대하여 발생할 수 있는 예외 상황을 미리 정리해봤습니다.

코드 작성 전에 발생 가능한 예외 상황을 미리 정리해두었고, 해당 내용은 study-log/error-design.md에 기록해두었습니다.
- 이런 방식으로 에러 응답을 통일하는 게 적절한지 궁금합니다. 어떤 부분이 부족하거나 과한지 피드백 부탁드립니다.
구현하면서 느낀 불편한 점도 있었습니다. ErrorCode Enum에 미리 선언해야 하는 항목이 많아질수록 클래스 자체의 가독성이 떨어진다고 느꼈습니다. 커스텀 예외 구조도 제 판단대로 자유롭게 설계한 것이라 적절한지 확신이 없습니다.
- ErrorCode를 Enum으로 관리하는 것보다 더 나은 구조가 있다면 알고 싶습니다.

- 삭제에서 없는 ID 입력 시 예외처리 제거

- reservaiton 테이블 삽입 후 도메인을 반환하기 위해 join하는 과정을 제거함. - ReservationService에서 다른 Service를 의존하도록 변경함. - ReservationRepository에 다른 service의 response 값을 전달함.

- 예약 테마 삭제 시 CASCADE - 예약 시간 삭제 시 RESTRICT

- 이미 존재하는 시간에 대해서도 커스텀 에러 응답 반환

- themes/ranks limit 범위 검증

- 기존 ResponseEntityExceptionHandler 상속 제거

- GET /themes/ranks 의 limit 누락 테스트 추가

- 이미 지난 시간의 예약 수정 방지 - 이미 지난 시간으로의 예약 수정 방지

Gyuchool

안녕하세요 무빙!
빠르게 구현해주셨네요👍 다만, 내 예약을 삭제했는데 예약관리에는 그대로 있는데 이부분 확인부탁드릴께요!

마지막 사이클2도 파이팅입니다💪

몇 가지 코멘트를 남겨두었으니 반영 및 의견 주시면 좋을 것 같습니다.🙏
궁금한 점은 DM이나 PR 커멘트로 언제든지 여쭤봐주세요!

mockData.sql에 대한 테스트 의존성
진행하다 보니 컨트롤러 테스트들이 mockData.sql에 강하게 의존하게 된다는 걸 느꼈습니다.

의존성이 강해지면 어떻게 해결할 수 있을까요?🙄 의존성을 끊어내볼까요?

ErrorCode Enum 활용
이런 방식으로 에러 응답을 통일하는 게 적절한지 궁금합니다. 어떤 부분이 부족하거나 과한지 피드백 부탁드립니다.
ErrorCode를 Enum으로 관리하는 것보다 더 나은 구조가 있다면 알고 싶습니다.

자세한 에러 설명을 함께 제공해주신 점이 좋네요 👍

다만 한 가지 더 고민해볼 부분은, 사용자에게 구체적인 예외 내용을 어디까지 노출할 것인가라고 생각해요. 예를 들어 로그인 기능에서 "비밀번호가 틀렸습니다" 또는 "비밀번호는 8자 이상이어야 합니다"와 같이 상세한 정보를 제공하면, 사용자에게는 친절할 수 있지만 공격자에게도 유용한 힌트를 줄 수 있다고 생각해서요.

따라서 에러 메시지를 누구에게 제공하는지, 그리고 디버깅을 위한 것인지 UX 개선을 위한 것인지 먼저 명확히 하는 것이 중요하다고 생각해요. 이를 바탕으로 사용자 편의성과 보안 사이에서 적절한 수준의 정보를 제공하는 방향으로 고민해보면 좋을것 같네요😊

또한 ErrorCode enum에 모든 종류의 예외를 한곳에 모으기 시작하면 빠르게 무거워질 수 있습니다. 만약 ErrorCode를 모아두기로 한다면 개인적으로는 비즈니스 로직과 관련된 핵심 예외부터 enum으로 관리할 것 같네요.

Gyuchool · 2026-05-13T15:03:22Z

        this.date = date;
        this.time = time;
        this.theme = theme;
+        validateName(name);


validate를 맨아래로 내린 이유가 있을까요?

이전 사이클에서 했던 실수의 잔재가 남아있었네요!! 반영하겠습니다.

Gyuchool · 2026-05-13T15:03:45Z

-    private final LocalDate date;
-    private final ReservationTime time;
-    private final Theme theme;
+public record Reservation(Long id, String name, LocalDate date, ReservationTime time, Theme theme) {


record로 바꾼 이유가 있을까요?

객체 자체가 변경될 여지가 없다고 판단해서 입니다. 마찬가지로 ReservationTime도 record인데 서비스 로직이나 생명주기에서 변경될 여지가 없었습니다.

하지만 java record의 의미를 다시 살펴보니 순수하게 데이터를 담는 목적에만 최적화된 클래스라고 합니다. 저는 딱히 이런 의미를 몰랐어서, 단순히 보일러플레이트 코드를 줄이기 위해서 사용했던 것 같습니다.

또한 도메인에 비즈니스 로직도 담겨있으니, record보단 class가 적합하다고 판단됩니다. 수정해서 반영하겠습니다.

Gyuchool · 2026-05-13T15:04:15Z

-    private static void validateName(String name) {
-        if (name.isBlank() || name.length() > MAX_THEME_LENGTH) {


request에서 valid를 제거한 이유가 있을까요?

request에서 validate를 제거한 이유는, DTO는 데이터의 이동/변환 책임만 담당해야 한다고 판단했기 때문입니다.

검증 위치는 두 곳입니다.

Service: 필드 자체가 null인지(입력값이 아예 안 들어온 상태인지)

Model: 값이 있더라도 도메인 규칙상 유효하지 않은 경우(blank, 길이 초과, 입력값 그 자체가 유효한지)

추가로 도메인에서의 검증의 기준은, Repository에서 도메인 객체를 만드는 과정이 있기 때문에 도메인 스스로 자신의 값 경계를 보장해야 한다고 판단했습니다.

Gyuchool · 2026-05-13T15:05:57Z


    public boolean existsByDateAndTimeIdAndThemeId(LocalDate date, Long timeId, Long themeId) {
-        String sql = "SELECT COUNT(*) FROM reservation WHERE date = ? AND time_id = ? AND theme_id = ?";
+        String sql = "SELECT COUNT(id) FROM reservation WHERE date = ? AND time_id = ? AND theme_id = ?";


id가 들어간것과 *가 들어간것의 차이가 있을까요? id를 카운트하도록 바꾼 이유가 있을까요?

이전에 SELECT * 사용에 대한 리뷰의 영향으로 변경하신 것이라면, COUNT(*)에서 사용된 *가 어떤 의미를 가지는지도 한 번 생각해보시면 좋겠습니다.

제가 드린 리뷰의 의도는 모든 SQL에서 *를 사용하면 안 된다는 뜻은 아니었어요! SELECT 와 COUNT()의 *는 역할과 의미가 서로 다르기 때문에 다시 생각해보시죠!

COUNT(*)은 행 개수를 체크하는 의미라 지금 코드에선 사실 문제없이 사용할 순 있습니다. 하지만 COUNT(id)는 id가 null이 아닌 행을 세주는 것이기 때문에 의미가 약간 달랐습니다. 오히려 COUNT(*) 이 유지가 더 나을 것 같아서 그렇게 수정하겠습니다!

Gyuchool · 2026-05-13T15:22:58Z

+    private void reservationRequestDayCheck(LocalDate date, ReservationTime time) {
+        if (date.isBefore(LocalDate.now())) {
+            throw new RoomescapeException(ErrorCode.RESERVATION_WRONG_DATE);
+        }
+        if (date.isEqual(LocalDate.now())) {
+            if (time.startAt().getHour() <= LocalTime.now().getHour()) {
+                throw new RoomescapeException(ErrorCode.RESERVATION_WRONG_TIME);
+            }
+        }
+    }


항상 같은 기준 시점으로 비교된다고 볼 수 있을까요?

예를 들어 날짜를 비교하는 순간과 시간을 비교하는 순간 사이에 시간이 아주 조금이라도 흐른다면, 이 로직은 하나의 동일한 “현재 시각”을 기준으로 판단한다고 말할 수 있을지 고민해보면 좋겠습니다.

Gyuchool · 2026-05-13T15:25:40Z

+    private void reservationRequestValidate(ReservationRequest reservationRequest) {
+        if (reservationRequest.name() == null) {


request에 대한 validation인데 null만 검사하는 이유가 있을까요?

Gyuchool · 2026-05-13T15:28:15Z

    PRIMARY KEY (id),
    FOREIGN KEY (time_id) REFERENCES reservation_time (id),
-    FOREIGN KEY (theme_id) REFERENCES theme (id)
+    FOREIGN KEY (theme_id) REFERENCES theme (id) ON DELETE CASCADE


CASCADE를 거신 이유가 있을까요? 어떤 장단점이 있나요?

Gyuchool · 2026-05-13T15:38:50Z

+                reservation.time(), reservation.theme());
    }

    public boolean existsByDateAndTimeIdAndThemeId(LocalDate date, Long timeId, Long themeId) {


같은 date, timeId, themeId를 가지면 예약을 못하는 로직인거같은데, 지금은 동시에 요청이 들어오면 생성이 가능하겠네요? 어떻게 해결할 수 있을까요?

힌트: 데이터베이스

mvg01 · 2026-05-13T11:21:37Z

+
+        Assertions.assertThatThrownBy(() -> reservationService.register(reservationRequest))
+                .isInstanceOf(RoomescapeException.class);
+    }


여기서 timeId=1L은 오전 10시입니다. 오전 10시 이전에 이 테스트를 돌리면 정상 예약 요청으로 간주되어 테스트가 실패하고, 이후라면 성공합니다. 새벽에 작업해도 실패할 것입니다..

"오늘 날짜이면서 이미 지난 시간에는 예약/수정이 불가능하다"는 케이스를 안정적으로 테스트하려면 어떻게 해야 할지 모르겠습니다. 지금처럼 mockData.sql의 데이터를 활용하는 식으로는 불가능한 것일까요?

- 정각 시간 정책은 도메인 검증보단 서비스 검증이 올바르다고 판단 - 잘못된 값 삽입 시 예외, 조회 시 경고 로그로 분리(예외는 안터짐)

K-iise and others added 30 commits May 4, 2026 14:42

Migration: Initial Commit

6074e26

docs: readme 기능 요구사항 작성

16b922c

feat: 테마 도메인 추가

5521701

feat: 시작 시간 정각 검증 로직 추가

0b0958a

chore: theme 테이블 스키마 추가

378e09f

fix: theme 테이블 스키마 순서 변경

bae9a79

fix: Theme 도메인 id 추가

f8e4cdf

feat: ThemeRequest, ThemeResponse 추가

2c8b138

feat: 테마 추가 API 구현

847261e

feat: 테마 삭제 API 구현

f0f3eed

refactor: 삭제 상태코드 변경

2088e5f

- 삭제에서 없는 ID 입력 시 예외처리 제거

fix: 삭제 http status code를 204로 수정

5d7e07d

test: 예약 정보에 테마 정보를 포함하는 테스트로 수정

be8ef20

feat: 예약 정보에 테마 정보 도입

00ebdc5

feat: ReservationRequest, ReservationResponse 테마 추가

809aaef

feat: Reservation에 테마 정보를 추가

cd6a588

docs: 1단계 스터디 로그 작성

98c9ee9

feat: 특정 테마, 예약 시간 ID를 통한 조회 기능 추가

b0a495c

feat: 예약 추가 기능 수정

e4fa887

- reservaiton 테이블 삽입 후 도메인을 반환하기 위해 join하는 과정을 제거함. - ReservationService에서 다른 Service를 의존하도록 변경함. - ReservationRepository에 다른 service의 response 값을 전달함.

test: RoomescapeApplicationTest에 DirtiesContext 추가

74c43a8

docs: 1단계 스터디 로그 내용 추가

b603458

chore: reservation name을 username으로 변경

57eac43

feat: 테마 id와 날짜를 기반으로 예약 가능한 시간 조회 API 구현

2a76f59

feat: 사용자 이름 검증 기능 추가

6e50f0c

docs: 2단계 스터디 로그 작성

1b4f150

feat: 테마 조회 API 구현

926488d

feat: 인기 테마 조회 API 구현

6ba6843

refactor: API 명세서 추가, 반영

4a852c4

chore: DB 스키마 제약 조건 추가

d182c5c

- 예약 테마 삭제 시 CASCADE - 예약 시간 삭제 시 RESTRICT

test: ThemeRepository 테스트 추가

d21e71d

mvg01 added 25 commits May 12, 2026 17:37

feat: 중복 예약 생성 시 커스텀 에러 응답 반환

019dc16

feat: 존재하지 않는 예약 삭제 시 커스텀 에러 응답 반환

ccbff75

docs: readme 구현 체크리스트 추가

0a5fb55

feat: reservation post 필수 값 누락 커스텀 에러 응답 반환

cdbee16

feat: 존재하지 않는 시간 삭제 시 커스텀 에러 응답 반환

81d4755

feat: 삭제하려는 시간에 예약이 존재하는 경우 커스텀 에러 응답 반환

b012b51

feat: time post 필수 값 누락 커스텀 에러 응답 반환

c1408be

- 이미 존재하는 시간에 대해서도 커스텀 에러 응답 반환

feat: 존재하지 않는 테마 삭제 시 커스텀 에러 응답 반환

09f41b3

feat: theme post 필수 값 누락 커스텀 에러 응답 반환

c1b4976

- themes/ranks limit 범위 검증

refactor: 도메인 모델에서의 예외 처리를 커스텀 예외로 반환

250294d

feat: 글로벌 핸들러에 예상치 못한 예외 처리 (INTERNAL_ERROR) 추가

27ab3e7

feat: 잘못된 입력 형식(HttpMessageNotReadableException) 에러 응답 처리

9757c88

- 기존 ResponseEntityExceptionHandler 상속 제거

feat: 잘못된 입력(시간, 날짜) 형식 예외 처리를 전역 핸들러에서 반환

e2f5c4a

feat: URI에서 쿼리스트링 누락된 경우 커스텀 예외로 반환

f889015

- GET /themes/ranks 의 limit 누락 테스트 추가

feat: 이미 지난 날짜와 시간에 대한 예약 거절을 커스텀 예외로 반환

60516a0

docs: 내 예약 API 기능 명세 작성

ec49f7c

feat: 사용자의 이름으로 예약 조회하는 API 추가

ece116c

feat: 사용자의 예약 수정 API 구현

66834df

- 이미 지난 시간의 예약 수정 방지 - 이미 지난 시간으로의 예약 수정 방지

feat: theme 삭제 시 연관 예약 자동 삭제

3273119

feat: 내 예약 조회/변경/취소 UI 추가 및 에러 응답 메시지 표시 화면 구성

39d7dd1

refactor: 사용하지 않는 메서드, 값 제거

5077b5c

refactor: 사용하지 않는 필드 제거

4c39674

chore: 초기 데이터 구조 변경

b3b2169

Merge branch 'step2' into step2-pr

fb8bb74

refactor: 테스트 주석 제거

5b3b342

Gyuchool requested changes May 13, 2026

View reviewed changes

mvg01 commented May 14, 2026

View reviewed changes

mvg01 added 3 commits May 14, 2026 13:28

refactor: 시간 정각 검증 책임을 도메인에서 서비스로 이동

2db5f8a

- 정각 시간 정책은 도메인 검증보단 서비스 검증이 올바르다고 판단 - 잘못된 값 삽입 시 예외, 조회 시 경고 로그로 분리(예외는 안터짐)

refactor: 유효하지 않은 시간 필터링 로직을 isValidTime 메서드로 추출

43840f2

refactor: 도메인 레코드를 클래스로 변경

f17c0ea

		private static void validateName(String name) {
		if (name.isBlank() \|\| name.length() > MAX_THEME_LENGTH) {

		private void reservationRequestValidate(ReservationRequest reservationRequest) {
		if (reservationRequest.name() == null) {

Conversation

mvg01 commented May 13, 2026

개요

체크 리스트

베이스 코드 선택 체크

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

1. mockData.sql에 대한 테스트 의존성

2. ErrorCode Enum 활용

에러 응답 형식 (토론 그룹에서 정한 형식)

Uh oh!

Gyuchool 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants