[터틀] 지하철 경로 조회 - TDD 미션 제출합니다. #48

begaonnuri · 2020-05-15T08:53:29Z

감사합니다!

- 지하철 전체 노선 조회 기능

- Etag Filter 등록

kang-hyungu

안녕하세요 터틀 👋 리뷰어 구구입니다
코드 구현 깔끔하게 잘 하셨어요 👍
몇 가지 간단한 피드백 남겼으니 확인해보시고 궁금한 점 생기면 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-18T02:57:28Z

src/main/java/wooteco/subway/admin/config/SubwayDefaultDataConfiguration.java

+public class SubwayDefaultDataConfiguration {
+    @Profile("local")
+    @Configuration
+    private class LocalDataApplicationRunner implements ApplicationRunner {


코드로 개발용 데이터 관리하는 것보다 data.sql로 데이터 초기화하는 게 어떨까요?

프론트 화면을 테스트할 때 로직을 탄 검증된 데이터로 테스트하고싶어서 이렇게 했었습니다!
말씀해 주신 것 처럼 코드로 개발용 데이터를 관리하는 것이 적절하지 않은 것 같네요.

생각해 보니 장점이라면

로직을 탄 검증된 데이터이고,

단점이라면

실제 로직을 타기 때문에 약간의 성능 저하

불필요한 코드 생성

이 있겠네요.
하지만 원하는 시나리오대로 data.sql을 만들었어도 가능했겠네요!
data.sql을 통해 초기화하도록 변경하겠습니다!
제 생각에 오류가 있다면 지적부탁드려요.

kang-hyungu · 2020-05-18T03:00:21Z

src/main/java/wooteco/subway/admin/controller/LineController.java

 import wooteco.subway.admin.service.LineService;

 import java.net.URI;
 import java.util.List;

 @RestController
+@RequestMapping("/lines")
 public class LineController {
    private LineService lineService;


final을 붙여서 필수 멤버 변수라 표시해주는게 좋습니다

final을 추가했습니다!
기능적인 차이는 크게 없지만 명시적으로 표현해주는것이라고 생각하는데
제 생각이 맞을까요?

차이가 있죠.
이 객체를 생성했을 때 필요한 멤버 변수를 초기화하도록 강제해서 객체를 설계한 의도대로 사용하도록 만들어줍니다

kang-hyungu · 2020-05-18T03:09:17Z

src/main/java/wooteco/subway/admin/controller/PathController.java

+    }
+
+    @GetMapping
+    public ResponseEntity<PathResponse> findPath(@ModelAttribute PathRequest pathRequest) {


RestController라 ModelAttribute 어노테이션 없어도 정상 동작하지 않나요?

찾아보니 @RestController가 아닌 @Controller에서도 없어도 된다고 하네요!

저도 테스트해봤는데 되는것 같네요!
@ModelAttribute 를 제거하겠습니다 :)

kang-hyungu · 2020-05-18T03:19:39Z

src/main/java/wooteco/subway/admin/domain/Line.java

@@ -15,12 +20,13 @@
    private int intervalTime;
    private LocalDateTime createdAt;
    private LocalDateTime updatedAt;
-    private Set<LineStation> stations = new HashSet<>();
+    private Set<Edge> edges = new HashSet<>();

    public Line() {


mock 객체 말고 쓰이는 곳이 없으니 private으로 만드는게 낫지 않을까요?

private으로 변경하고, 기존 테스트 코드에서 사용되던 빈 생성자를 인자가 있는 생성자로 변경했습니다!

kang-hyungu · 2020-05-18T03:20:53Z

src/main/java/wooteco/subway/admin/domain/Line.java

@@ -15,12 +20,13 @@
    private int intervalTime;
    private LocalDateTime createdAt;
    private LocalDateTime updatedAt;
-    private Set<LineStation> stations = new HashSet<>();
+    private Set<Edge> edges = new HashSet<>();


만들어둔 Edges 클래스를 활용하는 것도 괜찮겠네요

Line에 Set를 사용하던 로직을 Edges로 이동했습니다!

kang-hyungu · 2020-05-18T03:30:21Z

src/main/java/wooteco/subway/admin/domain/SubwayWeightEdge.java

+
+import java.util.function.Function;
+
+public class SubwayWeightEdge extends DefaultWeightedEdge {


상속 대신 DefaultWeightedEdge를 멤버 변수로 두는건 어떨까요?

이번 미션에서 최단경로를 구하는 부분에서 외부 API를 사용했습니다.
DefaultWeightedEdge가 필요해서 처음엔 컴포지션을 고려했습니다.

위의 구현한 부분을 보시면 메소드의 접근제어가 protected라서 상속을 해야 한다고 생각했습니다!
혹시 다른 방법이 있다면 힌트를 주실 수 있을까요? 😉

라이브러리에서 강제하는 부분이라 상속을 쓸 수 밖에 없겠네요
따로 수정하지 않으셔도 됩니다!

kang-hyungu · 2020-05-18T03:35:20Z

src/test/java/wooteco/subway/admin/acceptance/EdgeAcceptanceTest.java

+    @DisplayName("지하철 노선에서 지하철역 추가 / 제외")
+    @Test
+    void manageLineStation() {
+        StationResponse stationResponse1 = createStation(STATION_NAME_KANGNAM);


변수명을 강남역으로 쓰면 좀더 읽기 쉽지 않을까요?

테스트코드라서 변수명을 홀대했네요🙄 값과 매칭되는 변수명으로 변경했습니다!

kang-hyungu · 2020-05-18T03:37:46Z

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

+import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+
+public class PathAcceptanceTest extends AcceptanceTest {


새로운 api와 uri 경로가 추가될 때마다 AcceptanceTest가 너무 커지겠네요
get, post, put, delete에 대한 중복처리만 AcceptanceTest에서 제공하는게 어떨까요?

최대한 중복되는 로직만 AcceptanceTest에서 관리하도록 하고,
나머지 로직은 해당 테스트 클래스로 이동했습니다!

kang-hyungu · 2020-05-18T03:38:20Z

src/test/java/wooteco/subway/admin/domain/SubwayGraphTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class SubwayGraphTest {


예외 상황에 대한 테스트 케이스도 추가해보면 좋겠어요

예외 상황에 대한 테스트 케이스도 추가했습니다!

kang-hyungu · 2020-05-18T03:39:36Z

src/main/java/wooteco/subway/admin/domain/Lines.java

+                .collect(Collectors.collectingAndThen(Collectors.toList(), Lines::new));
+    }
+
+    public SubwayGraphs makeSubwayGraphs(Long sourceStationId, Long targetStationId) {


파라미터를 사용하지 않는데 의도한게 맞나요?

ㅎㅎ.. 인자 제거했습니다😅

- 테스트 충돌을 방지하기 위해 ServiceTest에서 truncate.sql을 실행하고 테스트하도록 변경

- 테스트에서 사용되던 빈 생성자를 인자가 있는 생성자로 변경

- @embeded 추가

begaonnuri · 2020-05-18T13:02:26Z

리뷰 반영했습니다!
코멘트 별로 커밋 남겼습니다😉
또한 궁금한 점을 코멘트로 남겼는데, 답변해주시면 감사하겠습니다🙇🏻‍♂️

kang-hyungu

터틀, 피드백 반영 잘 하셨어요
추가로 수정해보면 좋을 부분에 피드백 남겼어요
확인해보시고 궁금한 점 생기면 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-21T22:53:21Z

src/main/java/wooteco/subway/admin/service/LineService.java

-        lineRepository.save(persistLine);
+    @Transactional(readOnly = true)
+    public LineDetailResponse findLineWithStationsById(Long id) {
+        Line line = lineRepository.findById(id).orElseThrow(RuntimeException::new);


lineRepository.findById(id).orElseThrow(RuntimeException::new);

이 부분을 메서드로 추출하면 중복을 제거할 수 있겠네요

해당 부분을 메소드로 추출했습니다 :)

kang-hyungu · 2020-05-21T22:58:28Z

src/main/java/wooteco/subway/admin/service/PathService.java

+        Long targetId = stations.findIdByName(target);
+
+        Lines lines = new Lines(lineRepository.findAll());
+        SubwayGraphs subwayGraphs = lines.makeSubwayGraphs(sourceId, targetId);


컴파일 에러가 발생하네요

꼼꼼하게 확인하지 못했네요 죄송합니다 😭

kang-hyungu · 2020-05-21T23:01:02Z

src/test/java/wooteco/subway/admin/acceptance/AcceptanceTest.java

 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.web.server.LocalServerPort;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
 import org.springframework.test.context.jdbc.Sql;
-import wooteco.subway.admin.dto.*;
+import wooteco.subway.admin.dto.LineDetailResponse;


api가 늘어나면 AcceptanceTest 클래스에 계속 메서드가 추가 되겠네요
아래 샘플 코드처럼 get, post, put, delete 호출에 대한 중복 코드를 제거하면 AcceptanceTest가 변경될 일은 없지 않을까요?

<T> T post(String path, Map<String, String> params, Class<T> responseType) { return given(). body(params). contentType(MediaType.APPLICATION_JSON_VALUE). accept(MediaType.APPLICATION_JSON_VALUE). when(). post(path). then(). log().all(). statusCode(HttpStatus.CREATED.value()). extract().as(responseType); }

제네릭을 이렇게 적용할수도 있군요! 테스트코드의 중복을 제거했습니다 :)

kang-hyungu · 2020-05-21T23:03:58Z

src/main/java/wooteco/subway/admin/service/PathService.java

+        Long targetId = stations.findIdByName(target);
+
+        Lines lines = new Lines(lineRepository.findAll());
+        SubwayGraphs subwayGraphs = lines.makeSubwayGraphs(sourceId, targetId);


Lines 객체 생성할 때 sourceId, targetId, pathRequest.getKey() 까지 한번에 전달하면 이 로직도 Lines 객체에 감출 수 있지 않을까요?
PathDetail path = lines.getShortestPath();
이런식으로 실제 비즈니스 로직을 서비스 객체에 노출하지 않도록 변경할 수 있겠네요

해당 로직을 Lines로 이동했습니다!

비즈니스 로직을 서비스 객체에 노출하지 않도록

이 부분을 앞으로 주의해야겠네요!

kang-hyungu

터틀, 피드백 반영한 내용 확인했어요
조금 더 보완해보면 좋을 부분에 코멘트 남겼어요
수정하시면서 궁금한 점 생기면 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-25T00:32:36Z

src/test/java/wooteco/subway/admin/acceptance/AcceptanceTest.java

+						jsonPath().getList(".", StationResponse.class);
+	}
+
+	StationResponse createStation(String name) {


AcceptanceTest는 모든 인수 테스트에서 공통적으로 사용할 기능만 남기는게 좋습니다.
getStations, createStation 같은 메서드는 StationAcceptanceTest로 옮겨주세요

getStations, createStation을 비롯해 특정 인수테스트에서만 사용하는 메소드를 분리했습니다!

아래 메서드들도 각 인수 테스트로 옮겨주세요
Station, Line 같이 특정 도메인과 관련된 테스트도 AcceptanceTest에 없는게 좋습니다
상속해서 사용하다가 수정사항이 생기면 다른 상속 받은 테스트에도 영향 받습니다

kang-hyungu

머지 하기 전에 조금만 더 보완해보면 좋을 것 같아요
추가로 남긴 코멘트 한 번 더 확인해주세요!

kang-hyungu · 2020-05-25T01:18:56Z

src/test/java/wooteco/subway/admin/acceptance/AcceptanceTest.java

+						jsonPath().getList(".", StationResponse.class);
+	}
+
+	StationResponse createStation(String name) {


아래 메서드들도 각 인수 테스트로 옮겨주세요
Station, Line 같이 특정 도메인과 관련된 테스트도 AcceptanceTest에 없는게 좋습니다
상속해서 사용하다가 수정사항이 생기면 다른 상속 받은 테스트에도 영향 받습니다

begaonnuri · 2020-05-25T01:52:10Z

리뷰 반영했습니다 :)
코멘트로도, 디엠으로도 친절하게 답변해주셔서 감사합니다! 🙇🏻‍♂️

kang-hyungu

터틀 🐢 피드백 반영 잘 하셨어요 👏
pr은 머지 처리할게요
코드 구현하느라 수고하셨어요!

kang-hyungu · 2020-05-25T01:54:51Z

src/test/java/wooteco/subway/admin/acceptance/EdgeAcceptanceTest.java

+        addEdge(lineNumberTwo.getId(), hongik.getId(), shinchon.getId(), 10, 10);
+        addEdge(lineNumberTwo.getId(), shinchon.getId(), jamsil.getId(), 10, 10);
+        addEdge(lineNumberEight.getId(), jamsil.getId(), seokchon.getId(), 10, 10);
+        addEdge(lineNumberEight.getId(), seokchon.getId(), mongchon.getId(), 10, 10);

        //when
        WholeSubwayResponse wholeSubwayResponse = given().


다음 미션에서 이 부분도 수정해주세요!

pci2676 added 12 commits May 12, 2020 18:35

feat : 실습

c7b39f2

- 지하철 전체 노선 조회 기능

feat : 1단계 캐시

49474f9

- Etag Filter 등록

checkpoint

580ae6b

feat : 최단 거리 경로 찾기 기능 구현

7c0a541

feat : service에서 최단 경로 그래프 가져오기

72c05df

refactor : 최단 경로 로직 변경

72340ec

test : 테스트 실패 코드 수정

80aae87

feat : 경로 찾기 인수테스트 성공

4d6f48f

feat : Web View 연결

3928a96

refactor : StationController에서 StationRepository 의존 제거

e035eda

refactor : PathService의 데이터베이스 접근 횟수 개선

8c0cd84

fix : 출발역과 도착역이 연결이 되어 있지 않은 경우 처리

adb141e

kang-hyungu requested changes May 18, 2020

View reviewed changes

begaonnuri added 13 commits May 18, 2020 16:10

refactor: 초기 데이터를 설정하는 방법을 ApplicationRunner에서 data.sql로 변경

49e5757

- 테스트 충돌을 방지하기 위해 ServiceTest에서 truncate.sql을 실행하고 테스트하도록 변경

refactor: lineService 필드에 final 추가

ca23215

refactor: @ModelAttribute 제거

f73ee8a

refactor: 빈 생성자를 private으로 변경

83adc39

- 테스트에서 사용되던 빈 생성자를 인자가 있는 생성자로 변경

refactor: Line 클래스에서 Set<Edge>를 사용하던 로직을 Edges로 이동

5d9285b

- @embeded 추가

refactor: Edge들의 StationId를 받아오는 로직 depth 개선

8da51d9

refactor: 사용하지 않는 코드 제거

a2c770e

refactor: SubwayGraphKey의 symbol 필드 제거

427de66

test: 테스트코드의 변수명을 적절하게 변경

bc9b429

test: 하위 테스트 클래스에서만 사용되는 로직을 해당 클래스로 이동

e607a4e

test: SubwayGraphTest의 예외사항 테스트 추가

00f7cac

refactor: makeSubwayGraphs의 인자 제거

1f41d52

refactor: 설정 파일의 profiles 제거

d365f48

kang-hyungu requested changes May 21, 2020

View reviewed changes

begaonnuri added 2 commits May 25, 2020 01:24

test: AcceptanceTest의 중복 코드 제거

cb480c8

refactor: LineService의 중복 제거

1654f9e

refactor: Lines의 로직이 PathService에 드러나지 않도록 변경

c380f6f

kang-hyungu requested changes May 25, 2020

View reviewed changes

refactor: 테스트 메소드 분리

aaf87bd

kang-hyungu requested changes May 25, 2020

View reviewed changes

refactor: AcceptanceTest의 픽스쳐를 각 클래스로 이동

a3b299e

kang-hyungu approved these changes May 25, 2020

View reviewed changes

kang-hyungu merged commit 5eef614 into woowacourse:begaonnuri May 25, 2020


		import java.util.function.Function;

		public class SubwayWeightEdge extends DefaultWeightedEdge {


		import static org.assertj.core.api.Assertions.assertThat;

		class SubwayGraphTest {

[터틀] 지하철 경로 조회 - TDD 미션 제출합니다. #48

[터틀] 지하철 경로 조회 - TDD 미션 제출합니다. #48

Conversation

begaonnuri commented May 15, 2020

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri commented May 18, 2020

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri commented May 25, 2020

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begaonnuri May 18, 2020 •

edited

Loading

begaonnuri May 18, 2020 •

edited

Loading

begaonnuri May 18, 2020 •

edited

Loading

begaonnuri May 18, 2020 •

edited

Loading