[Spring Core] 정상희 미션 제출합니다. #388
Conversation
There was a problem hiding this comment.
안녕하세요, 상희님.
다시 돌아온 루카입니다. 🐳
매번 뵐 때마다 성장한 것이 느껴집니다.
상희님의 열정이 멋있습니다!!
리뷰어에게도 새로운 가이드가 생겨서, 앞으로는 적은 맥락을 여러번 자주 주고 받을 에정입니다.
🎯 리뷰 포인트
- DTO의 역할
- String 대신 LocalTime 사용하기
- Entity의 비교
- 동일한 시간 생성 막기
🔮 질문에 대한 답변
[1번 질문]
같은 계층인 ReservationTimeService에 의존 VS ReservationTimeRepository에 의존
이에 대해서는 코드 코멘트에 간접적으로 질문을 다시 드렸습니다.
결론부터 말씀드리면, 이는 정답은 없습니다.
📚 레이어간 의존성
여기서 확실히 말씀드릴 수 있는 것은 하나입니다.
ReservationService가 Service를 의존하건 Repository를 의존하건
레이어간 의존성은 아래에서 위로 흐르게 해야겠어요.
- 도메인 영역은 제일 안쪽에서 온전히 비즈니스 규칙을 담고 있죠. 그러므로 도메인에선 다른 레이어를 참조하긴 힘들죠.
- 영속화(레파지토리) 레이어는 도메인 모델을 영속화하고 영속화된 도메인을 조회할 수 있도록 도와주는 역할이므로 도메인 이외에는 참조하기 어렵습니다.
- 어플리케이션(서비스) 레이어는 비즈니스 로직을 담고 있으므로, 도메인과 레파지토리를 의존하겠지만, 표현영역에는 의존하면 안되겠죠.
- 표현(웹) 계층 이 곳은 우리 서버에서 어쩌면 가장 외각에 존재하는 레이어라고 볼 수 있겠어요. 외부와 소통하기 위한 인터페이스 역할을 하는 것이지요. 의존성에 흐름상 모든 레이어의 변화에 영향을 받을 수 밖에 없는 영역입니다.
의존성이 있다는 것은 변경에 영향을 받는다는 것입니다.
각 레이어 간 변화가 전파되지 않으면 좋겠지만, 꼭 전파되어야한다면 방향성이 있어야겠죠.
🖇️ 도메인간 의존성
(이 도메인간 의존성에 대한 내용은 추가적인 리뷰이니 깊게 보실 필욘 없습니다)
도메인간 의존성은 어떻게되어야할까요? 한방향?
예를 들어, 현재는 Reservation이 ReservationTime을 의존하고 있죠.
반대로 ReservationTime이 해당 시간에 예약된 List을 갖고 있을 수도 있겠죠.
그럼 한방향이 아니라 의존성이 순환하게 됩니다.
이런 상황은 좋은 상황일까요? 안좋은 상황일까요?
(다시 한 번, 중요하지 않음)
[2번 질문]
이건 좀 사소한? 고민이긴 한데 변수명이 전체적으로 너무 긴 것 같은데 혹시 바꾸는 게 나을까요?
저는 그런 부분을 딱히 느끼지 못했는데, 어딘지 구체적으로 말씀해주실 수 있나요?
👀 추가 전달
제출된 PR에 2가지 문제가 발생 중입니다.
- 테스트 통과하지 않음
시간관리nav 메뉴 접근 불가
어떤 결과물을 인수할 때는
본인이 작성한 테스트가 잘 통과하는지
QA하여 유저 시나리오가 잘 이행되는지
꼭 확인해주세요.
이번엔 짧게 리뷰 남긴만큼 짧은 주기로 의견 주고 받아봐요.
미완성인 부분과 개선할 부분이 있어서 RC 드리겠습니다.
파이팅!!
| package roomescape.dto; | ||
|
|
||
| import roomescape.entity.ReservationTime; | ||
|
|
||
| public record ReservationTimeResponse( | ||
| Long id, | ||
| String time | ||
| ) { | ||
| public static ReservationTimeResponse fromReservationTime(ReservationTime reservationTime) { | ||
| return new ReservationTimeResponse( | ||
| reservationTime.getId(), | ||
| reservationTime.getTime() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Comment]
ReservationTime에 대한 응답 DTO로 ReservationTimeResponse를 선언하셨네요.
멋진 방법입니다.
ReservationTime이라는 엔티티를 Domain과 application layer 안에서만 다룰 수 있게 해주네요.
그치만, ReservationTime과 이 DTO는 필드와 타입이 정확하게 일치하는데요.
굳이 DTO를 따로 만들 필요가 있을까요?
의존성의 흐름을 고려해보면,
그림을 예로 들어 아래에서 위로 흐릅니다.
Domain과 applicationLayer 둘 다 표현 영역이 의존하고 있는 아이들이라 엔티티 자체를 넘겨줘도 상관없지않을까요?
장단점이 있을 것 같은데요.
장단점에 대해서 한번 고민해보면 좋을 것 같습니다.
There was a problem hiding this comment.
오! 지난 스터디 때 동아리원들과 함께 고민해본 내용인데요,
우선 DTO를 사용할 때의 가장 큰 장점은 API 스펙과 도메인의 의존성이 줄어들게 된다는 점 같습니다. 로직이 변경되었을 때 모든 곳을 수정해야하는 것이 아닌 최소한의 수정만으로 끝낼 수 있다는 것은 코드의 안정성에도 기여를 하는 부분이라 중요한 것 같습니다.
ex) 아래에서 리뷰어님이 Time을 String으로 받는 것을 LocalTime으로 수정하라고 제안해주셨는데 바꾸는 과정에서 엔티티는 수정되지만 DTO는 수정할 필요가 없었습니다.
단점은 리뷰어님이 말씀해주셨다시피 필드가 같은데 굳이 엔티티 -> DTO로 변환하는 과정이 필요하다는 것과 개별적으로 엔티티와 DTO를 관리해야 하기에 코드에 중복이 발생할 수 있다는 것 같습니다.
| @Getter | ||
| public class ReservationTime { | ||
| private Long id; | ||
| private String time; |
There was a problem hiding this comment.
[Request Chnage]
Oh... no....
시간을 String으로 받고 있군요...
이런 엔티티를 설계를 하고 불안감을 느끼지 않으셨다면,
조금 더 위험에 대한 예민도를 올릴 필요가 있겠네요.
String은 정말 자유로운 타입이죠.
"서울시 여러분~" 이라고 생성자에 넣어도 잘 객체 생성이 되는데요...
String을 필드로 사용했다면, 최소한 엔티티 생성자에서 값에 대한 validate는 실행해야되지 않을까요?
새로운 요구사항
java에는 시간을 나타내는 LocalTime, LocalDate, ... 같은 여러 타입이 존재합니다.
- 요구사항: time 필드를 LocalTime으로 변경해주세요.
- 고민
- 레이어 별로 지원하거나 필요로하는 형식이 다를텐데요. 어떻게 할 수 있을까요?
There was a problem hiding this comment.
변경했습니다!
표현 -> 레포지토리 계층의 방향은 String -> LocalTime
반대로는 LocalTime -> String으로 형식을 지정했습니다.
클라이언트에게 보이는 방향은 String이 비교적으로 다양한 형식을 나타낼 수 있어서 이렇게 수정했습니다.
| @Getter | ||
| public class ReservationTime { | ||
| private Long id; | ||
| private String time; | ||
|
|
||
| public ReservationTime(Long id, String time) { | ||
| this.id = id; | ||
| this.time = time; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Comment]
Entity인 ReservationTime은 ID를 갖고 있군요.
ReservationTime에 대한 비교는 어떻게 할 수 있을까요?
예를 들어서, 중복된 예약을 막기 위해 Reservation을 서로 비교해야한다면, 어떻게 비교해야하죠?
어떤게 맞을까요. 예시를 드려볼게요.
- id가 같을 때
- time이 같을 때
- id와 time이 둘 다 같을 때
There was a problem hiding this comment.
time을 생성할 때 중복 생성을 막았다면 1번을 통해 비교할 수 있을 것 같습니다.
| public ReservationTimeResponse createReservationTime(ReservationTimeRequest reservationTimeRequest) { | ||
| ReservationTime reservationTime = reservationTimeRequest.toEntity(); | ||
|
|
||
| reservationTime = reservationTimeRepository.save(reservationTime); | ||
|
|
||
| return ReservationTimeResponse.fromReservationTime(reservationTime); | ||
| } |
There was a problem hiding this comment.
[Request Change]
헉 이 방탈출 카페는 테마가 되게 여러갠가봐요.
예약가능한 시간을 동일한 시간으로 여러개 만들 수 있네요...
12시를 무한대로 생성할 수 있는데, 이는 막아야하지 않을까요?
만약, 방탈출 러닝타임은 고려하지 않고 (하시면 좋고)
예약가능 시간이 중복될 수 없다면 어떤 계층에서 어떤 로직으로 막아야할까요?
이 힌트와 함께 생각해보면 좋겠습니다.
There was a problem hiding this comment.
비즈니스 규칙 책임에 해당한다고 생각을 해서 도메인이 가지도록 위임하려고 했으나
도메인에서 처리하는 마땅한 방법을 찾지 못해 서비스가 가지도록 했습니다.
| public List<ReservationTimeResponse> findAllReservationTimes() { | ||
| List<ReservationTime> reservationTimes = reservationTimeRepository.findAll(); | ||
|
|
||
| return reservationTimes.stream() | ||
| .map(ReservationTimeResponse::fromReservationTime) | ||
| .toList(); | ||
| } | ||
|
|
||
| public ReservationTime findReservationTimeById(Long id){ | ||
| return reservationTimeRepository.findById(id); | ||
| } |
There was a problem hiding this comment.
[Comment]
- 전체 조회 로직은 Response Dto를 반환
- 단일 조회 로직은 Entity를 반환
왜 차이가 생긴 것일까요?
위 DTO 관련 리뷰와
상희님이 질문으로 남겨주신 내용을 함께 고민해보면 좋겠어요.
고민 포인트
- DTO에 쓰임은?
- Service에 있어야하는 로직은?
- AService이 참조할 대상은? BService vs BRepository
There was a problem hiding this comment.
참조할 대상이 BService가 아닌 BRepository인 것 같아서 수정했습니다.
현재 상황에서는 DAO에 접근하는 것만 필요한데 다른 서비스를 참조하는 것은 불필요한 것 같다는 생각이 들었습니다!
|
너무 늦게 리뷰 반영이 이뤄져서 죄송해요ㅠ 리뷰 반영하면서 오류를 발견했는데 검색해도
|
안녕하세요 루카님 :) 이번에도 잘 부탁드립니다
고민 1)
이번 미션에서 특히 고민이 됐던 부분이 있었는데요,
ReservationService에서 ReservationTime을 조회해야 하는 상황이 발생했는데
같은 계층인 ReservationTimeService에 의존 VS ReservationTimeRepository에 의존
처음엔 후자로 작성했다가 마지막엔 전자로 바꿨는데 리뷰어님 생각은 어떠신지 궁금합니다. 처음 생각했을 때는 그냥 단순히 DAO에서 조회만 해오는 거라 굳이 전자를 택하지 않아도 된다고 생각했는데 인터넷을 찾아보니 또 데이터를 다루는 책임이 분산되어 관리하기가 어려워진다는 단점도 있더라구요!
근데 결국 지금 생각해보니 현재 프로젝트는 규모가 크지 않아서 후자가 더 나을 것 같기도 하네요..ㅎ
=> 스터디를 하며 다른 사람들과 함께 고민을 해봤는데 지금은 단지 조회하는 로직 하나만 존재하기도 하고 레이어드 아키텍처의 측면에서 의존성의 방향이 하위 계층인 Repository를 의존하는 것이 더 맞는 것 같아서 다시 후자로 바꿔보려 합니다..!
고민 2)
이건 좀 사소한? 고민이긴 한데 변수명이 전체적으로 너무 긴 것 같은데 혹시 바꾸는 게 나을까요?