[1 - 3단계 방탈출 사용자 예약] 몰리(김지민) 미션 제출합니다. #22

jminkkk · 2024-05-02T08:09:50Z

안녕하세요 비밥!
이번 미션 리뷰 받게 된 몰리라고 합니다.
이번 단계 변경 사항은 다음과 같습니다. 링크

참고하실 부분

패키지 구조

현재 패키지 구조는 도메인형으로 작성되어 있습니다.
코드 리뷰하실 때 미리 참고하시면 더 수월하실 것 같아요 😁

reservation
	⎿ controller
	⎿ service
	⎿ dao
	⎿ dto

theme
	⎿ controller
	⎿ service
	⎿ dao
	⎿ dto
// ...

🤔 도메인형 패키지 구조를 선호하는 이유

특정 기능에 대해 수정이 필요해서 관련 Domain, Service등 을 수정해야 경우,
관련 도메인이 있는 패키지를 먼저 보고 그것과 연관된 패키지들만 열어놓는 것이 패키지를 헷갈릴 일이 더 적었다고 느껴서 선호합니다..!

DTO 네이밍

패키지 구조와 관련해 도메인형 구조에서는, 행위가 먼저 나오는 것이 빠르게 인식할 수 있을 것이라고 생각하여 아래와 같은 컨벤션을 따라 작성하였습니다.

<행위> <자원> <Response/Request>

🤔 도메인형 패키지 구조에서 행위를 먼저 작성했던 이유

만약 동료 개발자가 예약 조회 API의 응답을 수정해야 한다면

/reservation 패키지 확인 -> /dto 확인 -> 조회에 대한 dto 확인

순서로 확인을 할 것 같다고 생각했습니다. 그러다보니 dto 파일 이름에 행위가 먼저 나올 때 더 빠르게 찾을 수 있을 것 같아 행위를 먼저 작성하도록 했습니다!

추가 질문 사항은 코멘트를 통해 작성하겠습니다.
리뷰 잘 부탁드립니다 🙂

Co-authored-by: seunghye218 <[email protected]>

…경우 예외처리 Co-authored-by: seunghye218 <[email protected]>

- ReservationRepository의 findAllByTimeId 추가 Co-authored-by: seunghye218 <[email protected]>

Co-authored-by: seunghye218 <[email protected]>

- ReservationRepository에서 예약이 존재하는 지 확인하는 쿼리문을 날짜와 시간만 비교 Co-authored-by: seunghye218 <[email protected]>

Co-authored-by: seunghye218 <[email protected]>

…ThemeId() 메서드 제거

…figurer 구현체로 대체

…ime, Theme의 FakeRepository 구현

…해 변경

- ReservationTimeRepository에 existsByStartAt 추가 - ReservationTime에 isSameStartAt 추가

…를 사용해 변경

…로 변경 및 Fixture 제거

… data.sql이 위치 설정

jminkkk

안녕하세요 비밥! 오랜만입니다 😁
첫 리뷰 이후 이것저것 반영해보느라 2차 리뷰요청이 늦어졌어요 😭

1차 리뷰 이후 변경 사항이 조금 많은 것 같아서, 1차 리뷰 반영에 대한 변경 사항을 첨부합니다!

1차 리뷰 반영에 대한 변경 사항

주요 변경 사항

검증, 예외처리 보완

첫 제출 당시 시간 여유가 많이 부족했는데요 🥲
특히 검증과 예외처리 부분이 많이 아쉬워서 해당 부분을 치밀하게 반영하고자 노력했습니다!

테스트 개선

이전에는 더미데이터를 테스트용 db에 넣어두고 테스트를 진행했었는데요.
가독성 부분에서 매우 안좋다는 문제점을 느꼈어요.
또 계층별 테스트에서도 부족했던 부분이 많은 것 같아서 이 부분도 개선하고자 했습니다.

때문에 이번 pr에서는 테스트에 대해 개선하고자 제일 노력한 것 같아요...ㅎㅎ

테스트를 수정하면서 거짓 양성에 대한 정도가 매우매우 높았었구나를 느꼈습니다...
다른 개발자가 유지보수를 해야되는 상황이였다면 야근과 함께 저를 원망했을 것 같아요 😭

다른 참고사항은 코멘트로 작성해놓겠습니다!
이번 리뷰도 잘 부탁드립니다 🙇

jminkkk · 2024-05-05T19:07:39Z

src/main/java/roomescape/common/ViewControllerConfiguration.java

+import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
+
+@Configuration
+public class ViewControllerConfiguration implements WebMvcConfigurer {


기존에 템플릿 파일만 렌더링하는 역할만 하는 AdminController, UserController 가 있었는데요.
두 컨트롤러 모두, 별다른 로직 없이 단순 렌더링만 처리했었습니다.

따라서 다음과 같은 하나의 클래스를 만들어서, 뷰 매핑에 대한 처리를 담당하게 하는 것이 변경 사항을 쉽게 관리할 수 있을 것이라는 생각이 들었습니다!

jminkkk · 2024-05-05T19:23:04Z

src/test/java/roomescape/reservation/service/ReservationServiceTest.java

-        // stub
-        Mockito.when(reservationTimeRepository.findById(1L)).thenReturn(Optional.of(super.getReservationTimeById(1L)));
-        Mockito.when(themeRepository.findById(1L)).thenReturn(Optional.of(super.getThemeById(1L)));
-        Mockito.when(reservationRepository.existsByDateAndTimeAndTheme(LocalDate.of(2024, 10, 10), 1L, 1L)).thenReturn(true);


기존에 Mock를 사용했던 방식에서 Fake 객체를 활용해서 개선을 해보았는데요!
테스트 메서드 내에서 흐름도 더 잘보이고,
필요한 데이터만 제공하면 내부 동작까지는 생각할 필요없이 동작을 검증할 수 있다는 장점이 있는 것 같아요.
또 테스트 짜기에도 더 수월했던 것 같아요 🙌

좋은 피드백 감사합니다!🤗

pci2676

지난번 리뷰에 대해 반영해주신 것 확인했습니다. 👍

다음단계를 진행함에 있어 고민해보시면 좋을 부분에 리뷰를 남겨두었으니 확인 바랍니다.

머지하겠습니다. 🚀

pci2676 · 2024-05-06T12:24:14Z

src/main/java/roomescape/common/GlobalExceptionHandler.java

+
+    @ExceptionHandler
+    public ResponseEntity<String> catchValidationException(MethodArgumentNotValidException ex) {
+        return ResponseEntity.badRequest().body(ex.getBindingResult().getFieldErrors().get(0).getDefaultMessage() );


디버그 포인트를 잡아보시고 validate를 통과하지 못할 요청을 보내보시면 error가 여러개 들어오는것을 확인하실 수 있습니다.

그러면 지금 처럼 get(0)이 아닌 다른 모습으로 개선하실 수 있을것 같습니다.

검증 에러가 여러 개 올 수도 있는 것을 미쳐 몰랐네요 🥲
MethodArgumentNotValidException를 잘 알아보지 않고 사용해서 문제였던 것 같아요 개선하겠습니다!

pci2676 · 2024-05-06T12:27:08Z

src/main/java/roomescape/reservation/service/ReservationService.java

+        Reservation reservation = createReservationRequest.toReservation(reservationTime, theme);
+
+        validateReservationIsPast(reservation);


Reservation을 생성하고 validate를 하지 않는 다면 올바른 상태를 가지지 않은 Reservation 객체를 사용할 수 있어보입니다.

다시말해 생성과 검증이 분리되어 있는것 같아보입니다.

이를 조금 더 방어적으로 대응할 수 있는 방법에 대해 고민해보시면 좋을것 같습니다.

pci2676 · 2024-05-06T12:28:55Z

src/main/java/roomescape/reservation/service/ReservationService.java

+    private ReservationTime findReservationTime(final Long id) {
+        return reservationTimeRepository.findById(id)
+                .orElseThrow(() -> new NoSuchElementException("생성하려는 예약의 예약 시간이 존재하지 않습니다."));
+    }
+
+    private Theme findTheme(final Long id) {
+        return themeRepository.findById(id)
+                .orElseThrow(() -> new NoSuchElementException("생성하려는 테마가 존재하지 않습니다."));
+    }


exception이 발생하게 된 세부적인 사항을 담아서 exception 메세지를 구성한다면 운영할때 큰 도움이 될 수 있을것 같습니다.

pci2676 · 2024-05-06T12:36:47Z

src/main/java/roomescape/theme/repository/JdbcThemeRepository.java

+    @Override
+    public boolean existsById(final Long id) {
+        String sql = """
+                select count(*)
+                from theme as t
+                where t.id = ? 
+                """;
+        return jdbcTemplate.queryForObject(sql, Integer.class, id) != 0;
+    }


쿼리 성능에 대한 리뷰라 지금은 한번 읽고 넘어가셔도 좋습니다.

존재하는지에 대한 여부를 확인할때는 count 집계보다 limit 1 절을 이용하는 것이 더 효율적입니다.

count는 집계를 하기 위해 where절에 해당하는 모든 데이터를 검색해야하는 반면 limit은 1개의 집계결과가 탐색되면 거기서 탐색이 중지되기 때문입니다.

다시 말씀드리지만 지금은 쿼리에 집중해야하는 때는 아니니 쿼리에 대한 공부는 천천히 시간을 가지고 해보시면 좋을것 같습니다.

pci2676 · 2024-05-06T12:38:27Z

src/main/java/roomescape/theme/repository/JdbcThemeRepository.java

+    @Override
+    public List<Theme> findOrderByReservation() {
+        String sql = """
+                select t.id, t.name, t.description, t.thumbnail, count(t.id) as count
+                from theme as t
+                left join reservation as r
+                on r.theme_id = t.id
+                group by t.id
+                order by count desc
+                limit 10
+                """;
+        return jdbcTemplate.query(sql, themeRowMapper);
+    }


정책의 변경사항이 생겼을때 유동적인 대응이 필요할 수 있습니다.

가령 상위 10개의 테마가 아닌 3개만 보여주어야 한다는 등의 변화가 있을 경우

서버측은 변경사항이 없이 대응할 수 있으려면 limit절에 들어가는 10이라는 상수는 매개변수로 전달받아도 좋을것 같습니다.

또한 한번쯤 고민해보시면 좋을것 같은 포인트를 리뷰를 드려보면

해당 테마로 예약된 예약의 건수가 10만건 100만건 1000만건이 넘어간다면 위 쿼리로는 대응이 불가능해질 수 있습니다.

이를 해결할 수 있는 방법이 무엇이 있을지 고민해보시면 좋을것 같습니다.

지금 단계에서 반영할 정도의 내용은 아니니 고민만 해보셔도 좋습니다.
다른 크루들과 이야기해보아도 좋을것 같습니다.

pci2676 · 2024-05-06T12:45:42Z

src/main/java/roomescape/reservation/model/Reservation.java

+import roomescape.reservationtime.model.ReservationTime;
+import roomescape.theme.model.Theme;
+
+public class Reservation {


무언가 개선을 바라고 드리는 리뷰는 아닙니다.

다만 도메인 모델과 데이터 영속성 모델을 1:1로 대응하여 바라보는 것을 경계해보시면 좋을것 같습니다.

비즈니스 영역에서 필요한 도메인 모델이 데이터 영속성 모델로 동일하게 대응되어 저장되어야 한다는 법은 없으니까요.

jminkkk and others added 30 commits April 30, 2024 14:47

chore(/src): 프로젝트 동기화

b7fa7c8

test(RoomescapeApplicationTest): 요구사항 테스트 추가

220dac0

fix(/resources): 프론트 코드를 제공된 코드로 변경

e9e512a

docs(README.md): 요구사항 분석 작성

adcd185

Co-authored-by: seunghye218 <[email protected]>

feat(ReservationTimeController): 시간 관리 API가 적절한 응답을 하도록 변경

0140c32

Co-authored-by: seunghye218 <[email protected]>

feat(ReservationController): 예약 관리 API가 적절한 응답을 하도록 변경

7d72b09

Co-authored-by: seunghye218 <[email protected]>

fix(ReservationServiceTest, ReservationTimeServiceTest): 깨지는 테스트 수정

d4344c1

Co-authored-by: seunghye218 <[email protected]>

feat(CreateReservationTimeRequest): 시간 생성 요청 시 시간이 empty인 경우 예외처리

84c13ce

Co-authored-by: seunghye218 <[email protected]>

feat(CreateReservationRequest): 예약 생성 시 예약자명, 날짜, 시간에 유효하지 않은 값이 입력된 …

3ede852

…경우 예외처리 Co-authored-by: seunghye218 <[email protected]>

feat(ReservationTimeService): 특정 시간에 대한 예약이 존재하는데, 그 시간을 삭제하려 할 때 예외처리

e1db45d

- ReservationRepository의 findAllByTimeId 추가 Co-authored-by: seunghye218 <[email protected]>

feat(ReservationService): 지나간 날짜와 시간에 대한 예약 생성은 불가능하다는 정책 반영

0bf09ec

Co-authored-by: seunghye218 <[email protected]>

feat(ReservationService): 중복 예약에 대한 예외처리

3df73f6

Co-authored-by: seunghye218 <[email protected]>

feat(GlobalExceptionHandler): 예외 처리 시 응답을 위한 ControllerAdvice 구현

a2a6a6e

Co-authored-by: seunghye218 <[email protected]>

fix(ReservationService): 동일한 예약인지 체크하는 경우, 날짜와 시간만 체크하도록 변경

7a4eea9

- ReservationRepository에서 예약이 존재하는 지 확인하는 쿼리문을 날짜와 시간만 비교 Co-authored-by: seunghye218 <[email protected]>

fix(ReservationRepository): save 시 저장한 Reservation 객체를 반환하도록 변경

6eecf8e

Co-authored-by: seunghye218 <[email protected]>

refactor(ReservationRepository): existsByDateAndTime 로 이름 변경 및 메서드 인자 변경

35e9de8

Co-authored-by: seunghye218 <[email protected]>

fix(ReservationTimeRepository): save 시 저장한 ReservationTime 객체를 반환하도록 변경

063b784

Co-authored-by: seunghye218 <[email protected]>

docs: 2단계 요구사항 분석 작성

8c0119a

Co-authored-by: seunghye218 <[email protected]>

chore(schema.sql): 테마 DDL 추가

91ff6b2

Co-authored-by: seunghye218 <[email protected]>

feat(ThemeController): 테마 도메인 추가 및 테마 조회 기능 구현

0a4155a

Co-authored-by: seunghye218 <[email protected]>

feat(ThemeController): 테마 추가 기능 구현

c317e61

Co-authored-by: seunghye218 <[email protected]>

feat(ThemeController): 테마 삭제 기능 구현

cb08ac7

Co-authored-by: seunghye218 <[email protected]>

feat(AdminController): 테마 관리 페이지 추가 및 방탈출 예약 페이지 변경

c120aed

Co-authored-by: seunghye218 <[email protected]>

feat(Reservation): 예약 시 테마 기능 추가

6c63950

Co-authored-by: seunghye218 <[email protected]>

docs: 3단계 요구사항 분석 및 API 명세 작성

5298eef

Co-authored-by: seunghye218 <[email protected]>

feat(UserController): 사용자 예약 페이지 조회 추가

982bbc8

Co-authored-by: seunghye218 <[email protected]>

feat(UserController): 인기 테마 페이지 조회 추가

74e1ea6

Co-authored-by: seunghye218 <[email protected]>

feat(ReservationController): 예약 가능 시간 조회 기능 구현

1bbd1e1

Co-authored-by: seunghye218 <[email protected]>

feat(user-reservation.js): 예약 가능한 시간 추가/변경하도록 js 파일 수정

63ad269

Co-authored-by: seunghye218 <[email protected]>

feat(ThemeController): 인기 테마 조회 기능을 추가

0eb9926

Co-authored-by: seunghye218 <[email protected]>

jminkkk added 27 commits May 4, 2024 16:32

refactor(ReservationServiceTest): @nest 사용하여 같은 메서드에 대한 테스트 묶음

a5793e3

refactor(ReservationRepository): 사용하지 않는 findAllByTimeId(), findAllBy…

1736156

…ThemeId() 메서드 제거

refactor(ReservationTimeServiceTest): 예약 생성 및 삭제 실패 테스트 추가 및 @nest 사용

1c160ff

refactor(/service): 예외 메시지를 상세하게 변경

573361a

refactor(ViewControllerConfiguration): 템플릿 파일 렌더링 컨트롤러 제거 및 WebMvcCon…

b2ce8e2

…figurer 구현체로 대체

feat(ThemeController): 예약 생성 시 요청 바디에 대한 검증 추가

5d9f532

feat(CreateReservationRequest): 예약 생성 시 null 값에 대한 검증 추가

85ed3a3

feat(ReservationName): 예약자 명을 래핑한 객체 생성 및 검증 로직 추가

88634f1

feat(Reservation, ReservationTime, Theme): 객체 생성 시 검증 로직 추가

909326e

test(JbdcRepository): 단건 조회 시 오직 1개의 값만 반환하도록 쿼리 수정

a88fed1

refactor: 정적 팩토리 메서드를 네이밍 컨벤션에 맞게 변경

417deb2

test(FakeRepository): Mock이 아닌 Fake 테스트를 위해 Reservation, ReservationT…

dd903c0

…ime, Theme의 FakeRepository 구현

fix: 정적 팩토리 메서드의 네이밍 변경에서 누락된 수정 파일 수정

7a4c7ff

refactor(/test/fixture): 쉬운 테스트를 위한 객체별 픽스쳐 클래스 생성

e457c0c

refactor(ReservationServiceTest): Repository를 Mocking이 아닌 Fake 객체를 사용…

37f4c19

…해 변경

fix(ReservationTimeService): 방탈출 시간 생성 시 시작 시간이 이미 존재할 경우에 대한 검증 추가

7b1839b

- ReservationTimeRepository에 existsByStartAt 추가 - ReservationTime에 isSameStartAt 추가

refactor(ReservationTimeServiceTest): Repository를 Mocking이 아닌 Fake 객체…

bd0e081

…를 사용해 변경

refactor(ThemeServiceTest): Repository를 Mocking이 아닌 Fake 객체를 사용해 변경

ea7737b

refactor(JdbcReservationTimeRepositoryTest): @SpringBootTest를 @jdbcTest…

12168f6

…로 변경 및 Fixture 제거

refactor(ReservationTest): Fixture 제거

2f8ef8c

refactor(JdbcThemeRepositoryTest): @SpringBootTest를 @jdbcTest로 변경 및

8110bd4

refactor(JdbcReservationRepositoryTest): @SpringBootTest를 @jdbcTest로 변경

32e3bce

chore(application-test.properties): 테스트 main의 data.sql이 실행되지 않도록 테스트의…

6611432

… data.sql이 위치 설정

refactor(ReservationControllerTest): @SpringBootTest를 @WebMvcTest 변경

81653b1

refactor(ReservationTimeControllerTest): @SpringBootTest를 @WebMvcTest 변경

932126c

refactor(ThemeControllerTest): @SpringBootTest를 @WebMvcTest 변경

1ad986f

fix(/service, RoomescapeApplicationTest): 테스트 메서드 재활용 및 빈 충돌 해결

b0adc58

jminkkk commented May 5, 2024

View reviewed changes

pci2676 approved these changes May 6, 2024

View reviewed changes

pci2676 merged commit bda7a3b into woowacourse:jminkkk May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1 - 3단계 방탈출 사용자 예약] 몰리(김지민) 미션 제출합니다. #22

[1 - 3단계 방탈출 사용자 예약] 몰리(김지민) 미션 제출합니다. #22

jminkkk commented May 2, 2024 •

edited

Loading

jminkkk left a comment

jminkkk May 5, 2024

jminkkk May 5, 2024

pci2676 left a comment

pci2676 May 6, 2024

jminkkk May 7, 2024

pci2676 May 6, 2024

pci2676 May 6, 2024

pci2676 May 6, 2024

pci2676 May 6, 2024

pci2676 May 6, 2024

pci2676 May 6, 2024

		Reservation reservation = createReservationRequest.toReservation(reservationTime, theme);

		validateReservationIsPast(reservation);

[1 - 3단계 방탈출 사용자 예약] 몰리(김지민) 미션 제출합니다. #22

[1 - 3단계 방탈출 사용자 예약] 몰리(김지민) 미션 제출합니다. #22

Conversation

jminkkk commented May 2, 2024 • edited Loading

참고하실 부분

패키지 구조

DTO 네이밍

jminkkk left a comment

Choose a reason for hiding this comment

주요 변경 사항

검증, 예외처리 보완

테스트 개선

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pci2676 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk commented May 2, 2024 •

edited

Loading