[1 - 3단계 - 방탈출 예약 관리] 알파카(최휘용) 미션 제출합니다. #18

slimsha2dy · 2024-04-16T13:04:49Z

안녕하세요 썬, 알파카입니다!
레벨 1 첫 미션 때도 썬의 리뷰를 받았었는데 레벨 2 첫 미션에서도 다시 받을 수 있게 되어 영광입니다.. 🙇
이번 미션도 잘 부탁드립니다!!

스프링 및 웹 경험

스프링 및 웹 개발 경험은 전혀 없습니다

syoun602

안녕하세요 알파카 리뷰어 썬입니다 ☀️

또 만나게 되어서 반갑습니다~ 1단계 빠르게 요청 주셨네요!
몇 가지 코멘트 남겨보았으니 한번 학습해보시고 재요청 주세요 😊

syoun602 · 2024-04-17T15:09:26Z

README.md

@@ -0,0 +1,11 @@
+# 기능 구현 목록


API가 4개가 있는 것 같은데, 문서에 다 정의해봅시다 😊

API 요구사항인 예약 조회, 추가, 취소, 예약 관리 페이지 호출의 API 명세서를 추가했습니다.

syoun602 · 2024-04-17T15:09:38Z

src/main/java/roomescape/ReservationInfo.java

+    private String time;
+
+    public ReservationInfo() {
+


개행을 의도하신걸까요?

의도한 것은 아니었습니다. 불필요한 개행인 것 같아 수정했습니다.

syoun602 · 2024-04-17T15:10:26Z

src/main/java/roomescape/ReservationInfo.java

+    public static ReservationInfo toEntity(ReservationInfo reservationInfo, Long id) {
+        return new ReservationInfo(id, reservationInfo.name, reservationInfo.date, reservationInfo.time);
+    }


toEntity를 사용하신 의도가 궁금합니다. Entity는 무엇인가요?

Entity란 테이블과 직접적으로 연결되는 클래스이고 하나의 인스턴스가 데이터베이스 테이블의 한 row에 해당됩니다.
현재 코드에선 reservationInfos가 DB의 역할을 하고 있습니다.
컨트롤러에서 @ResquestBody를 통해 받은, id가 비어있는 reservationInfo에 id를 부여하는 기능이 데이터베이스에 연결되는 entity로 만드는 것이라고 생각해 toEntity라는 네이밍을 사용하게 되었습니다.

syoun602 · 2024-04-17T15:11:05Z

src/main/java/roomescape/RoomescapeController.java

+
+@Controller
+public class RoomescapeController {
+


ReservationInfo 는 첫 줄 개행이 없는데, 알파카만의 컨벤션이 있을까요?

페어 프로그래밍을 하다 보니 놓친 부분인 것 같습니다. 전부 첫 줄에 개행이 없도록 수정했습니다!

syoun602 · 2024-04-17T15:11:55Z

src/main/java/roomescape/RoomescapeController.java

+import java.util.List;
+import java.util.concurrent.atomic.AtomicLong;
+
+@Controller


Controller와 RestController는 어떤 차이가 있는 지 학습해보고 공유해주세요!

@RestController는 @Controller에 @ResponseBody가 추가된 것입니다. 실제로 @RestController의 인터페이스를 확인해보니 @Controller와 @ResponseBody 어노테이션이 포함되어 있는 것을 알 수 있었습니다.
기존의 Controller는 String 형태의 ViewName을 반환하며, json 형태의 데이터를 반환하기 위해선 @ResponseBody를 사용해야 합니다. @ResponseBody 어노테이션을 사용하면 http 메시지의 바디에 메서드의 반환값을 json의 형태로 serialize하여 넣어주고 이를 응답하게 됩니다.
그러므로 RestController는 항상 json 형태의 데이터를 반환하는 컨트롤러, 즉 데이터를 응답하는 REST API를 처리하기 위한 컨트롤러입니다.
기존 코드에선 사용할 일이 없다고 생각했는데 아래에 남겨주신 리뷰를 보니 RestController와 Controller를 분리하여 @ResponseBody의 중복을 줄이고 책임을 분리할 수 있을 것 같네요.

syoun602 · 2024-04-17T15:12:47Z

src/main/java/roomescape/RoomescapeController.java

+    private List<ReservationInfo> reservationInfos = new ArrayList<>();
+    private AtomicLong counter = new AtomicLong();
+
+    @GetMapping("/admin")


admin API와 실제 예약에 관한 API가 하나의 컨트롤러에 섞여있는데, 분리해볼 수는 없을까요? 🤔

두 컨트롤러로 분리하고 실제 예약에 관한 API에 @RestController를 적용했습니다!

syoun602 · 2024-04-17T15:13:19Z

src/main/java/roomescape/ReservationInfo.java

+
+public class ReservationInfo {
+    private Long id;
+    private String name;


여기는 원시값이 사용되고 있네요! 레벨 1에서 학습한 것 내용을 적용해보면 어떨까요~?

id와 name을 포장하는 클래스를 구현하고, date와 time은 LocalDate와 LocalTime을 사용하도록 수정했습니다.
이렇게 포장을 할 경우, 이 도메인을 그대로 @ResponseBody를 통해 json으로 넘기면 원하는 형태로 넘길 수가 없게 되는 문제가 발생하게 되었습니다. 따라서 id와 name을 원시값으로 넘기는 dto인 ReservationResponse를 추가로 구현하였습니다.

syoun602 · 2024-04-17T15:14:22Z

src/test/java/roomescape/MissionStepTest.java

                .then().log().all()
                .statusCode(200);
    }

+    @Test
+    void 이단계() {


어떤 테스트인지 작성해보면 좋을 것 같아요. 메서드명이든 DisplayName이든 테스트의 의도를 나타내주세요!

요구 사항에 있는 테스트를 그대로 쓰다 보니까 이렇게 됐네요.. 메서드 명을 수정하고 DisplayName을 추가했습니다!

syoun602 · 2024-04-17T15:15:12Z

src/test/java/roomescape/MissionStepTest.java

+import java.util.Map;
+
+import static org.hamcrest.Matchers.is;
+
 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)


DefinedPort를 사용하신 이유가 궁금합니다. 이렇게 사용하게되면 어떤 문제가 생길 수 있을 지 고민해보아요

따로 이유가 있어서 사용한 것은 아니고 처음 포크 받았을 때 있는 테스트를 그대로 쓰다 보니까 사용하게 되었습니다.
그런데 테스트가 DEFINED_PORT인 8080 포트를 사용하게 되면 Application 실행 시 테스트를 실행할 수 없게 되는 문제가 있었네요.
이를 해결하기 위해 찾아보고 여러 삽질을 해본 결과, @SpringBootTest에서 RANDOM_PORT를 사용하도록 하고, 기본적으로 8080 포트에 요청을 보내는 RestAssured의 포트 설정을 위의 RANDOM_PORT와 맞춰 주어 해결할 수 있었습니다.

syoun602

안녕하세요 알파카 👋

리뷰 요청 빠르게 주셨네요!
몇 가지 코멘트 더 남겨두었으니 확인 후 재요청주세요 😊

syoun602 · 2024-04-18T12:22:49Z

src/main/java/roomescape/controller/RoomescapeAdminController.java

+import org.springframework.web.bind.annotation.GetMapping;
+
+@Controller
+public class RoomescapeAdminController {


syoun602 · 2024-04-18T12:24:20Z

src/main/java/roomescape/controller/RoomescapeRestController.java

+
+@RestController
+public class RoomescapeRestController {
+    private final List<ReservationInfo> reservationInfos = new ArrayList<>();


Info, Data와 같은 의미가 불분명한 용어를 변수로 사용하는 것은 추천되지 않습니다~
차라리 Reservation으로 변경하고 해당 리스트는 reservations로 변경해보면 좋을 것 같아요.

syoun602 · 2024-04-18T12:26:09Z

src/main/java/roomescape/controller/RoomescapeRestController.java

+    public ReservationResponse addReservationInfo(@RequestBody ReservationInfo reservationInfo) {
+        ReservationInfo newReservationInfo = ReservationInfo.toEntity(reservationInfo, counter.incrementAndGet());
+        reservationInfos.add(newReservationInfo);
+        return new ReservationResponse(newReservationInfo);


이렇게 반환된 값을 response로 포장했다면, request에 대해서도 만들어보면 어떨까요?

syoun602 · 2024-04-18T12:27:31Z

src/main/java/roomescape/domain/ReservationInfo.java

    public static ReservationInfo toEntity(ReservationInfo reservationInfo, Long id) {
-        return new ReservationInfo(id, reservationInfo.name, reservationInfo.date, reservationInfo.time);
+        return new ReservationInfo(new Id(id), reservationInfo.name, reservationInfo.date, reservationInfo.time);
    }


이미 public 생성자가 존재하는데 굳이 지금과 같은 정적 팩터리 메서드가 필요한지 잘 모르겠습니다 🤔

사용하는 곳에서의 편의성을 고려했었는데 굳이 정적 팩토리 메서드를 사용할 필요가 없어보이긴 하네요.
요청에 대한 dto를 만들면서 삭제되었습니다!

syoun602 · 2024-04-18T12:28:11Z

src/main/java/roomescape/dto/ReservationResponse.java

+    public ReservationResponse(ReservationInfo reservationInfo) {
+        this.id = reservationInfo.getId().getId();
+        this.name = reservationInfo.getName().getName();
+        this.date = reservationInfo.getDate();
+        this.time = reservationInfo.getTime();
+    }


ReservationInfo를 바로 받기보다는 기본 생성자를 두고, 부 생성자를 활용해 만들어볼 수 있을 것 같아요 😁

생성자 체이닝을 사용하도록 수정해봤습니다.
그런데 현재 상황처럼 생성자가 하나인 경우에 생성자 체이닝을 사용하는 것이 확장성을 제외하고 어떤 면에서 이점이 있는지 궁금합니다!

저는 보통 '온전한 객체'를 보장하기 위해 주생성자를 항상 두는 편이라서 말씀드렸습니다!
부생성자를 둠으로써 객체 생성이 복잡해지더라도 this를 이용해 온전한 객체를 믿고 만들어줄 주생성자를 사용해볼 수 있을 것 같아요

syoun602 · 2024-04-18T12:28:22Z

src/test/java/roomescape/RoomescapeAdminControllerTest.java

+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.web.server.LocalServerPort;
+
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)


syoun602 · 2024-04-18T12:29:01Z

README.md

+
+***
+
+# API 명세


API 명세서까지 추가하셨네요 좋습니다 👍👍👍

syoun602 · 2024-04-18T12:31:11Z

src/main/java/roomescape/domain/Id.java

+
+import java.util.Objects;
+
+public class Id {


ID는 사실 데이터베이스를 위해 도메인에 추가된거라 특별한 로직이 필요한게 아니라면 포장하지 않았을 것 같아요!
지난 리뷰에서 함께 라인이 추적되서 만든거라면 롤백되어도 괜찮을 것 같습니다 🙂

특별한 로직이 없으니 굳이 있는 것이 더 어색하긴 하네요. 롤백하였습니다!

syoun602 · 2024-04-18T12:31:41Z

src/main/java/roomescape/domain/Name.java

@@ -0,0 +1,13 @@
+package roomescape.domain;
+
+public class Name {


validation과 같은 로직이 있으면 좋을 것 같아요! 명확한 정책이 없다면 문서에 작성한 후 알파카가 만들어봐도 좋구요 👀

이름 정책을 만들어서 예외 처리를 구현했습니다!

syoun602

안녕하세요 알파카!
빠르게 리뷰 반영해주셨네요 👍
이번 단계는 충분히 진행한 것 같아 머지하겠습니다~
다음 단계에서 뵐게요 💪

syoun602 · 2024-04-20T03:41:10Z

src/test/java/roomescape/domain/NameTest.java

+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class NameTest {


Name에 대한 테스트 추가까지 👍

syoun602 · 2024-04-20T03:41:44Z

src/main/java/roomescape/domain/Name.java

 public class Name {
+    private static final Pattern PATTERN = Pattern.compile("[가-힣a-zA-Z-_0-9]+");
    private final String name;

    public Name(String name) {


잘만들어주셨네요! 이런 부분도 README 에 추가되면 좋을 것 같아요 🙂

syoun602 · 2024-04-20T03:47:10Z

src/main/java/roomescape/dto/ReservationResponse.java

+    public ReservationResponse(ReservationInfo reservationInfo) {
+        this.id = reservationInfo.getId().getId();
+        this.name = reservationInfo.getName().getName();
+        this.date = reservationInfo.getDate();
+        this.time = reservationInfo.getTime();
+    }


저는 보통 '온전한 객체'를 보장하기 위해 주생성자를 항상 두는 편이라서 말씀드렸습니다!
부생성자를 둠으로써 객체 생성이 복잡해지더라도 this를 이용해 온전한 객체를 믿고 만들어줄 주생성자를 사용해볼 수 있을 것 같아요

jhon3242 added 6 commits April 16, 2024 19:15

docs : 기능 구현 목록 작성

03138d4

feat : 어드민 페이지 구현 완료

1e9569d

docs : step2 기능 구현 목록 작성

b1ccfc9

feat : 2단계 기능 구현

da61e88

docs : 3단계 기능 구현 목록 작성

e3309fe

feat : 3단계 기능 구현

6ee6202

syoun602 requested changes Apr 17, 2024

View reviewed changes

slimsha2dy added 10 commits April 18, 2024 13:46

test: 테스트 포트 번호 변경

b3aa67c

docs: API 명세 추가

a5cc15c

style: 기본 생성자의 불필요한 개행 삭제

975888c

style: 클래스 첫 줄에 개행이 없도록 수정

c04967d

docs: API 명세 추가

2a969e3

refactor: 컨트롤러 분리

fcae9f6

docs: 코드블럭 오류 수정

b5b0b01

test: 테스트 분리 및 메서드 명 수정

6de5ef1

refactor: ReservationInfo의 원시값 필드 포장

2faf2b9

feat: 예약 정보를 응답할 때 사용하는 dto 구현

4e5eee2

syoun602 requested changes Apr 18, 2024

View reviewed changes

slimsha2dy added 5 commits April 19, 2024 15:30

rename: ReservationInfo 클래스명 수정

cdb0736

refactor: 예약 요청을 위한 dto 구현

7c8fcba

refactor: 포장 객체 Id 삭제

afb1751

refactor: 주 생성자와 부 생성자 분리

5dcfd91

feat: 이름 클래스 예외 처리 구현

793b8b0

syoun602 approved these changes Apr 20, 2024

View reviewed changes

syoun602 merged commit 53395f0 into woowacourse:slimsha2dy Apr 20, 2024

		@@ -0,0 +1,13 @@
		package roomescape.domain;

		public class Name {

[1 - 3단계 - 방탈출 예약 관리] 알파카(최휘용) 미션 제출합니다. #18

[1 - 3단계 - 방탈출 예약 관리] 알파카(최휘용) 미션 제출합니다. #18

Conversation

slimsha2dy commented Apr 16, 2024

스프링 및 웹 경험

syoun602 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimsha2dy Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimsha2dy Apr 18, 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

syoun602 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syoun602 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

slimsha2dy Apr 18, 2024 •

edited

Loading

slimsha2dy Apr 18, 2024 •

edited

Loading