[럿고] 지하철 경로 조회 - TDD 미션 제출합니다. #33

ksy90101 · 2020-05-15T08:26:30Z

안녕하세요 처음뵙겠습니다! 구구리뷰어님!
리뷰 받고 궁금했던 점들을 따로 DM으로 물어 보겠습니다~
잘부탁드리겠습니다!

- 지하철 역 순서대로 정렬하는 sortBySubwayRule 메서드 추가

- 레이어 사이 데이터 통신을 Entity -> DTO로 수정 - URL에서 api를 명시

- graph를 구하는 로직 리팩토링 - station을 구하는 로직 Map으로 변경

…input이 아닌 select로 입력받도록 수정"

- DML 분리 - Gradle에 thymeleaf 추가 - @EnableJdbcAuditing 추가

kang-hyungu

안녕하세요 럿고, 리뷰어 구구입니다 👋
구현한 코드 확인하고 몇 가지 피드백 남겼어요
확인해보시고 궁금한 점 생기면 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-17T07:13:55Z

src/main/java/wooteco/subway/admin/controller/validator/PathValidator.java

+
+import wooteco.subway.admin.dto.PathRequest;
+
+@Component


굳이 믿으로 등록할 필요는 없겠네요
메서드도 멤버 변수를 쓰지 않으니 static으로 처리하면 어떨까요

bean인데 믿으로 써놨네요;;;

kang-hyungu · 2020-05-17T07:17:47Z

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

 import wooteco.subway.admin.repository.LineRepository;
 import wooteco.subway.admin.repository.StationRepository;

-import java.util.List;
-
 @Service
 public class LineService {
    private LineRepository lineRepository;


서비스 멤버변수는 final을 붙여주세요

cud에 대한 트랜잭션 처리가 없네요
트랜잭션 처리가 필요한 메서드에 어노테이션을 추가해보면 어떨까요

kang-hyungu · 2020-05-17T07:27:13Z

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

    }

    public void updateLine(Long id, LineRequest request) {
-        Line persistLine = lineRepository.findById(id).orElseThrow(RuntimeException::new);
+        Line persistLine = lineRepository.findById(id)


findBy 메서드를 사용하면 중복을 제거할 수 있을거에요

kang-hyungu · 2020-05-17T07:36:33Z

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

+        List<Station> stations = stationRepository.findAllById(lineStationsIds);
+
+        return lineStationsIds.stream()
+            .map(lineStationsId -> stations.stream()


stations.stream()
...
이 부분을 메서드로 추출하면 2뎁스를 제거할 수 있겠네요

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

kang-hyungu · 2020-05-17T07:51:08Z

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

+        final Station startStation = getStationWithValidation(stations, pathRequest.getSource());
+        final Station endStation = getStationWithValidation(stations, pathRequest.getTarget());
+
+        DijkstraShortestPath<Station, Edge> dijkstraShortestPath =


최단 시간 최소시간 계산하는 로직들을 Edge 클래스로 캡슐화하면 서비스 객체에 비즈니스 로직을 처리하는 책임을 제거할 수 있겠네요

kang-hyungu · 2020-05-17T07:53:05Z

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


 @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
 @Sql("/truncate.sql")
-public class PageAcceptanceTest {
+public class PageAcceptanceTest extends AcceptanceTest {


상속은 신중하게 사용해야 합니다
각 테스트마다 분리해서 작성해야 AcceptanceTest에 변경이 생겼을 때 영향 받지 않습니다.
이펙티브 자바 3판 아이템 18, 19 참고하세요

중복을 제거하기 위해 어떤 부분만 상속하는게 좋을까요?
get, post, put, delete 를 공통으로 처리하는 부분만 부모 클래스에 두면 어떨까요?

중복코드가 아닌 것도 많이 들어있었네요! 수정했습니다!

kang-hyungu · 2020-05-17T07:54:21Z

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

+
+    @BeforeEach
+    void setup() {
+        StationResponse stationResponse1 = createStation("강남역");


테스트 코드도 유지보수해야되는 코드에요
불용어 대신 변수명을 강남역으로 하면 어떨까요?
StationResponse 강남역 = createStation("강남역");

kang-hyungu · 2020-05-17T07:56:15Z

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

+		line.addLineStation(new LineStation(null, 4L, 10, 10));
+
+		assertThat(line.getStations()).hasSize(4);
+		LineStation lineStation = line.getStations().stream()


테스트 코드에서 로직 구현하기 보단 Line 객체에 기능 추가하는게 낫지 않을까요?

kang-hyungu · 2020-05-17T07:57:53Z

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

@@ -105,7 +118,7 @@ public void removeLineStationById(Long stationId) {
        stations.remove(targetLineStation);
    }

-    public List<Long> getLineStationsId() {
+    public List<Long> getLineStationsIds() {


메서드 내 2뎁스 코드를 1뎁스로 바꿔보아요

while을 for문으로 변경하여 뎁스를 없앴습니다!

ksy90101 · 2020-05-18T12:53:15Z

꼼꼼한 피드백 감사합니다! 많이 깔끔해진 것 같습니다~ 아직 남은 부분이 남았다면 남겨주시면 감사하겠습니다!

kang-hyungu

럿고 피드백 반영 잘 하셨어요
몇가지 추가 피드백 남겼어요
확인해보시고 궁금한 점 생기면 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-21T22:33:37Z

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

+    }
+
+    @Transactional(readOnly = true)
+    public List<Station> sortBySubwayRule(List<Long> lineStationsIds) {


정렬하는 기능은 안보이는데 메서드명에 sort가 있네요
find가 맞지 않을까요?

findAllById(ids)를 했을때, ids안에 있는 순서가 아니라 default order By인 PK으로 정렬이 되어서 나와서 다시 순서를 맞춰주기 위해 만든 메서드입니다.
따라서 return에 있는 스트림이 정렬한다는 의미로 이름을 이렇게 지었습니다....
혹시, 사실 find후에 정렬을 하는 것이여서 이름이 조금 맞지 않는 것 같습니다..
혹시 다른 이름 추천해주실게 있을까요??

다시 보니 sortBySubwayRule는 다른 메서드에서 정렬해준 값을 전달 받은 순서 그대로 매핑하고 있군요
그럼 더 이상한데요. 메서드 이름처럼 정렬하는게 아니라 정렬된 값이랑 매핑 시켜주네요
정렬은 line 객체에 의존하니 line에 List stations를 전달해서 정렬시키는게 낫지 않을까요?

Suggested change

public List<Station> sortBySubwayRule(List<Long> lineStationsIds) {

public List<Station> sortBySubwayRule(Line line) {

List<Station> stations = stationRepository.findAllById(line.getLineStationsIds());

return line.getStations(stations);

}

말씀하시는 방법으로 변경했는데, 맞는지를 모르겠습니다.

DB 성능을 생각해서 Station를 다 불러오지 않고 필요한 Station을 불러오기 위해 findAllById를 사용하는데 이때 Line에 있는 getLineStationIds가 필요합니다. 따라서 이걸 호출하고, 다시 line에다가 stations를 넣어서 findStations()를 할때 다시 getLineStationIds를 만드는 메서드를 호출하는 방식으로 하는데, 방법이 조금 이상한 거 같아서.. 혹시 충고해주실 부분이 있다면 알려주시면 감사하겠습니다!

kang-hyungu · 2020-05-21T22:35:17Z

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

+            .map(Line::getStations)
+            .flatMap(Collection::stream)
+            .filter(lineStation -> Objects.nonNull(lineStation.getPreStationId()))
+            .forEach(lineStation -> {


Consumer 처리하는 부분도 메서드로 추출하면 코드가 깔끔해지겠네요

kang-hyungu · 2020-05-21T22:38:49Z

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

+            .collect(toMap(Station::getId, station -> station));
+        final List<Line> lines = lineRepository.findAll();
+        Path path = new Path(stations, lines);
+        GraphPath<Station, Edge> graphPath = path.getGraphPath(pathRequest.getSource(), pathRequest.getTarget(),


path 클래스 생성할 때 source, target, type에 대한 정보도 넘기면 이 로직도 감출 수 있겠네요
distance, duration도 path 클래스에서 계산하고 결과만 반환해주도록 수정해보아요

Graph 객체를 만들어서 Graph를 생성하는 역할을 주었고, Path에서는 Path 관련 값들을 반환할 수 있도록 변경하였습니다.

kang-hyungu · 2020-05-21T22:39:47Z

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

+
+import wooteco.subway.admin.exception.NotConnectEdgeException;
+
+public class Path {


path 클래스에 대한 테스트 코드를 추가해서 의도한대로 동작하는지, 예외가 발생하면 어떻게 처리하는지 점검해보아요

kang-hyungu · 2020-05-21T22:41:27Z

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

-import java.util.Map;
+import io.restassured.RestAssured;
+import io.restassured.specification.RequestSpecification;
+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-21T22:43:17Z

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

+    @DisplayName("전체 노선별 지하철 역 전체 조회")
+    @Test
+    void showWholeLineStation() {
+        LineResponse lineResponse1 = createLine("브라운선");


Suggested change

LineResponse lineResponse1 = createLine("브라운선");

LineResponse 브라운선 = createLine("브라운선");

테스트 코드도 변수명을 읽기 좋게 만들어야 코드 파악하기 용이해져요

ksy90101 · 2020-05-24T12:12:34Z

테스트 코드를 리팩토링했는데, 원하는 방향인지 의문입니다.
다른 피드백이 있다면 따끔하게 알려주세요!

kang-hyungu

럿고, 피드백 반영한 부분 확인했어요
조금더 보완하면 좋을 부분에 피드백 남겼어요
확인해보시고 잘 이해 안되는 부분은 코멘트 남기거나 dm 주세요

kang-hyungu · 2020-05-25T00:07:11Z

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

-        final List<Edge> edgeList = graphPath.getEdgeList();
-        final int distance = edgeList.stream().mapToInt(Edge::getDistance).sum();
-        final int duration = edgeList.stream().mapToInt(Edge::getDuration).sum();
+        Map<Long, Station> stations = getStations();


Map<Long, Station> stations = getStations(); List<Line> lines = lineRepository.findAll(); Path path = new Path(pathRequest.getSource(), pathRequest.getTarget(), pathRequest.getType(), stations, lines); List<Station> shortestPath = path.getShortestPath(); int distance = path.getDistance(); int duration = path.getDuration(); return new PathResponse(StationResponse.listOf(shortestPath), distance, duration);

거리 계산을 온전히 path 객체에게 맡기도록 수정해보는게 좋겠네요

kang-hyungu · 2020-05-25T00:09:04Z

src/main/java/wooteco/subway/admin/util/StationsUtil.java

+
+import wooteco.subway.admin.domain.Station;
+
+public class StationsUtil {


findStationWithValidation 메서드는 path 클래스에 추가해도 되지 않나요?

이 부분에 대해서 고민을 했었는데, Path 객체에서도 쓰이고 Graph 객체에서도 쓰이다 보니, 중복 코드라고 생각해서 고민하다 보니 Util을 따로 만든거 같습니다. 이럴때는 혹시 어떻게 하는게 좋을까요?

Map<Long, Station> stations을 일급 컬렉션으로 만들고 그 일급 컬렉션에 기능 추가하면 되겠네요
컬렉션인 Map 형태로 파라미터 주고 받으면 일급 컬렉션으로 묶을 수 없을지 고민해보는게 좋아요

kang-hyungu · 2020-05-25T00:13:59Z

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

 import wooteco.subway.admin.dto.StationResponse;

 import java.util.List;

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

-public class StationAcceptanceTest extends AcceptanceTest {
+public class StationManageAcceptanceTest extends StationAcceptanceTest {


StationAcceptanceTest를 상속하지 말고 AcceptanceTest만 상속해서 필요한 api 직접 호출해서 테스트하는게 좋겠네요

kang-hyungu · 2020-05-25T00:17:05Z

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

+
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
+@Sql("/truncate.sql")
+public class AcceptanceTest {


AcceptanceTest는 수정 잘 하셨네요
그런데 이 클래스를 상속 받은 자식 클래스 각각에서 테스트 코드가 작성하는게 좋겠어요
예를 들어 StationAcceptanceTest 클래스는 StationController에 있는 api가 의도한대로 동작하는지 테스트하는 식으로요.
StationManageAcceptanceTest를 따로 만들 필요는 없어요

ksy90101 · 2020-05-28T02:09:21Z

안녕하세요! 구구님! 제네릭 방식은 너무 좋았습니다~ 훨씬 테스트 코드가 사용하기 편리해진 것 같습니다. 부족한 부분 지적해주셔서 감사합니다!

kang-hyungu

럿고, 피드백 반영 잘 하셨어요
질문 주신 내용은 답변 남겼고 다음 미션에 반영해보면 좋겠네요
pr은 머지 처리할게요
구현 하느라 수고하셨어요

aegis1920 and others added 23 commits May 12, 2020 15:12

feat : 인수 테스트 작성

5dc9f2e

test : getLineDetails() 테스트 구현

2252cec

feat : 전체 노선의 지하철 역을 조회하는 기능 구현

c4189e5

- 지하철 역 순서대로 정렬하는 sortBySubwayRule 메서드 추가

refactor : LineController와 LineService 수정

56c0019

- 레이어 사이 데이터 통신을 Entity -> DTO로 수정 - URL에서 api를 명시

refactor : StationService 추가, URL에 api 경로 추가

1bc4053

refactor : 서비스 page 컨트롤러 구현 및 pageController 분리

c64a65e

feat : Etag 적용

2b785cc

docs : README.md 추가

f9be1b0

test : 인수 테스트 코드 작성

3671e7a

Route 컨트롤러 테스트 및 RouteController 컨트롤러 작성

cf9bfb1

test : 외부 라이브러리 JgraphtdApi 테스트 작성

20ca832

test : path를 반환하는 기능 테스트 구현

eb31efd

feat : 최단 거리 경로를 구하는 기능 추가

1c20e93

refactor : pathService 리팩토링

651a3bd

- graph를 구하는 로직 리팩토링 - station을 구하는 로직 Map으로 변경

refactor : 시간과 거리를 포함하고 있는 Edge 클래스 추가

adf4022

feat : 전체노선 화면과 전체노선 api 연동

e3b0d64

chore : 프론트엔드 샘플 코드 추가

d6a2ff1

feat : 경로 조회 하면 연동 구현, refactor : Line에 backgroundColor 필드 추가, 지하철역을 …

759f40b

…input이 아닌 select로 입력받도록 수정"

feat : 출발역과 도착역이 같은 경우 및 존재하지 않는 역을 입력했을 시 예외처리

3d1be64

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

794b293

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

165b91a

feat : pathType을 만들어서 최소 거리와 최소 시간을 구할 수 있도록 기능 구현

3525f49

feat : RequestDTO에 대한 validation 추가

3b0f116

- DML 분리 - Gradle에 thymeleaf 추가 - @EnableJdbcAuditing 추가

kang-hyungu requested changes May 17, 2020

View reviewed changes

ksy90101 added 6 commits May 17, 2020 19:51

refactor : PathValidator Component 어노테이션 제거 및 static 처리

9b56415

refactor : LineService 리팩토링 및 transactional 설정

2c0a48f

refactor : Station, Line Service Transactional 추가

a7aabde

refactor : PathReponse에 stations가 DTO를 가지도록 변경

c5e9a65

refactor : getLineStationsIds() 메서드 리팩토링

f790e74

refactor : path를 구하는 로직을 path 도메인에게 위임하는 리팩토링

fc1a602

ksy90101 added 2 commits May 18, 2020 21:36

refactor : AcceptanceTest에서 중복된 코드가 아닌 것들을 각자 테스트 클래스로 이동

dc1d96a

feat : lineStation을 찾는 기능을 line에 추가

d626b1c

kang-hyungu requested changes May 21, 2020

View reviewed changes

ksy90101 added 4 commits May 24, 2020 14:06

refator : Edge를 더하는 것과 EdgeWeight를 지정하는 부분을 메서드로 분리

95ceefb

refator : Graph 객체를 만들어서 Graph 만드는 기능 구현

360861e

refactor : 인수 테스트 메서드 분리 리팩토링

b055416

test : PathTest 작성

19a8bfb

kang-hyungu requested changes May 25, 2020

View reviewed changes

ksy90101 added 2 commits May 27, 2020 13:42

refactor : sortBySubWayRule() 메서드 리팩토링

81ca55d

refactor : 거리 계싼을 온전히 Path 객체에 맡기도록 수정

371f212

refactor : 인수 테스트 수정:

7d09ddd

kang-hyungu approved these changes May 31, 2020

View reviewed changes

kang-hyungu merged commit 328fc0b into woowacourse:ksy90101 May 31, 2020

-    public List<Station> sortBySubwayRule(List<Long> lineStationsIds) {
+    public List<Station> sortBySubwayRule(Line line) {
+        List<Station> stations = stationRepository.findAllById(line.getLineStationsIds());
+        return line.getStations(stations);
+    }


		import wooteco.subway.admin.exception.NotConnectEdgeException;

		public class Path {

	LineResponse lineResponse1 = createLine("브라운선");
	LineResponse 브라운선 = createLine("브라운선");


		import wooteco.subway.admin.domain.Station;

		public class StationsUtil {

[럿고] 지하철 경로 조회 - TDD 미션 제출합니다. #33

[럿고] 지하철 경로 조회 - TDD 미션 제출합니다. #33

Conversation

ksy90101 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksy90101 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

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

ksy90101 commented May 24, 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

ksy90101 commented May 28, 2020

kang-hyungu left a comment

Choose a reason for hiding this comment