[Spring JDBC] 남해윤 미션 제출합니다. #379

haeyoon1 · 2024-11-13T17:03:23Z

안녕하세요 송은우 리뷰어님!
늦게 pr을 올리게 되었는데 늦은만큼 코드 수정은 더 성실히 해보도록 하겠습니다! 아낌없는 리뷰 부탁드려요~

그리고 Spring 미션에서의 패키지 나누기관련해서 드릴 질문이 있습니다.
저는 이전 자바 미션에서는 항상 domain, controller, view 세개로 패키지를 나누어서 코드를 작성하였습니다.(그게 꼭 맞는 코드는 아니겠지만요...) 다른분들의 코드도 보면서 그렇게 자바미션 코드의 구조를 잡게 된것이었는데, 스프링 미션은 아직 익숙하지 않아 패키지를 제대로 나누지 못한 것 같습니다. 대부분의 분들이 service 패키지도 있고, 다른 패키지들도 더 세세하게 나누신 것 같은데 코드의 정답은 없다지만 혹시 권장하는 방법이 있으신가요??

제가 JDBC에 대해 학습한 내용이 많이 없어서 의미 있는 질문을 pr에 올리지 못한 것 같네요... 코드에 대한 질문도 많이 물어봐주시면 열심히 찾아보면서 학습하겠습니다! 감사합니다:)

- Gradle 수정 - HomeController, ReservationController 생성 - Reservation dto 생성

- ReservationController속 변수에 값 넣어준 코드 주석처리

- h2 데이터베이스를 활용하여 데이터를 저장하도록 수정

- 예약 조회 API 처리 로직에서 저장된 예약을 조회할 때 데이터베이스를 활용하도록 수정

- 예약 추가/취소 API 처리 로직에서 데이터베이스를 활용하도록 수정

haeyoon1 · 2024-11-17T14:22:34Z

안녕하세요 리뷰어님! 조언주신대로 구조를 변경해보았습니다.
컨트롤러가 비즈니스 로직을 수행하지 않도록 service 패키지를 추가했고, dto도 responsedto와 requestdto로 분리하였습니다. 아래는 그 과정에서 생긴 질문입니다! 잘부탁드립니다:)
그리고 추가로 예외문은 다음 pr작성때 더욱 꼼꼼하게 작성하겠습니다..!

Q1. controller 분리 기준: controller를 분리하는 기준에 대해 찾아보니 역할과 책임에 따라 나누거나, 기능 순서에 따라 나눈다고 하더라고요! 다른분들 코드 보니 @RestController를 사용하는 컨트롤러, @controller를 사용하는 컨트롤러로 나눈 경우도 있던데 이는 둘중에 어디에 해당되는것인가요? 저는 예약을 조회하고 추가하고 삭제하는 것이 모두 비슷한 역할을 한다고 생각해 한 패키지에 넣었거든요. 그래서 이를 분리한다면 어떻게 나누면 좋을지 여쭤보고 싶습니다.

Q2. dto분리 기준:
controller의 입력값으로 가져오는것은 requesetDto, 반환값으로 나오는 것은 responseDto라고 이해했는데 맞나요? 근데 왜 하나의 DTO를 사용하지 않고 그렇게 따로 관리하는 것인지 아직 잘 이해가 되지 않습니다..

be-student · 2024-11-18T09:25:17Z

먼저 답변부터 드리고 시작할게요

@RestController를 사용하는 컨트롤러, @controller를 사용하는 컨트롤러로 나눈 경우도 있던데 이는 둘중에 어디에 해당되는것인가요?

아마 인프런 가장 첫 ai 가 써준 글을 본 것 같은데요
일단 역할쪽에 조금 더 가깝지 않을까 싶네요. Json 을 반환하는 역할, Html 을 반환하는 역할 정도가 되려나요?

같은 패키지에 두어도 괜찮다고 생각합니다!
지금 단계에서는 클래스 분리 정도로도 충분하겠죠

근데 왜 하나의 DTO를 사용하지 않고 그렇게 따로 관리하는 것인지 아직 잘 이해가 되지 않습니다..

이 부분은 입력, 응답이 모든 경우에 동일하지 않기 때문인데요
대표적으로 생성 후에 알 수 있는 id 의 유무가 있습니다
이걸 같은 클래스로 풀려고 하면 오히려 어떤 경우에는 null 어떤 경우에는 not null 로 섞여있다보니 로직에 복잡도가 훨씬 올라갑니다

be-student

약간 고민이 되는 포인트인데요
이 프로젝트에서 대부분의 것들은 이미 잘 완성을 시켜주셨어요

아마 코드에 대한 대부분의 피드백은 전 리뷰 단계에서 받으셨을 것이라서 코드상에서는 크게 피드백을 드릴 것이 없는데요

어떤 방향으로 리뷰를 받고 싶은지 궁금합니다

이대로 그냥 바로 머지
코드 외적인 프로젝트 구조 잡기
스프링에 있는 기능들을 활용해보기
코드의 디테일을 조금 더 챙겨보기

be-student · 2024-11-18T09:26:23Z

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

+        return ResponseEntity.ok(reservations);
+    }
+
+    //예약 추가


저는 개인적으로 이렇게 메소드 주석보다는 클래스 레벨에 어떤 책임을 가지고 있는지 적는 것을 선호합니다
코드 레벨에서 당연히 바로 알 수 있는 내용일 경우에 전체 주석의 신뢰도를 떨어뜨려서 오히려 보기 힘들 수도 있는 악영향을 끼칠 수도 있거든요

be-student · 2024-11-18T09:28:10Z

src/main/java/roomescape/dto/ReservationResponseDto.java

+    private String time;
+
+    public ReservationResponseDto(Long id, String name, String date, String time) {
+        this.id = id;


이 필드의 유무에 따라서 dto 를 분리하는지 아닌지가 나뉠겁니다

적어도 이제 각 목적별로는 dto 를 분리해야지 나중에 특정 경우에는 어떤 데이터가 내려가고, 어떤 경우는 아니고 했을 때 대응하기가 쉬워져요

예를 들면 id랑 name만 필요한경우, id, name, date만 필요한경우, id, name, date, time모두 필요한 경우가 있다면, 세개의 Dtoclass를 만들어서 해당 경우에 필요한 Dto 클래스를 골라 써야 한다는 말로 이해했는데 맞나요?!

넵 맞습니다!
만약 그걸 명확하게 구분해서 사용해야한다면 이상적으로는 구분하는 것이 맞겠죠

ex) 만약 date 를 바꿀 수 있는 경우는 관리자밖에 없다
이런 케이스는 해도 괜찮을 것 같아요

만약 이런 케이스가 아니라 단순히 date 가 있을 수도 있고, 없을 수도 있다 정도의 레벨에서는 같이 두고, nullable 하게 처리하는 것도 괜찮은 방법입니다!

타입 지정은 완벽하게 정답이 있기 보다는 프로젝트의 성숙도에 따라서 더 달라지는 부분이라서 예시 상황마다 다를 것 같아요
대부분의 경우에 id 는 따로 둬야 한다는 것이 약간 국롤처럼 잡혀있는 느낌이에요

be-student · 2024-11-18T09:35:33Z

src/main/java/roomescape/repository/ReservationRepository.java

+
+    //예약 조회
+    public List<Reservation> findAll() {
+        String sql = "SELECT id, name, date, time FROM reservation";


이 sql 부분은 dao 로 분리해보면 어떨까요?

https://ttl-blog.tistory.com/1285

be-student · 2024-11-18T09:37:33Z

src/main/java/roomescape/entity/Reservation.java

+    private String date;
+    private String time;
+


이제는 String 부분을 LocalDateTime 으로 바꿔보면 어떨까요?
시간의 형태를 String 으로 들고 다니면 나중에 스트링의 형태가 잘못되었을 때 어디서부터 잘못되었는지 파악하기가 힘들 것 같아요

be-student · 2024-11-18T09:38:10Z

src/main/resources/schema.sql

+(
+    id      BIGINT       NOT NULL AUTO_INCREMENT,
+    name    VARCHAR(255) NOT NULL,
+    date    VARCHAR(255) NOT NULL,


여기도 VARCHAR 형태가 아닌 TimeStamp 형태로 바꿔보면 어떨까요?

be-student · 2024-11-18T09:43:31Z

src/main/java/roomescape/exception/InvalidValueException.java

@@ -0,0 +1,8 @@
+package roomescape.exception;
+
+public class InvalidValueException extends RuntimeException {


이 프로젝트에서 다루는 공통 예외가 있으면 어떨까요?
ex) RoomscapeException

그래서 모든 예외는 저 RoomscapeException 을 상속하는 구조로 만들어지는거죠
그랬을 때의 장점은 라이브러리에서 터지는 예외와 실수로 catch 하지 않은 우리 서비스 로직에서 터지는 exception 하고 구분이 안될 것 같아요

ControllerAdvice 쪽에서도 RoomscapeException 를 처리하는 ExceptionHandler 와 Exception 을 처리하는 ExceptionHandler 을 서로 분리해서 서로 다른 처리를 할 수 있을 것이고요

이미 InvalidValueException랑 NotFoundReservationException는 RuntimeException를 상속 받고 있는데 그러면 RuntimeException 대신 새로 만든 RoomscapeException를 상속받는 클래스로 수정하라는 말씀이신가요?

그랬을 때의 장점은 라이브러리에서 터지는 예외와 실수로 catch 하지 않은 우리 서비스 로직에서 터지는 exception 하고 구분이 안될 것 같아요

ControllerAdvice 쪽에서도 RoomscapeException 를 처리하는 ExceptionHandler 와 Exception 을 처리하는 ExceptionHandler 을 서로 분리해서 서로 다른 처리를 할 수 있을 것이고요

그리고 이 부분이 잘 이해가 되지 않습니다..!ㅠㅠ ControllerAdvice는 사용하지 않은 것 같은데.... 설명부탁드립니다!

이미 InvalidValueException랑 NotFoundReservationException는 RuntimeException를 상속 받고 있는데 그러면 RuntimeException 대신 새로 만든 RoomscapeException를 상속받는 클래스로 수정하라는 말씀이신가요?

graph TD; InvalidValueException --> RoomscapeException --> RuntimeException NotFoundReservationException --> RoomscapeException

Loading

와 같은 형태가 되면 가장 이상적일 것 같아요!
말씀해주신 부분이 맞습니다

그리고 이 부분이 잘 이해가 되지 않습니다..!ㅠㅠ ControllerAdvice는 사용하지 않은 것 같은데.... 설명부탁드립니다!

제가 말씀드린 ControllerAdvice = RestControllerAdvice 입니다!
https://tecoble.techcourse.co.kr/post/2020-08-17-custom-exception/
여기에 4번 항목을 보시면 조금 더 이해가 될 것 같아요!

be-student · 2024-11-18T09:50:33Z

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

+    public ReservationResponseDto createReservation(ReservationRequestDto requestDto){
+
+        Reservation reservation = new Reservation(
+                null,


보통 이런 경우에 null 을 직접 대입하는 것이 이상하다보니 생성자 오버로딩을 통해서 id 가 없는 경우에 자동으로 null 을 대입해주는 방식이 있는데요
이걸 적용해보시면 어떨까요?

haeyoon1 · 2024-11-21T16:18:57Z

같은 패키지에 두어도 괜찮다고 생각합니다!
지금 단계에서는 클래스 분리 정도로도 충분하겠죠

그렇다면 지금 제 코드가 클래스 분리가 된 상태인건가요? 아니면 추가로 클래스 분리를 했으면 좋겠다는 말씀이신가요?!

그리고 ReservationExceptionHandler에서 ExceptionHandler가 import되지 않아 저렇게 길게 본문 코드에 작성되는데 수정방법이 있나요?

리뷰 잘부탁드립니다!

be-student · 2024-11-23T03:25:43Z

리뷰가 늦어져서 죄송합니다!

질문주신 부분은
모든 것은 프로젝트의 규모에 따라 바뀌어야 합니다.
라는 말씀을 먼저 드리고 싶은데요

제 취향에서는 무조건 html 을 반환하는 클래스랑 json 을 반환하는 클래스랑은 분리되는 것이 더 관리하기 용이하다는 입장인데요 (저는 그냥 이 부분은 프로젝트 규모가 아무리 작아도 일단 분리하고 생각할 것 같아요)
그렇지만 이 부분은 워낙 취향을 많이 타는 부분이다보니....
어딘가의 시니어 개발자분중에 어떤 분은 분명히 이 규모에서는 그냥 분리하지 않고 써 라는 입장을 가지고 계실테니 선택해주시면 됩니다

import 가 안되는 부분은 이렇게 해결하면 돼요!
그냥 앞에 있는 부분을 다 지워두고 나서 다시 import 를 하거나, ExceptionHandler 에서 우클릭 후에 거기서 remove explicit import 였나? 그 옵션을 선택해서 보시면 됩니다!
저도 정확하게 기억이 안 나네요...
우클릭 해서 메뉴에서 찾아보시면 될 것 같아요!

아마 이번 리뷰의 메인은

soft delete 적용
time, date 합치기
dao 네이밍 고민해보기
dirtiestContext 말고 다른 방식으로 테스트간 격리를 어떤 방식으로 할 수 있는지 찾아보고, 적용해보기

be-student

고생하셨습니다!

be-student · 2024-11-23T03:04:11Z

src/main/java/roomescape/dto/ReservationResponseDto.java

+    private String time;
+
+    public ReservationResponseDto(Long id, String name, String date, String time) {
+        this.id = id;


넵 맞습니다!
만약 그걸 명확하게 구분해서 사용해야한다면 이상적으로는 구분하는 것이 맞겠죠

ex) 만약 date 를 바꿀 수 있는 경우는 관리자밖에 없다
이런 케이스는 해도 괜찮을 것 같아요

만약 이런 케이스가 아니라 단순히 date 가 있을 수도 있고, 없을 수도 있다 정도의 레벨에서는 같이 두고, nullable 하게 처리하는 것도 괜찮은 방법입니다!

타입 지정은 완벽하게 정답이 있기 보다는 프로젝트의 성숙도에 따라서 더 달라지는 부분이라서 예시 상황마다 다를 것 같아요
대부분의 경우에 id 는 따로 둬야 한다는 것이 약간 국롤처럼 잡혀있는 느낌이에요

be-student · 2024-11-23T03:06:53Z

src/main/resources/schema.sql

+(
+    id      BIGINT       NOT NULL AUTO_INCREMENT,
+    name    VARCHAR(255) NOT NULL,
+    date    VARCHAR(255) NOT NULL,


이번에는 date 와 time 을 한번 통일시켜보면 어떨까요?
날짜, 시간의 정보가 대부분의 경우에 같이 필요하지, 어느 한 가지만 필요한 경우는 많이 없을 것 같아요!

be-student · 2024-11-23T03:10:08Z

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

+    public void delete(Long id) {
+        String sql = "DELETE FROM reservation WHERE id = ?";
+        jdbcTemplate.update(sql, id);
+    }


보통 delete 를 해야 하는 경우에 실제 delete 를 하는 경우는 거의 없는데요
특히 예약이나 돈 거래와 같은 금전이 엮여있는 경우는 거의 절대로 하지 않는다고 보셔도 됩니다
보통 deleted 나 removed, status, enable 과 같은 뭔가 상태를 표현할 수 있는 column 을 두고, 그 column 에 값을 업데이트 하는 방법으로 많이 하게 되는데요

조회를 할 때는 진짜 전체를 조회하거나, 현재 활성화 되어있는 전체를 조회하는 방향으로 진행하게 됩니다
이를 soft delete 이라고 하는데요
한번 이렇게 바꿔보시는 것도 좋을 것 같아요!

soft delete 를 왜 해야 하는지에 대해서도 정리해서 댓글에 달아주시면 공부하는 과정에 많은 도움이 될 것 같아요

be-student · 2024-11-23T03:12:53Z

src/main/java/roomescape/repository/ReservationRepository.java

+@Repository
+public class ReservationRepository {
+
+    private final ReservationDao reservationDao;


보통 dao 의 네이밍은

Suggested change

private final ReservationDao reservationDao;

private final ReservationJdbcTemplateDao or MysqlReservationDao

와 같이 어떤 클래스를 사용했는지를 명확하게 적어주는 편이 좋은 것 같더라고요

repository 와 dao 가 나온 이유도 이와 조금 더 맞을 것 같은데요
repository -> db 종류에 구애받지 않는다
dao -> db 종류에 구애받는다
라고 봤을 때 dao 의 느낌상 하나의 db or 라이브러리에 무조건 종속되다보니 네이밍에서부터 드러내주는 것이 좋은 것 같아요

be-student · 2024-11-23T03:16:33Z

src/main/java/roomescape/exception/InvalidValueException.java

@@ -0,0 +1,8 @@
+package roomescape.exception;
+
+public class InvalidValueException extends RuntimeException {


이미 InvalidValueException랑 NotFoundReservationException는 RuntimeException를 상속 받고 있는데 그러면 RuntimeException 대신 새로 만든 RoomscapeException를 상속받는 클래스로 수정하라는 말씀이신가요?

graph TD; InvalidValueException --> RoomscapeException --> RuntimeException NotFoundReservationException --> RoomscapeException

Loading

와 같은 형태가 되면 가장 이상적일 것 같아요!
말씀해주신 부분이 맞습니다

그리고 이 부분이 잘 이해가 되지 않습니다..!ㅠㅠ ControllerAdvice는 사용하지 않은 것 같은데.... 설명부탁드립니다!

제가 말씀드린 ControllerAdvice = RestControllerAdvice 입니다!
https://tecoble.techcourse.co.kr/post/2020-08-17-custom-exception/
여기에 4번 항목을 보시면 조금 더 이해가 될 것 같아요!

be-student · 2024-11-23T03:25:16Z

src/test/java/roomescape/MissionStepTest.java

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


https://velog.io/@tjddus0302/Spring-DirtiesContext
이 부분은 한번 삭제해보시는 것이 어떨까요?

견본 코드에서부터 이렇게 만들어져 있을 것이라서 건들이면 안될 것처럼 되어있지만, 이 방법 외에 다른 방법으로 테스트 격리를 진행해보시면 좋을 것 같아요!
아마 무조건 리뷰를 달아달라고 넣어둔 것 같아요

haeyoon1 added 6 commits November 6, 2024 15:51

feat: 홈 화면 생성, 예약 조회

ad9fc14

- Gradle 수정 - HomeController, ReservationController 생성 - Reservation dto 생성

feat: 예약 추가/취소

7dfd8c5

- ReservationController속 변수에 값 넣어준 코드 주석처리

feat: 예외 처리

3452ba4

feat: 5단계 - 데이터베이스 적용하기

7b379b3

- h2 데이터베이스를 활용하여 데이터를 저장하도록 수정

feat: 6단계 - 데이터 조회하기

dec3a7a

- 예약 조회 API 처리 로직에서 저장된 예약을 조회할 때 데이터베이스를 활용하도록 수정

feat: 7단계 - 데이터 추가/삭제하기

bad771c

- 예약 추가/취소 API 처리 로직에서 데이터베이스를 활용하도록 수정

be-student approved these changes Nov 18, 2024

View reviewed changes

fix: 구조 변경

9f4ec62

haeyoon1 force-pushed the haeyoon1-jdbc branch from f682588 to 9f4ec62 Compare November 21, 2024 16:17

be-student approved these changes Nov 23, 2024

View reviewed changes

haeyoon1 changed the base branch from main to haeyoon1 November 25, 2024 05:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spring JDBC] 남해윤 미션 제출합니다. #379

[Spring JDBC] 남해윤 미션 제출합니다. #379

haeyoon1 commented Nov 13, 2024 •

edited

Loading

haeyoon1 commented Nov 17, 2024

be-student commented Nov 18, 2024

be-student left a comment

be-student Nov 18, 2024

be-student Nov 18, 2024

haeyoon1 Nov 21, 2024

be-student Nov 23, 2024

be-student Nov 18, 2024

be-student Nov 18, 2024

be-student Nov 18, 2024

be-student Nov 18, 2024

haeyoon1 Nov 21, 2024

be-student Nov 23, 2024

be-student Nov 18, 2024

haeyoon1 commented Nov 21, 2024

be-student commented Nov 23, 2024

be-student left a comment

be-student Nov 23, 2024

be-student Nov 23, 2024

be-student Nov 23, 2024

be-student Nov 23, 2024

be-student Nov 23, 2024

be-student Nov 23, 2024

		@@ -0,0 +1,8 @@
		package roomescape.exception;

		public class InvalidValueException extends RuntimeException {

	private final ReservationDao reservationDao;
	private final ReservationJdbcTemplateDao or MysqlReservationDao

[Spring JDBC] 남해윤 미션 제출합니다. #379

Are you sure you want to change the base?

[Spring JDBC] 남해윤 미션 제출합니다. #379

Conversation

haeyoon1 commented Nov 13, 2024 • edited Loading

haeyoon1 commented Nov 17, 2024

be-student commented Nov 18, 2024

be-student 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

haeyoon1 commented Nov 21, 2024

be-student commented Nov 23, 2024

be-student 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

haeyoon1 commented Nov 13, 2024 •

edited

Loading