-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Spring Core] 신혜빈 미션 제출합니다. #385
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 예전에 봤을 때를 떠올리기 힘들 정도로 자바나 스프링에 익숙해지셨네요!
특히 레이어 계층에 대한 따끔한 피드백 부탁드립니다!!
이 부분을 열심히 찾아봤지만... 레이어는 전체적으로 다 잘 잡아주신 것 같아요!
만약 이 정도 규모의 프로젝트를 해야하는데, 추가적인 정보나 확장 방향성이 주어지지 않는다면 저도 비슷하게 만들 것 같아요
이제 그나마 열심히 트집을 잡아본다면 2번째 질문쪽에 그나마 트집이 잡힐 수 있을 것 같은데요
디비 접근 책임은 DAO(Data Access Object)가 가지도록 위임해보세요.
조건 사항을 무조건 따르려고 했다면 이 코드의 Repository 는 잘못된 것 같아요
반대로 너무 과한 부분을 제거하고 현실적으로 코드를 짜려고 했다면 이 코드는 잘짜진 것 같아요
그렇게 말씀드리는 이유는 저는 소개드린 글과 비슷한 생각을 가지고 있는데요
https://ttl-blog.tistory.com/1285 에서 그나마 잘 정리를 해주신 것 같은데요 (절대 우테코 친구의 글이라 소개한 것은 아닙니다)
repository 의 경우에는 같은 생명주기를 가진 객체 덩이(Aggregate Root)를 한꺼번에 조회한다는 ddd 에서 처음 등장한 용어인데요
그래서 약간 근본을 따지자면 요 미션에서는 같은 생명주기를 가진 Aggregate Root 로 묶일만한 정도로 단위가 크지 않다 -> 따라서 Repository 가 등장하기 적절하지 않다 라고 할 수도 있고요
ddd를 완벽하게 배제한 상황에서 repository 는 보통 특정 db 구현체에 대한 정보가 없어야 된다고 봐도 될 것 같아요
지금의 경우에 Repository 를 그대로 RoomscapeMysqlDao 같은 형태로 이름을 바꾸고, Service 에서는 Repository 에 의존하고, Repository 는 RoomscapeMysqlDao
로 바로 모든 요청을 바이패스 할 것 같아요
과연 이것이 좋은지는 모르겠지만요?
여유가 되신다면
https://www.youtube.com/watch?v=dJ5C4qRqAgA 를 보시면 조금 도움이 될 것 같아요!
물론 엄청 시간이 많이 들어갈 것이라서 그냥 네이밍을 바꾸고, 바이패스하는 레이어를 추가해주셔도 됩니다!
data/database.mv.db
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 이 파일은 왜 들어간걸까요?
관계 없는 파일의 경우에는 gitignore 에 추가해주시면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찾아보니 초록 스터디를 할 때 초기에 제공된 데이터베이스로 저도 들어간 지 미처 몰랐네요 ㅜㅠ
예시용으로 들어있던 데이터니 없어도 될 거 같아서
gitignore에 추가해 Git이 해당 파일을 추적하지 않고 무시하도록 설정해두었습니다!!
|
||
import static org.assertj.core.api.AssertionsForClassTypes.assertThat; | ||
import static org.hamcrest.Matchers.is; | ||
|
||
|
||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) | ||
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
많은 분들에게 공통으로 남겨드린 리뷰인데요
테스트 격리를 dirtiestContext 를 통해서 하게 되면 스프링 컨텍스트가 너무 많이 떠서 테스트코드를 실행시키는 것이 힘들 정도로 오래 걸리는데요
다른 방식으로 격리를 해볼 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드가 초록 스터디에서 기본 제공한 코드라 자세히 들여다볼 생각을 못 했네요 ㅜㅠ
@DirtiesContext
란?
특정 테스트가 끝난 후 스프링 컨텍스트를 더럽혀졌다고 표시하여 기존 컨텍스트를 재사용하지 않고 새로운 컨텍스트를 생성하는 기능이다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이를 대체할 격리 방법?
데이터베이스 초기화 전략
사용
-> 테스트 간 데이터 상태를 초기화하여 Spring 컨텍스트를 재시작하지 않고도 테스트 격리를 유지할 수 있음
따라서 @beforeEach
를 사용해 각 테스트 메서드 실행 전에 데이터 베이스를 초기하는 방식으로 격리를 해주었습니다!!
ex)
@BeforeEach
void setUp() {
jdbcTemplate.execute("TRUNCATE TABLE reservation RESTART IDENTITY");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덕분에 또 새로운 지식을 쌓아 갑니다!! 감사합니다 :)
@@ -16,4 +32,187 @@ public class MissionStepTest { | |||
.then().log().all() | |||
.statusCode(200); | |||
} | |||
|
|||
@Test | |||
void 이단계() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 식으로 단계가 되어있는 것은 보통 순차적으로 실행되는 것을 기대하는 것일텐데요
TestMethodOrder 를 통해서 순서대로 호출되는 것을 보장할 수 있습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test의 순서도 조정할 수 있는 지는 몰랐네요!!
@TestMethodOrder
어노테이션과 함께 MethodOrderer
를 사용해 테스트가 지정된 순서대로 실행되도록 코드를 변경해주었습니다 :)
ex)
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
public class MissionStepTest {
@Test
@Order(1)
void 일단계() {
RestAssured.given().log().all()
.when().get("/")
.then().log().all()
.statusCode(200);
}
. . .
}
id BIGINT NOT NULL AUTO_INCREMENT, | ||
name VARCHAR(255) NOT NULL, | ||
date VARCHAR(255) NOT NULL, | ||
time_id BIGINT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 time 테이블을 따로 분리하려고 시도 자체는 좋은 것 같아요
그렇다면 분리를 하려고 하셨던 이유는 무엇이고, 이렇게 했을 때 어떤 장점이 있을까요?
단점이 있다면 어떤 단점이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드는 제가 작성한 코드가 아니라 문제에 초기 설정된 코드입니다!!
이 코드가 들어갔을 때의 장단점은 생각해보지 않았는 데 덕분에 이런 고민도 할 수 있게 되었네요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<time 테이블을 분리했을 때>
장점:
- 저장공간 절약
중복 데이터 제거해 저장비용을 줄일 수 있음 - 데이터 무결성 유지
데이터를 별도 테이블로 분리하여, 모든 테이블이 동일한 데이터를 참조하도록 하면, 데이터 변경 시 하나의 테이블만 수정하면 됨 - 변경 편리
중복 데이터가 없기 때문에 유지보수가 쉬움
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단점:
- JOIN의 성능 비용 발생
데이터를 조회할 때 time 테이블과의 JOIN 작업이 추가되므로, 성능이 문제가 발생할 수 있음 - 설계와 구현의 복잡성 증가
테이블이 분리되면 설계와 코드 구현이 복잡해짐. 특히 간단한 데이터베이스인 경우 불필요한 복잡성 발생함. - 추가 관리 필요
time 테이블의 관리(삽입, 업데이트, 삭제)가 별도로 필요함
spring.h2.console.enabled=true | ||
spring.h2.console.path=/h2-console | ||
spring.datasource.url=jdbc:h2:mem:database | ||
spring.jpa.hibernate.ddl-auto=create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현업에서는 보통 create 대신 none or validate 를 두고서 애플리케이션이 시작할 때, CommandLineRunner 와 같은 스프링 컨텍스트를 처음 띄울 때 사용되는 부분을 활용하거나
schema.sql 을 활용해서 초기 세팅을 해주는 것 같아요
jpa 가 설정해주는 초기 세팅이 완벽하지 않을 수 있다는 것 + 혹시 데이터가 날아갈까봐 안전하게 해주는 것이 중요하다 쪽인 것 같아요
여유가 된다면 이런 방식도 있다 정도로 배워보셔도 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 create
랑 update
만 알고 있었는 데 none
와 validate
도 있는 건 몰랐네요.
사실 실무에서는 여기에서보다 다루는 데이터가 훨씬 많을 테니 자동 생성(update)을 사용할 줄 알았습니다.
덕분에 새로운 사실을 알게되었네요 :) 감사합니다!! ㅎㅎ
자동으로 스키마를 생성하거나 변경하는 기능은 개발 초기 단계에서는 편리하지만, 운영 환경에서는 데이터의 안전성과 예측 가능성이 더 중요하기 때문에 실무에서는 명시적으로 변경 사항을 관리하기 위해 none이나 validate를 사용한다고 하네요!!
if (reservationRequest.getTime() == null) { | ||
throw new IllegalArgumentException("time은 숫자여야 합니다."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 Valid 어노테이션을 통해서 @NotNull
이었나? 어노테이션을 통해서 검증 코드를 없앨 수 있을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NotBlank(message = "이름은 필수 값입니다.")
final private String name;
@NotBlank(message = "날짜는 필수 값입니다.")
final private String date;
@NotNull(message = "timeId는 필수 값입니다.")
final private Long time;
이미 AddReservationRequest Dto에 제가 이렇게 적용을 해두었네요!!
저 코드는 없어도 될 거 같아 빼주었습니다!!
예리한 지적 감사합니다 :)
private Long id; | ||
private String name; | ||
private String date; | ||
private Time time; // time_id만 저장 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 db 쪽에도 비슷한 내용을 달았던 것 같은데요
혹시 Time_id 만 저장하게 된 이유가 있을까요?
Time 객체를 그대로 들고 다녀도 될 것 같아서요!
추가로 보통 로직에서는 LocalDateTime or ZonedDateTime 을 많이 가지고 다니는 것 같아요! Time 의 경우에는 Sql 라이브러리에 종속되어있다보니 약간 피하시는 것도 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에 time_id로 로직을 구성했다가 Time객체를 가지는 변경했는 데 주석을 제거하는 걸 깜빡한 거 같아요ㅜㅠ
현재 코드를 time_id가 아니라 Time객체를 그대로 들고 다니는 로직으로 변경했습니다!!
막연하게 "time(ex)11:00)이라는 값보다 id(ex)1)가 더 짧으니까 보관에 더 용이하지 않을까?"하고 단순하게 생각했던 거 같아요.
Time의 객체자체를 가지고 있는 게 sql 라이브러리에 종속을 피할 수 있다는 관점은 생각하지 못 했네요!!
앞으로 타입을 지정할 때 종속성도 잘 고려하도록 하겠습니다!! 새로운 관점을 알려주셔서 감사합니다 :)
URI location = URI.create("/reservations/" + reservation.getId()); | ||
return ResponseEntity.created(location).body(reservation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 전 과정에서 충분히 왜 이것들이 필요한지 명확하게 다 학습하셨겠죠?
저는 혜빈님을 믿고 있어요 🙇
약간 2가지 전혀 다른 포인트의 리뷰가 될 것 같아요
- Location 헤더를 줄 필요가 있을까?
보통 실무에서는 Location 헤더는 사실 못본지가 정말 오래 된 것 같아요 😢
이 헤더를 줬으니 뭘 할 것이다라는 것을 전혀 기대하지 않다보니, 오히려 복잡도를 늘린다고 여겨질 수도 있는 포인트인것이죠
그래서 만약 진짜 이것이 필요할지 고민해보셔도 좋을 것 같아요 - Location 헤더를 통해 얻게된 주소 /reservations/1 같은 형태는 동작할까?
보통 Location 헤더는 http 스펙상 저 주소로 get 을 날렸을 때 자원을 반환할 것을 기대하고 있는데요
현재 api path 상으로는 그 get method 가 없는 것 같아요!
만약 필요하다면 추가하는 방향으로 변경해도 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 리뷰에서 Location
헤더를 넣고 어디로 주소를 보낼 지 명확해졌다고 칭찬을 받아서 어디에나 Location
을 넣은 게 좋다고 생각했던 거 같아요 ㅜㅠ
자원을 반환할 필요가 없을때(get method)는 굳이 Location
헤더를 쓰지 않아도 된다는 사실을 인지하고 있겠습니다!!
코드는 return ResponseEntity.status(HttpStatus.CREATED).body(reservation);
이렇게 반환하도록 수정해주었습니다!
private Long id; | ||
private String time; // 필드 이름 수정 | ||
|
||
public TimeSchedule(Long id, String time) { // 생성자 수정 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오... 이 주석은 어떤 의미를 가지고 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드를 약간 수정하면서 이 부분은 지금 없어졌는 데 주석을 빼려 했는 데 모르고 남겨둔 거 같습니다 ㅜㅠ 별 의미 없어요!!
import jakarta.validation.constraints.NotNull; | ||
|
||
public class AddReservationRequest { | ||
@NotBlank(message = "이름은 필수 값입니다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
적용하려고 노력하신 모습이 보여서 정말 좋은 것 같아요! 👍
예전에 코드를 찾아볼 수 없을 정도로 많이 발전하셨네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다!! :) 리뷰어님 덕분입니당!!
안녕하세요 은우 리뷰어님 :)
다시 만나게 되어 반갑습니다!
<1>
이번에 미션을 진행하며 특히나 어려웠던 요구조건은 다음과 같습니다 ㅜㅠ
최대한 정보를 찾아보면서 배치하긴 했는 데 레이어드 아키텍처를 잘 적용되었는 지는 잘 모르겠네요.
데이터를 레이어로 나누어서 처리하려니까 관계성을 이해하는 게 어려운 거 같아요.
특히 레이어 계층에 대한 따끔한 피드백 부탁드립니다!!
<2>
스터디 시간에 제 코드의 repository에는 DAO가 들어있다는 피드백을 들었는 데
respository와 DAO의 차이점이 뭔지(추상적으로는 이해했으나 잘 와닿지는 않습니다), 왜 둘을 분리해야하는 지 잘 모르겠습니다.
쉽게 설명해주실 수 있을까요 ㅜㅠ? 아니면 제가 참고할 만한 쉬운 자료가 있을까요?