[4 - 9단계 방탈출 예약 관리] 미아(이종미) 미션 제출합니다. #95

jongmee · 2024-04-21T08:30:18Z

안녕하세요 수달! 이번 PR도 잘 부탁드립니다 ☺️

리팩토링된 부분

Service layer 메소드 DTO 사용

수달의 저번 코멘트를 보고 더 고민해보았습니다. Service layer에서 도메인 객체를 반환하면 해당 객체가 변경될 수 있다는 점과 더불어 객체의 정보를 다양한 형태로 반환해야 할 경우 DTO를 사용하면 Controller가 여러 Service에 의존하지 않아도 된다는 점에서 반환값에서 DTO를 사용하도록 변경하였습니다.

고민되는 부분

기능 요구사항에는 "기존에 사용하던 List(Collection) 및 AtomicLong 을 제거하고, 데이터베이스를 활용한다"라고 되어 있지만, 기존에 사용하던 Collection Repository(즉, AtomicLong과 ConcurrentHashMap을 사용하던 Repository)를 제거하기 보다 기존 코드에 영향을 주지 않고 새로운 Repository를 도입하는 것이 더 재사용성이 있는 코드라고 생각하였습니다.

따라서 Repository 인터페이스를 생성하고 Jdbc를 활용하는 레포지토리를 구현체로 추가하였습니다.
이 때 고민되었던 점은, Repository와 DAO의 사용입니다. JdbcTemplate으로 DB connection을 얻어 쿼리를 날리는 부분은 DAO에 가깝다고 생각하여(Repository와 DAO의 개념을 참고한 사이트) ReservationDao와 ReservationTimeDao에서 이 부분을 담당하고, DAO 메소드들을 JdbcRepository(ReservationJdbcRepository, ReservationTimeJdbcRepository)에서 사용하고 있습니다. Optional로의 변환이나 저장된 객체 반환(save 메소드)을 Repository에서 해주고 있지만, 불필요한 클래스 분리일지 고민됩니다. 수달의 의견을 듣고 싶습니다!

감사합니다 :)

…API 상태코드 변경

…dbc 사용 레포지토리 클래스명 변경

her0807

안녕하세요 미아! ㅎㅎ

리뷰어 수달입니다!

이번 4-9 단계도 깔끔하게 정말 잘 구현해주셨는데요!
그래서 학습할 키워드와 질문들을 남겨보았어요.
천천히 학습해주시고, 궁금한 점 남겨주세요 👍🏻

her0807 · 2024-04-22T11:28:24Z

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

+
+    @PostMapping
+    public ResponseEntity<ReservationTimeResponse> createReservationTime(@RequestBody ReservationTimeSaveRequest request) {
+        var reservationTime = request.toModel();


var 는 어떨 때 사용할까요?

var는 지역변수에서 사용할 수 있고 컴파일 시점에 타입을 추론합니다. 저는 ReservationTime reservationTime = request.toModel()같이 타입과 변수명이 일치하는 경우에 var를 사용해서 코드를 간결하게 쓰고자 하였습니다. (타입이 꼭 필요하지 않다고 생각했습니다>!)

그렇군요! ㅎㅎ
타입 추론을 사용하게 되면 다른 사람과 함께 코드작업을 할때
해당 메서드의 반환형태가 무엇인지 불명확하여 파악하기 위해 1 뎁스 더 들어가야하는게
조금 비용인것 같긴합니다!
이런 관점도 있다는것만 인지하시고 넘어가셔도 될 것 같아요~

her0807 · 2024-04-22T11:29:29Z

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

+    public ReservationTimeController(ReservationTimeService reservationTimeService) {
+        this.reservationTimeService = reservationTimeService;
+    }


의존성을 생성자 주입하면 어떤 이점이 있을까요?

객체가 생성되는 동시에 의존성이 생기고 불변(final이기에 재할당 불가능)이기 때문에 안전합니다. NPE 등을 방지할 수 있습니다. 또한 같은 이유로 스프링 컨테이너 없이도 의존성을 주입하여 쉽게 테스트할 수 있습니다. Mocking하기도 쉽습니다! 추가로, 순환 참조를 방지할 수 있습니다.

장점을 잘알고 사용하시는군요 💯

src/main/java/roomescape/dao/ReservationDao.java

her0807 · 2024-04-22T11:35:51Z

src/main/java/roomescape/dao/ReservationDao.java

+        jdbcTemplate.update(sql, id);
+    }
+
+    private Reservation rowMapper(ResultSet resultSet, int rowNumber) throws SQLException {


SQLException 은 어떤식으로 핸들링 해볼 수 있을까요?

checked Exception 일까요 unChecked Exception 일까요?

SQLException은 checked Exception 입니다. 런타임 예외를 던져주어 런타임에 발생하는 예외 사항을 상위에서 핸들링해줄 수 있을 것 같습니다. 현재는 레포지토리에 @Repository 애너테이션을 붙여 DataAccessException으로 처리됩니다!

her0807 · 2024-04-22T11:48:13Z

src/main/resources/schema.sql

+CREATE TABLE reservation_time
+(
+    id       BIGINT       NOT NULL AUTO_INCREMENT,
+    start_at VARCHAR(255) NOT NULL,


시간을 저장하는 DB Type 은 무엇이있을까요?

TIME이 있습니다! 타입을 보장해주는 게 안전할 것 같아 시간과 날짜는 각각 TIME과 DATE로 저장하도록 수정하였습니다 :)

타입에 대한 고민은 실무에서 굉장히 중요하다고 느껴요!

현재 날짜와 시간을 따로 저장하고 있지만 비지니스 로직 상에서는 두가지가 하나의 세트로 핸들링되고 있어요.
DATETIME 으로 저장한다면 ? 별도로 묶어주는 작업을 덜어낼 수 있을거예요.

(이렇게 변경하라는 건 아니고요! 타입에 대해서도 슬쩍 고민해보면 좋을 것 같다는 의견이었고, 현재 상태에서 적절히 반영된 것 같아요)

her0807 · 2024-04-22T11:48:41Z

src/test/java/roomescape/TestFixture.java

    public static final LocalTime MIA_RESERVATION_TIME = LocalTime.of(15, 0);

-
    public static final String USER_TOMMY = "토미";


오 ㅎㅎ 미아 토미조이신가요? 😁

네 맞습니다 ㅎㅎ 😆

저도 토미조였는데요 ㅎㅎ 내일 데일리 때 잘 계신지 안부전해주세요!!

her0807 · 2024-04-22T11:58:12Z

src/main/java/roomescape/repository/ReservationCollectionRepository.java

+    @Override
+    public Optional<Reservation> findById(Long id) {
+        return Optional.ofNullable(reservations.get(id));
+    }


Dao 로 쿼리들을 모아놓으셨군요 잘하셨습니다.

dao 는 항상 repository 랑 비교되는데요

관련한 김영한님 코멘트 남겨둡니다!

이 둘은 거의 같다고 생각하셔도 무방합니다. 좀 더 깊이있게 차이를 설명하면, repotiroy는 엔티티 객체를 보관하고 관리하는 저장소이고, dao는 데이터에 접근하도록 DB접근 관련 로직을 모아둔 객체입니다. 둘다 개념의 차이일뿐 실제로 개발할 때는 비슷하게 사용됩니다. 🙂

Q, Optional로의 변환이나 저장된 객체 반환(save 메소드)을 Repository에서 해주고 있지만, 불필요한 클래스 분리일지 고민됩니다. 수달의 의견을 듣고 싶습니다!

저는 데이터를 조회 한 후 해당 데이터 유무에 따른 예외 처리나 필터링 등등에 대한 행위를 하고 계신것 같아서 repotiroy 를 데이터 보관, 관리 용도로 사용하고 Dao 를 DB 와 접근하는 용도로 적절히 사용하셨다고 생각합니다.

그러나 optional 을 사용하는 것은 기존 객체를 한번더 래핑하는 것이기 때문에 조금 더 고민을 해보면 좋을 것 같아요!

find vs get

해당 메서드명을 통해서 값이 없으면 예외를 반환, 없으면 빈 값을 반환을 명시해둘 수 있는데요.
정책적으로 빈 값이 있으면 안된다! 라고 약속하면 옵셔널을 사용하지 않고 해당 로직에서 바로 예외를 반환할 수도 있을거예요.

저는 개발을 할 때 개발자 스스로 기획자가 정의한 요구사항에 누락된 점이나 보호해야하는 부분에 대해서는
최대한 꼼꼼히 도출하는 편인데요. 이번 기회에 미아도 레포지토리 형태를 어떤 식으로 가져갈지
직접 요구사항을 정의해보고 적용해보면 좋을 것 같아요.

get을 사용하면 대게 예외를 반환하기 보다 빈 값을 반환하는군요. 그 컨벤션은 몰랐습니다🥲 (getter를 생각해보니 이해가 되네요) Controller와 Repository의 메소드명도 get 대신 find로 바꾸었습니다.

예외를 던지는 것은 Repository보다 Service layer의 책임인 것 같아서 Repository의 Optional은 남겨두었습니다! 또한 findById 에서 데이터를 찾지 못할 경우 올바르지 않은 요청(잘못된 Id 값)을 한 것이므로 빈 값을 넘겨주는 것보다 예외를 반환하는게 옳다고 생각해서 Optional을 사용하였습니다.

Service layer 에서 예외처리를 위임하게되면 repository 의 재사용성이 올라가겠네요.
예를 들면 사용자 검색용으로 조회 할 때 값이 없으면 빈값을 내려줘야하는데 예약을 생성하기 위해 사용자를 조회했을 때는
없으면 예외처리를 해줘야하는데 그것을 하나의 repository 로 처리할 수 있게 되니까요 👍🏻

her0807 · 2024-04-22T12:00:04Z

src/test/java/roomescape/acceptance/AcceptanceTest.java

+import org.springframework.test.annotation.DirtiesContext;
+
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
+@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)


@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)
Comment 는 어떤 원리로 SpringTestContainer 를 초기화해줄까요?

각 테스트마다 Context cache에서 ApplicationContext를 삭제하고 새로운 context에서 테스트가 실행되도록 합니다.

for example, by modifying the state of a singleton bean, modifying the state of an embedded database, etc.
현재 Repository와 Acceptance 테스트에서 해당 어노테션을 사용하고 있는데, 싱글톤 빈들은 상태를 갖고 있지 않고 embedded db인 h2만 초기화하면 되는 상황입니다.

Context cache를 찾아보니 아래와 같이 테스트에서 context를 캐싱하는 것은 성능상 중요한 포인트였네요..!

Support for the caching of loaded contexts is important, because startup time can become an issue — not because of the overhead of Spring itself, but because the objects instantiated by the Spring container take time to instantiate.

따라서 @DirtiesContext 대신 DB를 초기화해주는 DDL을 각 테스트 전에 실행하도록 변경하였습니다 :)

her0807 · 2024-04-22T12:00:27Z

src/test/java/roomescape/acceptance/AcceptanceTest.java

+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.test.annotation.DirtiesContext;
+
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)


스프링을 사용하면 어떤점이 좋은가요?

스프링은 IoC 컨테이너로 빈의 생명 주기와 의존성을 관리해 줍니다. 개발자가 직접 관리하지 않습니다. 코드가 유연하고 확장성이 있습니다.

API 통합 테스트는 요청을 했을 때 모든 프로덕션 코드를 거쳐 응답까지 테스트해야 하는데, @SpringBootTest로 스프링 컨테이너를 사용해서 테스트하여 모든 설정과 빈들을 테스트할 수 있습니다!

추가로 보면 좋은 점이 @SpringBootTest 내부에 설정들인데요!
webEnvironment 에 아무 설정을 안해주면 기본값은 MOCK이라고 해요. WebApplicationContext를 로드하지만, 내장된 서블릿 컨테이너가 아닌 MOCK 서블릿을 제공해서 실제 서블릿 환경에서 테스트를 진행해보고 싶다면 RANDOM_PORT나 DEFINED_PORT로 진행해주어야 한답니다.. RANDOM_PORT랑 DEFINED_PORT는 위 사진에서도 볼 수 있듯이 embedded값이 true여서 EmbeddedWebApplicationContext를 로드하기 때문입니다!! ㅎㅎ

이런 설정들을 보는 것도 참 재밌는 것 같아요

@SpringBootTest를 쓰면서도 처음 알았네요..!! 공부해보겠습니다 감사합니다 👍

jongmee · 2024-04-24T13:25:07Z

안녕하세요 수달! 리뷰 요청 드립니다 ☺️ 리뷰에 코멘트들을 남겨두었고, 리뷰 반영하여 추가 커밋하였습니다!

새로운 기능 추가(feature)는 1) 예외를 처리하면서 추가한 RestControllerAdvice와 잊고 있던 @Transactional 입니다. 🥲 지금 미션에서 도입해야 할 지 약간 망설였지만 미션도 하나의 프로그램으로 봤을 때 필요한 부분인 것 같아 추가하였습니다 ! 잘 부탁드립니다 🙇🏻‍♀️

her0807 · 2024-04-25T04:10:00Z

src/test/resources/test-schema.sql

+DROP TABLE IF EXISTS reservation;
+DROP TABLE if EXISTS reservation_time;
+


drop vs delete vs trancate

같은 ddl 중에 trancate 라는 것도 있는데요.
현재는 테이블을 제거했다가 다시 만들고 있는데
테이블이 생성될 때는 생각보다 여러가지 일이 일어나요.
데이블에 인덱스가 있다면 인덱스도 생성해줘야하고 연관관계가 맺어져있다면
또 순서가 중요해질 수도 있고요.

실무에서 또 중요한 건 롤백이 가능한 구조인지 확인하는데요.
테스트 DB 가 devel DB 에 연결되어 있는 경우가 많기 때문에
Drop 해 버리면 예상치 못한 개발 환경에 장애를 유발할 수도 있답니다!

그래서 되도록 drop 은 지양하고 delete row 나 truncate 를 추천드립니다!

유용한 추천 감사합니다 !! drop table의 단점을 몰랐는데 확실히 와닿네요. 다음 미션 때부터는 delete row 나 truncate 를 꼭 적용해보아야 겠어요 ☺️

her0807 · 2024-04-25T04:15:29Z

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

+    @ExceptionHandler(DataAccessException.class)
+    public ResponseEntity<ErrorResponse> handleDataAccessException(DataAccessException e) {
+        return ResponseEntity.internalServerError()
+                .body(new ErrorResponse("[DATA ACCESS ERROR] " + e.getMessage()));
+    }


DataAccessException 같은 경우는 관련된 예외 메세지가 바로 반환되면 보안에 취약할 수 있습니다.
그래서 실무에서는 로그만 남기고 예외를 한번 감싸서 반환하곤 합니다!

her0807 · 2024-04-25T04:15:59Z

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

+import org.springframework.web.bind.annotation.RestControllerAdvice;
+import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
+
+@RestControllerAdvice


오~ GlobalExceptionHandler 👍🏻

her0807

안녕하세요 미아!

리뷰 반영해주신 코드 잘 보았습니다.
주로 개념에 대한 질문들이 많았는데요. ㅎㅎ
이번 리뷰를 통해 복습이 잘 되었으면 좋겠네요 🙂
요구사항들은 모두 충족된 것 같아 이만 머지하겠습니다!
참고로 주의할 점에 대한 코멘트 몇가지 남겨두었으니 확인부탁드립니다.

수고하셨어요.

jongmee · 2024-04-30T02:18:03Z

수달 리뷰를 보고 이것 저것 정리하느라 인사가 늦었네요 🥲 추가로 공부할 부분, 주의할 점 남겨주신 것 꼼꼼히 더 공부해보겠습니다 ! 정말 감사합니다 😊 !!!

jongmee added 30 commits April 19, 2024 14:13

refactor(ReservationSaveRequest): model 변환 메소드명 변경

44af3c3

refactor(ReservationService): 검증 로직 메소드 분리

44492a4

refactor(TestFixture): 반복 생성되는 객체들 추출

64f8664

remove(application): 불필요한 properties 삭제

4ccb259

test(AcceptanceTest): e2e 테스트 클래스명 변경

8cf4500

feat(application): h2 db 연결

06d2151

refactor(ReservationRepository): ReservationRepository 추상화

1d845a8

refactor: 패키지 이름 layered architecture에 맞게 변경

869b1b8

feat(ReservationDbRepository): database와 연결된 repository 생성

65b5212

feat(ReservationDbRepository): 데이터베이스를 활용해 데이터 조회

1fb772f

feat(ReservationDbRepository): 데이터베이스를 활용해 데이터 삽입

856f325

feat(ReservationDbRepository): 데이터베이스를 활용해 데이터 조회 및 삭제

e0c8696

test(ReservationDbRepositoryTest): TestFixture 사용

6fe8a28

test(LocalConnectionTest): 불필요 테스트 제거

b4295cd

test(ReservationDbRepositoryTest): 잘못된 sql 수정

95f1de4

fix(ReservationDbRepository, Reservation): 예약 생성 시 id 생성된 새 객체 반환

9e49db9

fix(ReservationController): POST API 상태코드 201로 변경

15d6755

fix(ReservationController): DELETE API 상태코드 204로 변경

a9657d0

feat(AdminController): 시간 관리 페이지 반환

e413470

test(ReservationAcceptanceTest): AcceptanceTest 분리

36018f2

refactor(ReservationService): 반환값 model에서 dto로 변경

8c956f3

refactor(ReservationService): 타입과 변수명이 중복되는 지역 변수 var 타입으로 변경

a5e4661

feat(ReservationTimeController): 예약 시간 생성 presentation layer 구현

3bc152a

feat(ReservationTimeService): 예약 시간 생성 business layer 구현

86501d6

feat(ReservationTimeRepository): 예약 시간 생성 persistence layer 구현

28d0578

test(ReservationTimeAcceptanceTest): 예약 시간 생성 API 테스트

0d6ec29

fix(ReservationResponse): 예약 시간 포매팅

b6178e9

feat(ReservationTimeController): 예약 시간 목록 조회 presentation layer 구현

44d5cc9

feat(ReservationTimeService): 예약 시간 목록 조회 business layer 구현

450c462

test(ReservationTimeServiceTest): 예약 시간 비교 타입 수정

5df1d58

jongmee added 13 commits April 21, 2024 13:35

fix(AdminController): 예약 페이지 html 변경

0d46338

fix(ReservationController, ReservationService): 예약 추가 API 응답값, 상태코드 변경

07777c4

fix(ReservationController, ReservationTimeController): 예약 및 예약 시간 삭제 …

3344861

…API 상태코드 변경

fix(ReservationTimeController): 예약 시간 추가 API 응답값, 상태코드 변경

3ff0b2b

fix(ReservationDao): 빈 등록 애너테이션 변경

af27d04

refactor: controller 및 service 메소드명, 순서 변경

243f914

test(ApiAcceptanceTest): API e2e 테스트 리팩토링

dc43f82

test(ReservationDbRepositoryTest): 불필요한 데이터 삭제 쿼리 제거

f7796df

style(ReservationDao, ReservationTimeDao): sql 포매팅

58c7ec6

refactor(ReservationJdbcRepository, ReservationTimeJdbcRepository): J…

d2593ff

…dbc 사용 레포지토리 클래스명 변경

docs(README): API 명세 수정

b485b2c

refactor: 패키지명 수정

7caed94

test(ServiceTest): Mockito에서 BDDMockito로 변경

c555e73

her0807 requested changes Apr 22, 2024

View reviewed changes

jongmee added 9 commits April 23, 2024 16:52

refactor(ReservationDao, ReservationTimeDao): SimpleJdbcInsert로 데이터 삽입

fe71692

fix(ReservationTimeDao): Id에 해당하는 예약 시간이 없다면 null 반환

12075ca

feat(GlobalExceptionHandler): 전역 예외 처리(Controller Advice) 추가

83c6361

fix(ReservationDao): Id에 해당하는 예약이 없다면 빈 Optional 반환

8d60550

feat(NotFoundException): 404 반환 추가

c00ec48

fix(schema.sql): column 속성 변경

7a8c3bd

test: DirtiesContext 애너테이션 제거, 테스트시 DB 초기화 DDL 추가

e5df87a

feat(ReservationService, ReservationTimeService): 스프링 트랜잭션 추가

838e1a2

refactor: controller와 service 조회 메소드명 변경

61f2581

her0807 reviewed Apr 25, 2024

View reviewed changes

her0807 approved these changes Apr 25, 2024

View reviewed changes

her0807 merged commit da6903d into woowacourse:jongmee Apr 25, 2024

		public static final LocalTime MIA_RESERVATION_TIME = LocalTime.of(15, 0);


		public static final String USER_TOMMY = "토미";

		DROP TABLE IF EXISTS reservation;
		DROP TABLE if EXISTS reservation_time;

[4 - 9단계 방탈출 예약 관리] 미아(이종미) 미션 제출합니다. #95

[4 - 9단계 방탈출 예약 관리] 미아(이종미) 미션 제출합니다. #95

Uh oh!

Conversation

jongmee commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

리팩토링된 부분

Service layer 메소드 DTO 사용

고민되는 부분

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jongmee Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

find vs get

Uh oh!

jongmee Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jongmee Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jongmee commented Apr 24, 2024

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!

her0807 left a comment

Choose a reason for hiding this comment

jongmee commented Apr 21, 2024 •

edited

Loading

jongmee Apr 24, 2024 •

edited

Loading

jongmee Apr 24, 2024 •

edited

Loading

jongmee Apr 24, 2024 •

edited

Loading