[1 - 3단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #9

alstn113 · 2024-05-02T07:38:25Z

안녕하세요 영이! 구름 ⛅️ 입니다!

이번 미션에서는 첫번째 미션을 이어서 하는 미션이었어요!
이번 단계의 변경 사항은 다음과 같아요! 링크
추가 사항들이 많았고 더 복잡한 sql문을 작성하거나 spring의 다양한 어노테이션을 사용할 수 있어서 재밌었습니다!

제가 궁금했던 부분들은 다음과 같아요.

도메인과 레포지터리에서 표준 에러만을 사용했어요. 또한 예전에는 Optional findById를 서비스에서 예외 처리하고 사용했는데요. 이번에는 repository interface에서 default 메서드를 사용해서 처리해봤어요. 이게 적절했을까요?
저는 예전에 프로그래밍할 때 deleteById 같은 경우 id가 있는지 검증하고 not found exception을 처리해줬어요. 그런데 성능 상이유로 그냥 없는 경우 무시하고 204처리를 한다고도 하더라구요. 이런 경우에 대해 어떻게 생각하시나요? 저는 무조건 검증하고 간다는 마인드입니다.
dto, domain뿐만 아니라 상황에 따라서 controller, service에서도 검증 처리를 할 수 있다고 생각하는데요. 이번 미션의 경우에 null, blank를 domain에서 처리해줬어요. 사실 제일 좋은 방법은 모든 부분에 대해서 동일한 검증을 하는 것이라고 생각해요. 하지만 그런 경우에는 리소스가 많이 들겠죠? 또한 각각 계층에서 검증해야하는 수준을 정의해서 사용해도 좋다고 생각해요. 저는 domain에서 null, blank를 처리해야 같이 개발하는 개발자의 실수 또한 막고 가성비가 좋다고 생각해서 그렇게 처리했어요. 영이는 어떻게 생각하시나요??
이번 미션에서는 바뀌는 부분도 많고 E2E 테스트에 집중하기 위해서 이전 테스트를 많이 지웠어요. 저번 미션에서 저는 RestAssured를 이용한 인수테스트, 목킹을 이용한 컨트롤러 테스트, 목킹을 이용한 서비스 테스트, JdbcTest를 이용한 레포지터리 테스트를 진행했어요. 하지만 시간적으로 여유가 없고 너무 자주 바뀌는 것 같아서 RestAssured를 이용해서 컨트롤러 부분만 테스트했어요. 현재 시간에 대한 처리를 하기 위해서 Clock을 Bean으로 사용하기도 하고, @Sql을 이용해서 테스트용 값들을 불러오기도 했어요. 이런 방법들이 적절하게 사용되었을까요??

미리 리뷰 감사합니다 영이! 좋은 하루되세요~

alstn113 · 2024-05-04T12:34:42Z

우선 레벨 2임에도 불구하고 컨벤션을 제대로 맞추기 못한 점 죄송합니다.

수정 사항은 다음과 같아요.

피드백 받은 것들을 수정하면서 코드 구조를 의존성 방향에 맞게 수정했습니다.
RequestDto에서 Time과 Date에 대한 예외 메세지를 주기 위해서 커스텀 validator를 만들었습니다.
도메인과 서비스에 대한 테스트를 진행했습니다.
available reservation times를 구하기 위해 reservationTimeRepo.findAll과 reservationRepo.findByDateAndThemeId를 사용했으나 한 번의 쿼리로 개선했습니다.

choijy1705

구름 피드백 잘 반영해주셨어요
다만 times/available api가 정상 동작하지 않는것 같습니다
이부분 다시한번 확인해보시고 다시 리뷰요청 부탁드립니다!

choijy1705 · 2024-05-05T06:42:50Z

src/main/java/roomescape/controller/exception/GlobalExceptionHandler.java

+                .body(new ErrorResponse(e.getMessage()));
+    }
+
+    @ExceptionHandler({NoSuchElementException.class})


Suggested change

@ExceptionHandler({NoSuchElementException.class})

@ExceptionHandler(NoSuchElementException.class)

choijy1705 · 2024-05-05T06:49:10Z

src/main/java/roomescape/domain/Reservation.java

+    private void validate(String name, LocalDate date, ReservationTime time, Theme theme) {
+        validateName(name);
+
+        if (date == null) {
+            throw new IllegalArgumentException("날짜는 필수 값입니다.");
+        }
+
+        if (time == null) {
+            throw new IllegalArgumentException("예약 시간은 필수 값입니다.");
+        }
+
+        if (theme == null) {
+            throw new IllegalArgumentException("테마는 필수 값입니다.");
+        }
+    }
+
+    private void validateName(String name) {
+        if (name == null || name.isBlank()) {
+            throw new IllegalArgumentException("이름은 필수 값입니다.");
+        }
+
+        if (name.length() > NAME_MAX_LENGTH) {
+            throw new IllegalArgumentException(String.format("이름은 %d자를 넘을 수 없습니다.", NAME_MAX_LENGTH));
+        }
+    }


private 메서드들은 클래스 가장 하단에 위치하면 좋을것 같아요!

저는 private을 사용하는 곳 아래에 두는 편이에요.
그 다음으로 equals, hashCode두고, 가장 아래에 getter를 둬요.

choijy1705 · 2024-05-05T06:54:04Z

src/main/java/roomescape/repository/JdbcReservationTimeRepository.java

+
+

Suggested change

choijy1705 · 2024-05-05T06:54:35Z

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

+
+    @Override
+    public List<Theme> findPopularThemes(LocalDate startDate, LocalDate endDate, int limit) {
+        // BETWEEN A AND B: A와 B를 포함한 구간


Suggested change

// BETWEEN A AND B: A와 B를 포함한 구간

LocalDate의 isBefore 등이나 BETWEEN은 항상 헷갈리더라구요.
다른 개발자들이 이해하기 쉽게 저 정도는 괜찮지 않을까요??

로직에 대한 설명도 아니고
SQL에 대한 설명을 주석으로 적는건 더더욱 이상한것 같아요!🤔
극단적으로 보면 java 메서드에대한 설명을 적는것과 같은 느낌이네요

흠 솔직히 상관없다고 생각하는데 모르겠습니다,,,
SQL에 대한 지식이 불충분해서 그런걸까요?? 그렇다고 생각해서 지웠습니다.

혼자 작업하면 사실 문제없지만
수십명이 같이 작업하는 코드에
한명 두명씩 쿼리 혹은 메서드 등에 대하여 설명을 적으면 난장판이 되지 않을까요🤔🤔

맞는 말인 것 같습니다 ㅎㅎ

choijy1705 · 2024-05-05T07:00:34Z

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

+        return reservations.stream()
+                .map(ReservationResponse::from)
+                .toList();
+    }


service에서 dto로 변환해서 반환하면 어떤 장점이 있나요??

단점으로는 해당 메서드를 재활용할 수 없을것 같네요!

service에서 domain을 반환하면 다른 계층에서 변경 가능성이 생기거나 불필요한 정보가 노출 될 수 있다고 생각해서 이 방법은 제외하고 생각했습니다.

대체 방안으로 web dto와 service dto를 사용하거나, domain service를 만들어서 해결할 수 있을 것 같습니다.
하지만 현재 프로젝트의 복잡도가 크지 않아서 하지 않았습니다.

현업에서는 아마도 모든곳에서 service layer에서 반환하는 값은 도메인을 그대로 반환할것 같네요!
dto의 변환은 controller에서 하고요
대부분의 경우 service에서의 메서드들은 대부분 다른 service 혹은 같은 클래스의 다른 메서드에서 호출하는 경우가 대부분일것 같습니다. 물론 지금은 로직 자체가 거의없어서 dto를 반환할 수 있겠지만 조금만 로직이 추가되어도 훨씬 복잡해질것 같네요

흠,,, 현재 프로젝트 복잡도 상 dto를 controller와 service에서 모두 사용해서 그렇게 보일 수 있을 것 같습니다.
Web Dto와 Service Dto로 분리하면 해결되는 문제라고 생각되는데요. 너무 복잡해진다고 생각해요.
그리고 dto를 반환해도 충분히 재사용 가능하지 않나요??

도메인에서 로직을 처리해야하지 dto로 로직을 처리하면 안될것 같습니다.
물론 지금은 로직자체가 없기 때문에 그렇게 생각하실 수 있을것 같아요

choijy1705 · 2024-05-05T07:05:54Z

src/main/java/roomescape/controller/api/ReservationTimeController.java

+        return ResponseEntity.noContent().build();
+    }
+
+    @GetMapping("/available")


이 api가 정상동작하지 않네요!

다음과 같이 수정했습니다. 죄송합니다!

변경 전

GROUP BY r.id

변경 후

GROUP BY rt.id, rt.start_at

choijy1705 · 2024-05-05T07:50:02Z

src/main/java/roomescape/repository/JdbcReservationTimeRepository.java

+    public List<AvailableReservationTimeDto> findAvailableReservationTimes(LocalDate date, Long themeId) {
+        String sql = """
+                    SELECT
+                        rt.id,
+                        rt.start_at,
+                        COUNT(r.id) > 0 AS already_booked
+                    FROM reservation_time AS rt
+                    LEFT JOIN reservation AS r
+                    ON r.time_id = rt.id AND r.date = ? AND r.theme_id = ?
+                    GROUP BY r.id
+                """;


원하는 동작이 나오는지 다시한번 점검해보면 좋을것 같습니다🤔

다음부터는 꼭 확인하도록 하겠습니다,,,

choijy1705

구름 빠르게 수정해주셨네요👍
테스트 관련해서 코멘트 남겼는데
확인해보시고 답변 혹은 수정해보시면 좋을것 같습니다!

choijy1705 · 2024-05-05T13:37:43Z

src/test/java/roomescape/domain/ReservationTest.java

+    @ParameterizedTest
+    @NullSource


null인 경우만 테스트 하는것 같은데
parameterizedTest를 사용할 필요가 있나요??
어노테이션으로 테스트 의도를 표현해서 명시적이지 못한것 같아요!

위에서부터 같은 코드를 계속 적다보니까 그렇게 된 것 같습니다.
일반 Test로 변경했습니다.

choijy1705 · 2024-05-05T13:41:31Z

src/test/resources/application.yml

+      enabled: true
+      path: /h2-console
+  datasource:
+    url: jdbc:h2:mem:database


production과 공유하는걸 의도하신건가요?

아 무의식적으로 적었던 것 같습니다.
application.yml에 관련 내용을 적지 않으면 랜덤한 이름의 메모리 DB가 생긴다는 건 알고 있었습니다.
production쪽과 공유된다는 걸 인지하지 못했네요. 수정했습니다!

choijy1705 · 2024-05-05T13:45:18Z

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

+import roomescape.dto.request.ReservationRequest;
+import roomescape.dto.response.ReservationResponse;
+
+@ExtendWith(MockitoExtension.class)


service 테스트들을 왜 mock으로 한걸까요?
실제로 db에 저장하고 이를 통해 로직들이 정상적으로 수행되는지를 테스트해보는게 좋지 않을까요?
이 test들이 동작한다고해서 모든 로직들이 성공한다는것을 신뢰할 수 없을것 같아요🤔

예를 들면 좀전에 수정하신 예약가능한 시간을 조회하는 쿼리를 안되던 걸로 다시 바꿔도
테스트들은 모두 통과할것 같네요

서비스를 격리해서 테스트하고 싶었습니다.
아무래도 repository 테스트를 하지 않아서 이전 쿼리 문제를 발견하지 못한 것 같아요.
그래서 repository 테스트를 추가했습니다.

영이는 mock 테스트를 어느 경우에 사용하시나요??

mock은 분명 개발을 하시다보면 필요한 경우가 있을거에요!
예를들면 시간 관련 로직을 짠다고 하면 항상 시간의 값은 항상 변하기 때문에 mocking을 통하여 고정된 값을 반환받는 경우가 있을 수 있을것 같습니다
지금과 같은 방법의 테스트는 저라면 신뢰할 수 없을것 같습니다. 단순히 테스트 커버리지를 높이는 테스트라고 밖에 생각이 안들어요ㅠ
service - repsitory를 통합해서 제대로 값이 저장되는지의 통합테스트로서 충분히 테스트의 목적을 이룰수 있을것 같습니다

choijy1705

구름 피드백 반영 고생하셨어요👍
이번 단계는 여기서 머지하겠습니다
테스트에 관한 답변 남겼으니 확인해주세요!

choijy1705 · 2024-05-06T14:04:48Z

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

+import roomescape.dto.request.ReservationRequest;
+import roomescape.dto.response.ReservationResponse;
+
+@ExtendWith(MockitoExtension.class)


mock은 분명 개발을 하시다보면 필요한 경우가 있을거에요!
예를들면 시간 관련 로직을 짠다고 하면 항상 시간의 값은 항상 변하기 때문에 mocking을 통하여 고정된 값을 반환받는 경우가 있을 수 있을것 같습니다
지금과 같은 방법의 테스트는 저라면 신뢰할 수 없을것 같습니다. 단순히 테스트 커버리지를 높이는 테스트라고 밖에 생각이 안들어요ㅠ
service - repsitory를 통합해서 제대로 값이 저장되는지의 통합테스트로서 충분히 테스트의 목적을 이룰수 있을것 같습니다

alstn113 added 30 commits May 1, 2024 12:35

migrate first mission

0bc239e

feat: 도메인 예외 검증 처리

e3f2f70

feat: 예약, 예약 시간 삭제 시 존재 여부 처리

27a9444

feat: 전역 예외 처리 구현

01dbb41

feat: 동일한 시간, 예약 검증 처리

d54f0dd

feat: 지나간 날짜/시간에 대한 예약 생성 불가능

1fca9c5

feat: 테마 구현

502a62c

feat: 예약 테마 추가

e91ff24

chore: exception handler 대기

155f4c5

feat: 초기 데이터 추가

bf86182

feat: 사용자 예약 구현 추가

58acf32

feat: 인기 테마 페이지 연결

249978b

feat: 인기 테마 구현

833bb9a

refactor: 불필요한 도메인 제거 및 코드 개선

fe9f1a3

test: 페이지 조회 테스트

3c5dac9

feat: 예약 생성 테스트

1702516

refactor: 삭제 시 검증 로직 개선

4587085

refactor: 이용 가능한 예약 시간의 시작 시간 응답 포맷 수정

15b090a

test: 예약 시간 컨트롤러 테스트

8c45bb1

feat: 테마 컨트롤러 테스트

e7e1421

test: 예약 생성 테스트

4a9d06c

refactor: spring config 수정

f17f2ae

test: 예약 조회, 삭제 테스트

cc4d678

test: 예약 시간 생성, 삭제 예외 테스트

0393318

test: 예약 생성, 삭제 실패 테스트 작성

9768b4e

feat: 초기 데이터 추가

10aebea

test: 중복된 테마 생성, 존재하지 않는 테마 생성 테스트

91abcc7

refacotr: 중복된 이름의 테마로 테스트를 수정한다.

6a80bbd

refactor: 인기있는 테마 조회 기능 개선

8762821

test: 인기있는 테마 조회 테스트

ea5eda9

alstn113 added 8 commits May 4, 2024 12:48

feat: spring-web-validation 의존성 추가

d12f399

feat: 날짜, 시간 형식 Validator 구현

90e9c9e

refactor: dto 패키지 수정

59a7601

test: 도메인 테스트

6d0aac6

refactor: 패키지 이름 변경

d6772e3

refactor: 패키지 명 변경

fc15214

test: 서비스 테스트

e86495c

fix: 필드 수정

2b74989

alstn113 added 7 commits May 4, 2024 22:32

refactor: 메서드 분리

fc0deaf

refactor: available reservation times query 개선

4d4b193

fix: 중복된 이용 가능한 시간 수정

e96636b

refactor: 쿼리 성능 개선

2c2241d

refactor: 쿼리 개선

563b171

refactor: 중복 데이터 제거

40a8a31

refactor: 패키지 구조 변경

6271b4a

choijy1705 requested changes May 5, 2024

View reviewed changes

alstn113 added 4 commits May 5, 2024 16:56

fix: 이용가능한 시간 조회 쿼리 수정

96db16f

chore: 공백 제거

51280df

chore: 중괄호 제거

eaefe2f

refactor: 주석 제거

113a6a1

choijy1705 requested changes May 5, 2024

View reviewed changes

alstn113 added 6 commits May 5, 2024 22:55

refactor: null source test 변경

4b806f8

refactor: test application.yml 수정

3f02c91

refactor: web page 테스트 개선

0dc91a7

test: theme repository 테스트

e7ad751

test: reservation time repository 테스트

a91c1f0

test: reservation repository 테스트

280f402

choijy1705 approved these changes May 6, 2024

View reviewed changes

choijy1705 merged commit 9a4fe57 into woowacourse:alstn113 May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1 - 3단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #9

[1 - 3단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #9

alstn113 commented May 2, 2024 •

edited

Loading

alstn113 commented May 4, 2024 •

edited

Loading

choijy1705 left a comment

choijy1705 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024 •

edited

Loading

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 left a comment

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 5, 2024

choijy1705 May 5, 2024

alstn113 May 5, 2024

choijy1705 May 6, 2024

choijy1705 left a comment

choijy1705 May 6, 2024

	@ExceptionHandler({NoSuchElementException.class})
	@ExceptionHandler(NoSuchElementException.class)

[1 - 3단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #9

[1 - 3단계 방탈출 사용자 예약] 구름(김민수) 미션 제출합니다. #9

Conversation

alstn113 commented May 2, 2024 • edited Loading

alstn113 commented May 4, 2024 • edited Loading

choijy1705 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 May 5, 2024 • edited Loading

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

choijy1705 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

choijy1705 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 commented May 2, 2024 •

edited

Loading

alstn113 commented May 4, 2024 •

edited

Loading

alstn113 May 5, 2024 •

edited

Loading