[7 - 9단계 방탈출 예약 관리] 레디(최동근) 미션 제출합니다. #158

reddevilmidzy · 2024-04-25T06:29:59Z

안녕하세요 오리👋

7~9단계 미션 제출합니다!

이전 단계와의 http 상태코드가 변경되었는데 LMS에서 명시된 API를 따랐습니다 :)

리뷰 감사드립니다 🙇‍♂️

jinyoungchoi95

안녕하세요 레디 😄

7-9단계 잘 구현해주셨네요. 몇가지 코멘트 남겨두었으니 확인부탁드려요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2024-04-25T07:51:05Z

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

    }

-    public static Reservation create(final String name, final String date, final String time) {
-        return new Reservation(null, name, date, time);
+    public static Reservation create(final String name, final String date, final ReservationTime time) {


�정적 팩토리 메소드가 아닌 생성자로 해도 될 것 같은데 정적 팩토리 메소드여야할 이유가 있을까요?

정적 팩토리 메서드의 장점은 기본 생성자를 사용하는 것보다 이름을 부여할 수 있어 명확한 의도를 들어낼 수 있고, 검증 로직을 정적 팩토리 메서드 안에 넣어 생성자를 가볍게 만들 수 있다고 생각합니다!

하지만 현재 제 코드는 정적 팩토리 내에서 어떠한 검증도 처리하고 있지 않고 메서드 이름도 create로 불분명한 이름을 사용하고 있다는 문제점이 있네요ㅎㅎ

정적 팩토리 메서드를 제거하는 방향으로 리팩터링하였습니다!

jinyoungchoi95 · 2024-04-25T07:53:36Z

src/main/java/roomescape/repository/ReservationDao.java

+                on r.time_id = t.id
+                where r.id = ?
+                """;
+
        try {
            final Reservation reservation = jdbcTemplate.queryForObject(


findAll의 mapping과 동일한 코드가 반복되는데 제거할 수 있을까요?

헉 예리하십니다 !!

jinyoungchoi95 · 2024-04-25T07:54:34Z

src/main/java/roomescape/repository/TimeDao.java

+import roomescape.model.ReservationTime;
+
+@Repository
+public class TimeDao {


사용하는 도메인은 ReservationTime인데 TimeDao로 축약할 필요가 있을까요?

없을 거 같습니다!
Time 객체를 만들기 전에 TimeDao를 먼저 만들었었는데, Time을 만들려다보니까 sql의 Time과 겹쳐 ReservationTime으로 변경했었는데 Dao랑 Service, Controller는 놓쳤네요 😅

jinyoungchoi95 · 2024-04-25T07:54:54Z

src/test/java/roomescape/repository/TimeDaoTest.java

+            assertThat(connection).isNotNull();
+            assertThat(connection.getCatalog()).isEqualTo("DATABASE_TEST");
+            assertThat(connection.getMetaData().getTables(null, null, "RESERVATION_TIME", null).next()).isTrue();


assertAll이 붙어야할 것 같아요.

jinyoungchoi95 · 2024-04-25T07:56:43Z

src/test/java/roomescape/repository/TimeDaoTest.java

+
+    @DisplayName("전체 조회")
+    @Test
+    void findAll() {


@Sql(scripts = "/createTime.sql")를 다른 곳에는 적용되어있는데요. 어떤 동작을 할까요?

이부분을 적용하여서 이전 pr에 이야기했던 내용을 개선할 수 있지 않을까요?

헉 맞습니다!
timeDaoTest에도 추가해주었습니다 👍

일단 @Sql 애노테이션을 붙이면 지정한 경로의 sql 파일을 실행시켜주는 친구라는 것 정도만 파악하고 사용하였습니다

좀 더 찾아보는 과정에서 질문이 생겼습니다!

executionPhase를 BEFORE_TEST_METHOD로 설정해주었었는데 찾아보니 디폴트가 BEFORE_TEST_METHOD이더라구요.
그래서 executionPhase를 지웠습니다. 여기서 질문이 생겼는데요,,,

4~6 단계에서도 spring.h2.console.path의 경로의 디폴트가 /h2-console이여서 지워주었는데 이처럼 디폴트인 경우 무조건 지우는게 적절한가요?

@GetMapping 같은 경우 디폴트가 "" 여서 @GetMapping("") 보단 @GetMapping 이 훨씬 깔끔하여 제거했었는데 위처럼 디폴트가 뭐인지 예측이 안되는 경우 디폴트이더라도 명시해주는 것이 좋을까요 아니면 생략하는게 좋을까요?

질문이 생겼지만 크루와 이야기하면서 해소하였습니다! ㅎㅎ

우선 쉽게 예측할 수 없는 옵션들은 지정해주는 것이 좋다라고 결론이 났는데요, 이유는 사용하는 사람이 해당 설정이 디폴트라는 것을 인지하지 못할 수도 있고, 또 버전에 따라 디폴트 값이 변경될 수 있다는 의견도 들었습니다!

만약 버전이 변경되어 디폴트 값이 변경된다면 위 테스트들은 터지게될 수 있다는 문제점이 있어 명시해주기로 하였습니다 ㅎㅎ

jinyoungchoi95 · 2024-04-25T07:59:39Z

src/main/java/roomescape/service/TimeService.java

+        final ReservationTime created = ReservationTime.create(timeRequest.startAt());
+        final ReservationTime saved = timeDao.save(created);


created와 saved로 의미 전달이 잘 될까요? 단순히 crud들을 작성하는데 있어서 어떤 코드가 가독성이 좋을지를 고민해보시면 좋을 것 같아요.

jinyoungchoi95 · 2024-04-25T08:00:17Z

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

-    public ReservationResponse save(final ReservationRequest request) {
-        final Reservation created = Reservation.create(request.name(), request.date(), request.time());
-        final Reservation saved = reservationDao.save(created);
+    public ReservationResponse save(final ReservationRequest requestV2) {


Suggested change

public ReservationResponse save(final ReservationRequest requestV2) {

public ReservationResponse save(final ReservationRequest request) {

V2는 리팩터링에 사용했다면 제거해주세요.

jinyoungchoi95 · 2024-04-25T08:02:02Z

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

service들이 슬슬 복잡해지는데요. 테스트가 필요하지 않을까요?

controller에서 테스트하고 있어 생략한다면 레벨1때도 상위에서 쓴다고 테스트를 생략했었는지를 떠올려보시면 좋을 것 같아요.

Service 테스트는 누락된걸까요? 작성하지 않는다면 작성하지 않는 이유를 말씀부탁드려요.

흑 누락입니다...! 😥
저도 서비스에 대한 테스트가 필요하단 것에 동의합니다!
테스트 추가해주었습니다 :)

jinyoungchoi95 · 2024-04-25T08:03:20Z

src/main/java/roomescape/dto/TimeRequest.java

@@ -0,0 +1,4 @@
+package roomescape.dto;
+
+public record TimeRequest(String startAt) {


생성과 업데이트 등 어디에 Request가 되었는지 명시하는지 좋지않을까요?

jinyoungchoi95 · 2024-04-25T08:04:31Z

src/main/java/roomescape/controller/ReservationController.java

-                .toUri();
-        return ResponseEntity.created(uri)
-                .body(reservationResponse);
+        return ResponseEntity.ok(reservationResponse);


ResponseEntity가 반환타입으로 필요하지 않은 곳은 이제 제거해주어도 되지 않을까싶어요.

좋습니다!

jinyoungchoi95

안녕하세요 레디 😄

몇가지 코멘트 남겨두었으니 확인부탁드려요. 추가로 다음 pr보내주실 때 10단계 진행여부를 함께 말씀부탁드려요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2024-04-26T10:28:17Z

src/main/java/roomescape/model/ReservationTime.java

+        this.id = null;
+        this.startAt = LocalTime.parse(startAt);


Suggested change

this.id = null;

this.startAt = LocalTime.parse(startAt);

�this(null, LocalTime.parse(startAt));

으로 부생성자 표현을 사용할 수 있을 것 같아요.

Reservation에도 적용하였습니다!

jinyoungchoi95 · 2024-04-26T10:28:46Z

src/main/java/roomescape/repository/ReservationTimeDao.java

+                    (resultSet, rowNum) -> new ReservationTime(
+                            resultSet.getLong("id"),
+                            resultSet.getTime("start_at").toLocalTime()
+                    ), id);


여기도 RowMapper가 중복되네요.

jinyoungchoi95 · 2024-04-26T10:29:36Z

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

+        final Optional<ReservationTime> findTime = reservationTimeDao.findById(request.timeId());
+        if (findTime.isEmpty()) {
+            throw new IllegalArgumentException("존재하지 않는 시간입니다.");
+        }


#158 (comment)

동일하게 변경할 수 있겠네요.

오 맞네요!!
덕분에 밑에서 사용할 때 지저분했던 .get()도 제거할 수 있어졌네요 👍

jinyoungchoi95 · 2024-04-26T14:02:06Z

src/test/java/roomescape/MissionStepTest.java

@@ -62,19 +65,19 @@ void step2() {

    @DisplayName("예약 저장 및 예약 삭제")
    @Test
+    @Sql("/createTime.sql")


@Test에 @Sql을 두는게 맞을지, class단에 두는게 맞을지는 고민해보시면 좋을 것 같아요.

@Sql을 메소드마다 달면 너무 중복이 많을 것 같네요.

missionStepTest를 제외한 나머지 테스트들은 전부 클래스 레벨에 @Sql에 달아주었습니다!

missionStepTest는 lms에서 제공한 테스트를 전부 모아둔 클래스라 클래스 단에 붙이는건 살짝 애매할 거 같습니다..!

jinyoungchoi95 · 2024-04-26T14:04:37Z

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

Service 테스트는 누락된걸까요? 작성하지 않는다면 작성하지 않는 이유를 말씀부탁드려요.

jinyoungchoi95 · 2024-04-26T14:06:10Z

src/test/java/roomescape/repository/ReservationDaoTest.java

+        final ReservationTime time = new ReservationTime("10:30");
+        final Reservation reservation = new Reservation("레디", "2024-02-03", time);
+
+        reservationDao.save(reservation, 1L);


@Sql을 사용하면 save할 필요가 없지 않나요? 나머지 부분도 마찬가지에요.

@Sql을 사용한다면 통일성있게 전반적인 테스트들에서 적용해주시면 좋을 것 같아요.

jinyoungchoi95 · 2024-04-26T14:09:56Z

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

+        this.reservationTimeService = reservationTimeService;
+    }
+
+    @GetMapping


@GetMapping 같은 경우 디폴트가 "" 여서 @GetMapping("") 보단 @GetMapping 이 훨씬 깔끔하여 제거했었는데 위처럼 디폴트가 뭐인지 예측이 안되는 경우 디폴트이더라도 명시해주는 것이 좋을까요 아니면 생략하는게 좋을까요?

디폴트값이 명시적이어서 영향이 가는지 안가는지에 따라 다를 것 같아요. ""의 경우 큰 로직의 영향도가 없기 때문에 생략해도 문제가 없을 수 있지만 말씀해주신 executionPhase = BEFORE_TEST_METHOD의 경우 보이는 값을 명시함으로써 언제 실행되는지를 알 수 있어 명시하는 편이 좋다는 것에 공감합니다.

reddevilmidzy · 2024-04-29T07:52:34Z

리뷰 감사드립니다! 주말동안 배운 내용 정리하느라 미션 진행이 늦어졌네요..ㅎㅎ 이번 미션은 9단계에서 마무리하려 합니다!

jinyoungchoi95

안녕하세요 레디 😄

현재 미션에 필요한 구현과 학습을 충분히 하신 것 같아 머지하겠습니다. 스프링 첫 미션이었는데 많은 도움이 되셨길 바랍니다 :)

감사합니다 🙇‍♂️

jinyoungchoi95 · 2024-04-29T13:39:45Z

src/test/java/roomescape/controller/ReservationTimeControllerTest.java

+    @Test
+    @DisplayName("시간 예약 조회")
+    void getTimes() {
+        assertThat(controller.getTimes()).hasSize(3);
+    }
+
+    @DisplayName("시간 예약 저장")
+    @Test


어노테이션의 순서가 혼재되어있는데 어떤 어노테이션을 위로 둘것인지 기준을 잡으시면 나중에 도움이 되실 것 같아요.

jinyoungchoi95 · 2024-04-29T13:41:07Z

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

+        final List<ReservationResponse> beforeRemoving = reservationService.findAll();
+        reservationService.remove(1L);
+        final List<ReservationResponse> afterRemoving = reservationService.findAll();
+
+        assertThat(beforeRemoving.size() - afterRemoving.size()).isOne();


어짜피 createTimeAndReservations.sql을 사용한다는 것을 알기 때문에 beforeRemoving을 넣지 않고 afterRemoving에서 예상되는 결과로 assertion해도 되지 않을까해요.

reddevilmidzy added 17 commits April 24, 2024 14:15

feat: 예약 시간 화면 조회 페이지 구현

759ab17

feat: 예약 시간 저장 기능 구현

2aa4eb1

test: reservation_time 테이블 생성 확인

51f7a25

feat: 예약 시간 조회 기능 구현

28d892a

feat: 예약 삭제 기능 구현

e4910aa

test: 시간 저장 조회 삭제 테스트 추가

91800d0

feat: 예약 저장 시 시간은 정해진 시간내에서 선택 기능 구현

188d034

feat: 예약 조회 기능 구현

3e20a55

feat: 예약 삭제 기능 구현

ff96908

test: 시간 저장 테스트 추가

998eac9

refactor: 레거시 코드 제거

d05ed80

refactor: 클래스 이름 변경

0922556

refactor: 불필요한 메서드 분리 제거

68cf071

refactor: 중복되는 @Sql 애노테이션 클래스 레벨로 이동

0225870

docs: api 명세 변경

4c44990

refactor: 메서드명 변경

0f89c35

fix: 예외 메시지 수정

d14fe81

jinyoungchoi95 requested changes Apr 25, 2024

View reviewed changes

reddevilmidzy added 11 commits April 25, 2024 19:20

test: assertAll 추가

28f45cd

refactor: 파라미터 이름 변경

dcc34a4

refactor: 불필요한 ResponseEntity 제거

e505ef3

refactor: id 조회시 Optional 을 반환하지 않고 바로 예외 처리 하도록 변경

42113ff

refactor: time 앞에 Reservation 추가

0f105c0

refactor: 생성 요청 dto 이름에 create 접두사 추가

7c2c7e6

refactor: 불필요한 정적 팩토리 메서드 대신 생성자로 변경

2f79c2a

test: ReservationTimeDaoTest에서 @Sql 활용

6a51d04

refactor: 시간 생성 쿼리문 수정

294b6e5

refactor: @Sql executionPhase 명시

33c4645

refactor: resultSet을 Reservation으로 반환하는 중복 로직 메서드로 분리

7d76a40

jinyoungchoi95 requested changes Apr 26, 2024

View reviewed changes

reddevilmidzy added 8 commits April 28, 2024 15:31

refactor: 부생성자 사용

ed46c74

refactor: rowMapper 중복 메서드로 분리

f7f1e64

refactor: Optional 반환 대신 예외 처리

8187a12

refactor: 서비스의 단건 조회 메서드 제거

2f12c71

test: ReservationTimeService 테스트 추가

13aab82

test: ReservationService 테스트 추가

f7d6bb2

test: 존재하지 않은 시간 삭제시 예외 테스트 추가

40901bd

test: @Sql 메서드 레벨에서 클래스 레벨로 이동

e3ccd46

jinyoungchoi95 approved these changes Apr 29, 2024

View reviewed changes

jinyoungchoi95 merged commit 10e0aa4 into woowacourse:reddevilmidzy Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7 - 9단계 방탈출 예약 관리] 레디(최동근) 미션 제출합니다. #158

[7 - 9단계 방탈출 예약 관리] 레디(최동근) 미션 제출합니다. #158

reddevilmidzy commented Apr 25, 2024

jinyoungchoi95 left a comment

jinyoungchoi95 Apr 25, 2024

reddevilmidzy Apr 26, 2024 •

edited

Loading

jinyoungchoi95 Apr 25, 2024

reddevilmidzy Apr 26, 2024

jinyoungchoi95 Apr 25, 2024

reddevilmidzy Apr 25, 2024

jinyoungchoi95 Apr 25, 2024

jinyoungchoi95 Apr 25, 2024

reddevilmidzy Apr 26, 2024

reddevilmidzy Apr 26, 2024

jinyoungchoi95 Apr 25, 2024

jinyoungchoi95 Apr 25, 2024

jinyoungchoi95 Apr 25, 2024

jinyoungchoi95 Apr 26, 2024

reddevilmidzy Apr 28, 2024

jinyoungchoi95 Apr 25, 2024

jinyoungchoi95 Apr 25, 2024

reddevilmidzy Apr 25, 2024

jinyoungchoi95 left a comment

jinyoungchoi95 Apr 26, 2024

reddevilmidzy Apr 28, 2024

jinyoungchoi95 Apr 26, 2024

jinyoungchoi95 Apr 26, 2024

reddevilmidzy Apr 28, 2024

jinyoungchoi95 Apr 26, 2024

reddevilmidzy Apr 29, 2024

jinyoungchoi95 Apr 26, 2024

jinyoungchoi95 Apr 26, 2024

jinyoungchoi95 Apr 26, 2024

reddevilmidzy commented Apr 29, 2024

jinyoungchoi95 left a comment

jinyoungchoi95 Apr 29, 2024

jinyoungchoi95 Apr 29, 2024

		final ReservationTime created = ReservationTime.create(timeRequest.startAt());
		final ReservationTime saved = timeDao.save(created);

	public ReservationResponse save(final ReservationRequest requestV2) {
	public ReservationResponse save(final ReservationRequest request) {

		@@ -0,0 +1,4 @@
		package roomescape.dto;

		public record TimeRequest(String startAt) {

	this.id = null;
	this.startAt = LocalTime.parse(startAt);
	�this(null, LocalTime.parse(startAt));

[7 - 9단계 방탈출 예약 관리] 레디(최동근) 미션 제출합니다. #158

[7 - 9단계 방탈출 예약 관리] 레디(최동근) 미션 제출합니다. #158

Conversation

reddevilmidzy commented Apr 25, 2024

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddevilmidzy Apr 26, 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

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

jinyoungchoi95 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

reddevilmidzy commented Apr 29, 2024

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddevilmidzy Apr 26, 2024 •

edited

Loading