-
Notifications
You must be signed in to change notification settings - Fork 280
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
3단계 - 경로 조회 #234
base: zeroooooowest
Are you sure you want to change the base?
3단계 - 경로 조회 #234
Conversation
2단계 리뷰 반영 간단한 리팩토링 포함
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.
안녕하세요! 경로조회 구현 잘해주셨습니다!!
테스트 픽스처도 꼼꼼히 잘 만들어주셨어요!
다만 도메인에 최대한 로직을 넣으려고 하시다보니까 Station객체에서 service로 흐르는 의존이 생겨버렸습니다 ㅠㅠ 해당 부분을 끊어주시면 더 좋을 것 같습니다.
경로와 관련된 부분은 Path 객체를 만들어서 응집도를 높여보시는 것도 좋으실 것 같아요!
테스트와 관련된 부분은 아니지만 조금 더 고민해보시면 좋을 것 같아서 코멘트 남겨봤습니다 :)
확인해보시고 질문이나 궁금한 점 있으시면 언제든 질문주세요! 🔥
public List<Station> findShortestPathTo( | ||
GraphService graphService, | ||
Station targetStation, | ||
List<Line> allLines | ||
) { | ||
return graphService.getShortestPath(this, targetStation, allLines); |
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.
findShortesPathTo가 내부 엔티티인 Station에 위치해야하는 로직이 맞을까요?
도메인객체에서 서비스클래스인 GraphService로 흐르는 의존성이 생겨버렸습니다 😭
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.
의존은 안정적인 쪽으로 흐르는 것을 권장하기 때문에 Presentation -> Application -> Domain 쪽으로 흐르기를 권장드려요! 변경에 더 유연한 구조가 됩니다 :)
https://tecoble.techcourse.co.kr/post/2021-11-21-dip/
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.
3주차 제공된 코드를 참고해보니
Line의 List를 가지는 SubwayMap 클래스와 Sections를 한번 래핑한 Path 클래스를 통해 구현이 되어 있었습니다.
사실 처음에 생각했던 방식이 광역전철 이런개념으로 Line의 리스트를 가지는 엔티티를 만들까였는데 이와 유사한점도 있는거 같습니다. 다만 엔티티 여부의 차이가 있는거 같습니다.
제 코드에서 GraphService는 도메인 레이어의 서비스라고 생각해봤습니다.
참고했던 링크는 다음과 같은데요.
https://leejaedoo.github.io/domain_service/
이런쪽으로 설계해보는 건 어떤지 궁금합니다!
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.
아 어그리거트 단위를 끊기 위한 도메인 서비스로써 로직이고, 말씀하신것처럼 응용프로그램을 위한 로직이 아니라 도메인 로직이 위치하도록 잘 유지하신다면 괜찮을 것 같습니다 :)
@zeroooooowest
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.
설계에 완벽한 정답은 없겠지만
3주차 제공된 코드를 보면 PathService의 메서드에서 SubwayMap이라는 도메인 객체를 만들고 있던데,
1단계에서 준우님이 피드백주신 부분 #199 (comment) 이 생각나서 어플리케이션 레이어에서 직접 new로 도메인을 만들지 않는게? 좋을거 같다는 생각을 했습니다.
그래서 광역전철이라는 엔티티를 만들어서 애플리케이션 서비스에서 광역전철 리포지토리를 통해 조회해볼까라는 생각도 했었었구요.
사실 지금 코드도 완벽히 잘 설계된게 맞는지는 모르겠습니다!ㅎㅎ
var 강남역_ID = 역_생성_요청(강남역).jsonPath().getLong("id"); | ||
var 신논현역_ID = 역_생성_요청(신논현역).jsonPath().getLong("id"); | ||
var 고속터미널역_ID = 역_생성_요청(고속터미널역).jsonPath().getLong("id"); | ||
var 교대역_ID = 역_생성_요청(교대역).jsonPath().getLong("id"); | ||
var 사평역_ID = 역_생성_요청(사평역).jsonPath().getLong("id"); | ||
|
||
노선_생성_요청(LineFixture.of("이호선", 교대역_ID, 강남역_ID, 17)); | ||
노선_생성_요청(LineFixture.of("신분당선", 신논현역_ID, 강남역_ID, 14)); | ||
var 구호선_생성_응답 = 노선_생성_요청(LineFixture.of("구호선", 사평역_ID, 신논현역_ID, 19)); | ||
노선_생성_요청(LineFixture.of("삼호선", 고속터미널역_ID, 교대역_ID, 16)); | ||
|
||
구간_등록_요청(구호선_생성_응답.header("Location"), SectionFixture.of(고속터미널역_ID, 사평역_ID, 11)); |
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.
테스트 픽스처 잘만들어주셨습니다!! 👍 👍
@DisplayName("등록되어 있지 않은 역이 포함된 최단경로를 조회한다") | ||
@Test | ||
void getShortestPathNotExistStation() { | ||
// given | ||
var 강남역_ID = 역_생성_요청(강남역).jsonPath().getLong("id"); | ||
var 신논현역_ID = 역_생성_요청(신논현역).jsonPath().getLong("id"); | ||
var 고속터미널역_ID = 역_생성_요청(고속터미널역).jsonPath().getLong("id"); | ||
var 교대역_ID = 역_생성_요청(교대역).jsonPath().getLong("id"); | ||
var 사평역_ID = 역_생성_요청(사평역).jsonPath().getLong("id"); | ||
|
||
노선_생성_요청(LineFixture.of("이호선", 교대역_ID, 강남역_ID, 17)); | ||
노선_생성_요청(LineFixture.of("신분당선", 신논현역_ID, 강남역_ID, 14)); | ||
var 구호선_생성_응답 = 노선_생성_요청(LineFixture.of("구호선", 사평역_ID, 신논현역_ID, 19)); | ||
노선_생성_요청(LineFixture.of("삼호선", 고속터미널역_ID, 교대역_ID, 16)); | ||
|
||
구간_등록_요청(구호선_생성_응답.header("Location"), SectionFixture.of(고속터미널역_ID, 사평역_ID, 11)); | ||
|
||
// when | ||
var 최단_경로_요청_응답 = 최단_경로_찾기_요청(1000L, 10001L); | ||
|
||
// then | ||
최단_거리_요청_예외(최단_경로_요청_응답); | ||
} | ||
|
||
@DisplayName("존재하지 않는 경로에 대해 최단경로를 조회한다") | ||
@Test | ||
void getShortestNotExistPath() { |
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.
꼼꼼히 예외케이스 테스트 👍 👍 👍
안녕하세요~ 준우님😀
이번 단계는 설계에서 고민이 좀 있었는데요.
Line의 리스트를 갖는 엔티티를 하나 새로 만들까 하다가 GraphService라는 도메인 서비스를 만드는 쪽으로 구현을 해보았습니다.
테스트 코드 작성에서는 크게 어려움을 느꼈던 부분은 없었습니다!
리뷰 잘 부탁드립니다🙏