도전 - 가장 빠른 도착 경로 조회 #230

gaegulgaegul · 2022-08-17T06:50:05Z

안녕하세요. 한솔님!
도전 미션 구현해봤습니다.
인수테스트를 작성하고 TDD를 통해 기능을 구현하려고 했지만
조건이 너무 까다롭다고 생각되어 프로덕트 코드를 작성하고 테스트를 작성하는 방향으로 구현해봤습니다.
제약 조건은 만족하지만 ArrivalTime#getTimeOfFastestPath 내부에서 객체의 값에 변경이 너무 많다고 생각하는데
이 부분은 어떻게 생각하시는지 궁금합니다.
리뷰 요청드리겠습니다.

catsbi

안녕하세요 명섭님. 추가미션은 저도 너무 오랜만에 보는지라 반갑기도하고 가물가물하네요 ㅎㅎ
꽤나 어려운미션인만큼 구현했다는 자체만으로도 감탄인데, 테스트 케이스가 제가 원하는 케이스들이 다 있어서 인상깊었습니다 👍

ArrivalTime이라는 객체에 대해 고민하고 계신 것 같은데, 제가 봐도 고민을 참 많이 하신 것 같습니다. depth에 대해 고민이라면 우선 쪼개보시죠. 그리고 하나하나에 집중해서 리팩토링을 하다보면 좀 더 윤곽이 보이지 않을까요?

catsbi · 2022-08-17T13:29:28Z

src/main/java/nextstep/subway/applicaion/LineService.java

    @Transactional
    public LineResponse saveLine(LineRequest request) {
-        Line line = lineRepository.save(new Line(request.getName(), request.getColor(), request.getAdditionalFare()));
+        Line line = lineRepository.save(request.toLineEntity());


EntitySupplier 인터페이스를 LineRequest가 구현하도록한다면, 다음과 같이 작성해서 Request객체가 toLineEntity를 제공하는지에 대한 신뢰성이 높아질 수 있고 유연성도높아질 수 있죠.

public LineResponse saveLine(EntitySupplier<Line> supplier) { Line line = lineRepository.save(supplier.supply());

Supplier를 사용하는 방식은 처음 알았습니다. 유용한 정보 감사합니다.

catsbi · 2022-08-17T13:30:18Z

src/main/java/nextstep/subway/applicaion/LineService.java

    public void updateLine(Long id, LineRequest lineRequest) {
        Line line = findById(id);
-        line.update(lineRequest.getName(), lineRequest.getColor(), line.getAdditionalFare());
+        line.update(


위처럼 여기도 EntitySupplier가 가능하다면 이렇게 가능하겠죠.

line.update(supplier.supply());

catsbi · 2022-08-17T13:31:08Z

src/main/java/nextstep/subway/applicaion/PathService.java

-        AgeDiscountPolicy ageDiscountPolicy = AgeDiscountPolicy.of(getCurrentUserAge(user.getUsername()));
-        SubwayMap subwayMap = new SubwayMap(lines);
-        Path path = subwayMap.findPath(upStation, downStation, type, ageDiscountPolicy);
+        DiscountPolicy discountPolicy = DiscountPolicy.of(getCurrentUserAge(user.getUsername()));


할인정책이 하나 이상일 경우도 생길 수 있을 때 어떤식으로 리팩토링을 고려할 수 있을까요?

체인 형식의 디자인 패턴으로 구현하면 개선할 수 있을 것 같습니다.

catsbi · 2022-08-17T13:32:34Z

src/main/java/nextstep/subway/domain/ArrivalTime.java

+    private List<Section> sections;
+    private List<Line> lines;


둘 다 일급 컬렉션을 활용해 볼 수 있을 것 같네요

catsbi · 2022-08-17T13:38:09Z

src/main/java/nextstep/subway/domain/SubwayMap/ArrivalTimeSubwayMap.java

+
+    @Override
+    protected GraphPath<Station, SectionEdge> getGraphPath(Station source, Station target) {
+        KShortestPaths kShortestPaths = new KShortestPaths(getGraph(), 100);


KShortestPaths를 잘 사용하셨네요 매직넘버도 분리해주면 좋을 것 같습니다.

catsbi · 2022-08-17T13:39:24Z

src/main/java/nextstep/subway/domain/SubwayMap/DistanceSubwayMap.java

+        SimpleDirectedWeightedGraph<Station, SectionEdge> graph = new SimpleDirectedWeightedGraph<>(SectionEdge.class);
+
+        // 지하철 역(정점)을 등록
+        lines.stream()


정점과 간선 등록등의 중복로직들이 있다면, 일종의 템플릿이라 할 수 있고 템플릿 메서드 패턴을 사용할 수도 있을 것 같네요

catsbi · 2022-08-17T13:40:03Z

src/test/java/nextstep/subway/domain/ArrivalTimeTest.java

+    @DisplayName("막차 시간(23:00) 기준으로 환승이 없는 경로 도착시간 구하기")
+    @Test
+    void noneTransferAtEndTime() {
+        // given
+        List<Line> lines = List.of(노선A, 노선B);
+        List<Section> sections = List.of(구간BC, 구간CD);
+        String time = LocalDateTime.of(LocalDate.now(), LocalTime.of(23, 0)).format(DATE_TIME_PATH);
+        ArrivalTime arrivalTime = new ArrivalTime(sections, lines, time);
+
+        // when
+        LocalDateTime value = arrivalTime.value();
+
+        // then
+        assertThat(value).isEqualTo(LocalDateTime.of(LocalDate.now().plusDays(1), LocalTime.of(5, 10)));
+    }
+
+    @DisplayName("막차 시간(23:00) 기준으로 환승이 있는 경로 도착시간 구하기")
+    @Test
+    void withTransferAtEndTime() {
+        // given
+        List<Line> lines = List.of(노선A, 노선B);
+        List<Section> sections = List.of(구간AB, 구간BE);
+        String time = LocalDateTime.of(LocalDate.now(), LocalTime.of(23, 0)).format(DATE_TIME_PATH);
+        ArrivalTime arrivalTime = new ArrivalTime(sections, lines, time);
+
+        // when
+        LocalDateTime value = arrivalTime.value();
+
+        // then
+        assertThat(value).isEqualTo(LocalDateTime.of(LocalDate.now().plusDays(1), LocalTime.of(5, 24)));
+    }
+}


이 두 케이스에 대한 테스트가 있는지가 중요했는데 잘 작성해주셨네요 👍

catsbi · 2022-08-17T13:41:29Z

src/main/java/nextstep/subway/domain/ArrivalTime.java

+        for (Section section : sections) {
+            Line line = findLine(section);
+            if (!line.equals(currentLine)) {
+                currentLine = line;
+                currentDateTime = calculateTimeInBeforeStations(currentDateTime, currentLine, section);
+
+                if (goingLastTime(currentDateTime, currentLine.getEndTime())) {
+                    return getTimeOfFirstPath();
+                }
+            }
+
+            currentDateTime = currentDateTime.plusMinutes(section.getDuration());
+        }


depth를 낮추기 위해 좀 더 고민해볼 필요가 있습니다. 나중에 차차 실무에서 어쩔수 없이 depth가 높아지는 상황은 올 수 있지만, 학습할때는 무리해서라도 depth를 지킬 필요가 있죠.

보통 개념적으로 다른 행위가 수행되거나 의미있는 로직이 중첩될 경우 depth가 길어집니다.

catsbi · 2022-08-17T13:42:58Z

src/main/java/nextstep/subway/domain/SubwayMap/ArrivalTimeSubwayMap.java

+    }
+
+    @Override
+    protected GraphPath<Station, SectionEdge> getGraphPath(Station source, Station target) {


다익스트라다 알고리즘이 내가 원하는 결과를 간략하게 잘 뽑아내지 못한다면 직접 DFS나 BFS등을 이용해서 최단경로를 구하는 방법도 있습니다.

catsbi · 2022-08-17T13:44:10Z

src/main/java/nextstep/subway/domain/ArrivalTime.java

+        return fastestDepartureTime.plusMinutes(beforeDuration);
+    }
+
+    private Line findLine(Section section) {


일급 컬렉션이 구현되어있다면 lines.indexOf(section)도 가능하겠죠

일급 컬렉션으로 구현하니 코드가 간결해지네요 ㅎㅎ

gaegulgaegul · 2022-08-30T13:38:15Z

안녕하세요. 한솔님!
제가 개인적으로 일이 생겨서 도전 과제 피드백 주신 부분을 이제서야 적용해보게 되었습니다.
요금정책에 관련해서는 책임 연쇄 패턴을 사용하여 구현할 수 있었습니다. 하지만 ArrivalTime#getTimeOfFastestPath 메서드의 내부 로직을 리팩터링하는 과정에서 전역 변수로 변경되는 값을 저장할 수 있도록 하고 메서드 분리만 적용하게 되어 코드의 가독성 측면에서 좋아졌다고 실감되진 않는 느낌이 들었습니다.
그 동안 미션을 수행하면서 다양한 피드백을 주셔서 감사합니다. 리뷰어로 활동하시느라 고생하셨습니다.

lms-elonsoft added 8 commits August 15, 2022 21:24

test 지하철 노선 시간 정보 인수 테스트 추가

45489c4

feat 지하철 노선 시간 정보 관리 기능 추가

22dbe1d

test 가장 빠른 도착 경로 조회 인수 테스트 추가

9de7f04

test 모든 경로 조회 study

a202fd4

fix 지하철 노선 시간 관리 파라미터 데이터 타입 변경

53babb3

feat 지하철 가장 빠른 도착 경로 조회 기능 추가

e247492

test 도착시간 관련 단위 테스트 추가

00736ec

chore 코드 정리

2b73ac2

catsbi requested changes Aug 17, 2022

View reviewed changes

lms-elonsoft added 5 commits August 29, 2022 23:23

refactor EntitySupplier 적용 리팩터링

2fe9fa6

refactor 요금 정책 체인 형식의 클래스 구성으로 변경

bad7298

refactor 일급 컬렉션 적용

107ac85

refactor 메소드 템플릿 패턴 적용

c1a310c

refactor 최단시간 도착경로 조회 depth 리팩터링

8ae08fe

chore 코드 정리

fd1b61c

도전 - 가장 빠른 도착 경로 조회 #230

Are you sure you want to change the base?

도전 - 가장 빠른 도착 경로 조회 #230

Uh oh!

Conversation

gaegulgaegul commented Aug 17, 2022

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaegulgaegul commented Aug 30, 2022

Uh oh!

Uh oh!