[Spring 경로 조회 - 1단계] 기론(김규철) 미션 제출합니다. #181

Gyuchool · 2022-05-17T13:22:20Z

안녕하세요 카일!
처음 뵙겠습니다! 기론입니다~😆😆

이번 미션은 외부 라이브러리를 사용해서 경로를 조회하다 보니 빠르게 끝난 것 같네요😬
리뷰 잘 부탁드립니다!

KIMSIYOUNG

안녕하세요 기론!
1단계 미션 잘 구현해주셨네요🙂
다음 미션을 위해 1단계는 여기서 머지하면 좋을 것 같아요.
몇가지 코멘트 남겼는데 다음단계에서 반영해주세요.
궁금하신 내용은 언제나 편하게 연락주세요!

KIMSIYOUNG · 2022-05-19T09:36:43Z

README.md

+* 환승 노선을 고려한다.
+
+
+## 도메인


구체적인 리드미를 잘 작성해주셨네요🙂

KIMSIYOUNG · 2022-05-19T09:37:46Z

src/main/java/wooteco/subway/application/LineService.java

+    }
+
+    @Transactional(readOnly = true)
+    public PathResponse findPath(final Long sourceId, final Long targetId, final int age) {


해당 기능을 LineService에서 제공해야하는 역할이라고 간주하신 이유가 무엇인가요?

사용하지 않는 파라미터는 제거해주세요🙂

페어와 함께 할 당시에는 경로를 찾는 로직도 라인들속에서 경로를 찾는다고 생각해서 LineService에 넣었습니다. 그런데 Line과 Path가 도메인상으로 어떠한 상관관계도 없네요🤔 따로 PathService를 만드는게 좋다고 생각해서 만들었습니다. 또한 2단계부턴 단순히 노선에 대한 역할보단 경로에 대한 역할 (ex)경로에 따른 요금 계산 )이 생겼으므로 따로 PathService로 분리했습니다!

네네🙂 관점에 따라서 특정 도메인에 종속된다라고 생각할 수도 있지만 별개의 도메인으로 바라보는 것도 항상 고려하면 좋을 것 같아요.

그리고 언어적인 관점에서 라인들속에서라고 접근하는 것도 좋지만 기능적인 측면으로 접근했을 때도 독립적인 도메인으로 생각해볼 수도 있을 것 같아요🙂 언어로 접근하는 것은 너무나 당연하지만 라인이라는 포괄적 의미를 넓게 해석하면 역들도 포함하고 있고 라인들간의 역의 구간 처럼 해석할 수도 있으니까요!

KIMSIYOUNG · 2022-05-19T09:38:39Z

src/main/java/wooteco/subway/presentation/PathController.java

+    }
+
+    @GetMapping
+    public ResponseEntity<PathResponse> findPath(@RequestParam Long source, @RequestParam Long target, @RequestParam int age) {


기본적인 파라미터에 대한 검증이 추가되면 좋을 것 같아요. null이 넘어오는 경우에 대한 테스트도 추가해주시면 어떨까요?

기존의 3개의 RequestParam을 dto로 묶고 ModelAttribute를 사용하여 쿼리 스트링을 표현했습니다.
또한 dto내에서 검증을 추가하였고 ControllerAdvice에서 ModelAttribute와 binding이 실패할 때 발생하는 BindException 예외를 캐치하도록 하였습니다.

KIMSIYOUNG · 2022-05-19T09:45:14Z

src/main/java/wooteco/subway/application/LineService.java

+
+    @Transactional(readOnly = true)
+    public PathResponse findPath(final Long sourceId, final Long targetId, final int age) {
+        Path pathFinder = new Path(sectionService.findAll());


변수명과 클래스명을 다르게 설계한 이유가 있을까요?

변수 명을 더 명확히 표현하고 싶었는데 오히려 변수 명이 어색하게 느껴지네요...😬 변수명도 path로 수정했습니다.

KIMSIYOUNG · 2022-05-19T09:46:09Z

src/main/java/wooteco/subway/application/LineService.java

+    public PathResponse findPath(final Long sourceId, final Long targetId, final int age) {
+        Path pathFinder = new Path(sectionService.findAll());
+
+        Station sourceStation = stationDao.findById(sourceId)


LineService에서 섹션서비스에 접근할 때도 있고 스테이션다오에 접근할 때도 있네요. 서비스에선 다른 서비스와 다오 모두를 불러도 된다고 생각하신 이유가 있으실까요?

저는 service에서 dao를 불러오는게 좋다고 생각했습니다. 왜냐하면 의존성 방향이 service -> dao로 흘러야 유지보수에도 좋고 순환참조의 위험에서도 피할 수 있기 때문입니다!
하지만 이번 미션은 페어 코드로 진행하면서 이미 페어는 LineService에 SectionService와 dao를 공통으로 적용해있더라구요. 그래서 findPath()에서 sectionDao와 stationDao를 사용하기에는 오히려 기존에 LineService에 sectionDao가 없는데 findPath()를 위해서 sectionDao에 의존성이 추가되더라구요.
그래서 이미 lineDao, stationDao, sectionService가 의존되어있는데 더 의존되면 결합도가 너무 높아질거라고 생각해서 기존의 sectionService를 활용했습니다!

현재는 PathService로 분리하면서 sectionDao, stationDao를 활용해서 구현했습니다!
카일은 service 레이어에 service와 dao중 어느 것을 부르는 게 좋다고 생각하시나요? (왠지 프로젝트가 커지다보면 서비스에서 서비스와 레포지토리 모두를 부르는 경우도 생길 것 같은데 이런 상황도 괜찮다고 생각하시나요?)

두가지 모두 장단점이 있다고 생각해요🙂

A라는 서비스에서 B라는 서비스가 아닌 B의 다오를 부른다는 것은 B아닌 곳에서도 B의 데이터흐름을 제어할 수 있는 역할과 권한을 주는 것이기에 편리하게 사용할 수 있지만 데이터에 대한 제어권이 분산된다는 단점이 있는 것 같아요.
반대로 A라는 서비스에서 B라는 서비스를 호출하는 경우 B에서 외부에게 사용하라고 오픈해놓은 것을 사용하는 (객체와 동일하게) 것이기에 각자의 역할이 응집되고 데이터에 대한 접근도 내가 예상하고 사용하라고 만든 B메소드 내에서만 이루어지는 장점이 있는 것 같아요. 다만 A가 필요하니까 B에서 메소드를 만드는 구조가 반복되면 B의 역할이 희미해지는 단점도 존재하는 것 같아요.

상황에 따라 어떤 선택을 할 것인가가 중요한 것 같고 저는 후자에서 B서비스가 어떤 기능을 하는 어떤 메소드를 열어줄 것인가에 고민을 더 많이 하는 스타일입니다.

A가 필요하니까 B에서 메소드를 만드는 구조가 반복되면 B의 역할이 희미해지는 단점도 존재하는 것 같아요.

저도 이런 부분에서 A서비스를 위해서 B서비스에 메서드가 추가되는 것이 맞는가? 라는 생각이 들었었어요.

데이터에 대한 제어권이 분산된다

이 부분은 생각 못했는데 제가 이해하기로는 디비에 접근하는 로직이 여러 서비스에 흩어져있어서 관리하기 어렵다고 생각해도 될까요?

KIMSIYOUNG · 2022-05-19T10:15:12Z

src/test/java/wooteco/subway/domain/PathFinderTest.java

+        line2.addSection(section4To5);
+        line2.addSection(section2To4);
+
+        Path pathFinder = new Path(List.of(section1To2, section2To3, section3To5, section2To4, section4To5));


when / then이 빠졌네요🙂

추가했습니다!

KIMSIYOUNG · 2022-05-19T10:19:15Z

src/test/java/wooteco/subway/domain/PathFinderTest.java

+        station5 = new Station(5L, "station5");
+    }
+
+    @DisplayName("경로가 1-2-3-5/ 2-4-5 있을 떄, 최소경로를 조회한다.")


해당 테스트를 보고 3-5보다 4-5가 5만큼의 거리가 짧기 때문에 4-5로 가는 것을 택하는구나 라는 것을 유추하실 수 있나요? 테스트를 위한 데이터 세팅이 중복되는 경우 필드로 옮길 수 있지만 테스트는 각 단위별로 자신이 검증하고자 하는 바를 명확하게 보여주는 것이 좋은 것 같아요. 같은 생각이시라면 위에서 말한 것의 의미가 담기게 변경해보면 좋을 것 같습니다.

추가로 3개이상이 겹치는 경우나 한 라인만 있는 경우, 두개의 길이 존재하나 거리가 같은 경우 등 테스트를 조금 더 보강해주면 좋을 것 같습니다.

구간들에 대해서 과하게 필드로 뽑은 것 같습니다! 역들같은 경우는 모두 중복된 역이므로 필드로 뽑아두었고, 경로에 영향을 미치는 섹션들만 테스트마다 따로 세팅해줌으로써 구별을 하였습니다!

테스트 추가했습니다!

KIMSIYOUNG · 2022-05-19T10:20:39Z

src/test/java/wooteco/subway/domain/PathFinderTest.java

+        assertThat(fare).isEqualTo(1450);
+    }
+
+    @DisplayName("거리가 50km가 넘어가는 경우 추가로 8km마다 100원을 추가한다")


경계값에 대해서도 테스트해주면 좋을 것 같습니다. 50키로인 경우와, 49키로인 경우의 형태로요!

KIMSIYOUNG · 2022-05-19T10:21:03Z

src/test/java/wooteco/subway/acceptance/PathAcceptanceTest.java

+                .extract();
+
+        PathResponse expected = new PathResponse(List.of(station1, station2, station4, station5), 25, 1550);
+        PathResponse as = pathResponse.as(PathResponse.class);


as는 어떤 의미인가요?

변수명에 의미를 가지도록 response로 수정했습니다!

KIMSIYOUNG · 2022-05-19T10:21:15Z

src/test/java/wooteco/subway/acceptance/PathAcceptanceTest.java

+import wooteco.subway.dto.StationResponse;
+
+public class PathAcceptanceTest extends AcceptanceTest{
+    /*


요 부분은 주석인가요?

페어와 함께 진행 할 일들을 적었는데 삭제를 안했었네요.. 주석 제거했습니다!

awesomeo184 and others added 12 commits May 17, 2022 14:24

init project

d232013

docs: 기능 구현 목록 작성

cfc85df

feat: 최단 경로 및 요금 계산 기능 구현

ab8ba46

feat: 최단 경로 및 요금 계산 api 구현

85fad79

refactor: 클래스 명 변경 PathFinder -> Path 및 메서드 명 변경

f0edfaf

feat: SectionDao findAll 메서드 구현

b683a22

refactor: Path에서 Line의 의존성 제거

d41329c

refactor: Section이 line_id를 필드로 가지도록 수정

0f47a70

refactor: dirtycontext 제거

0efd8b5

refactor: transactional 추가

ef5e2f2

refactor: 경로가 존재하지 않을때 예외 사항 추가

554405a

refactor: null비교 메서드 수정

0115e45

KIMSIYOUNG approved these changes May 19, 2022

View reviewed changes

KIMSIYOUNG merged commit 3ccf620 into woowacourse:gyuchool May 19, 2022

KIMSIYOUNG mentioned this pull request May 28, 2022

[Spring 경로 조회 - 2단계] 기론(김규철) 미션 제출합니다. #287

Merged

[Spring 경로 조회 - 1단계] 기론(김규철) 미션 제출합니다. #181

[Spring 경로 조회 - 1단계] 기론(김규철) 미션 제출합니다. #181

Uh oh!

Conversation

Gyuchool commented May 17, 2022

Uh oh!

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

KIMSIYOUNG May 22, 2022 • 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!

Gyuchool May 20, 2022 • 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!

Gyuchool May 23, 2022 • 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KIMSIYOUNG May 22, 2022 •

edited

Loading

Gyuchool May 20, 2022 •

edited

Loading

Gyuchool May 23, 2022 •

edited

Loading