Skip to content
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

[Spring 지하철 경로 조회 3단계] 다니(이다은) 미션 제출합니다. #128

Merged
merged 16 commits into from Jun 1, 2021

Conversation

da-nyee
Copy link

@da-nyee da-nyee commented May 18, 2021

안녕하세요, 게이츠! 다니입니다 🙌

이번 단계에서는 다익스트라 알고리즘을 활용해서 출발역과 도착역 사이의 최단 거리를 구하는 기능을 구현했어요!
또, frontend 패키지에 있던 모든 TODO도 작성 완료했습니다 ~!

이번에도 코드에서 부족한 부분 전부 지적해주세요!!
피드백 기다리고 있겠습니다!

매번 귀한 시간 내주셔서 감사합니다 🙇‍♀️✨

@da-nyee
Copy link
Author

da-nyee commented May 20, 2021

📚 다니의 학습 로그

[Spring] Interceptor - CORS Issue - 3.5

내용

태그

{Spring}, {Interceptor}, {CORS}, {issue}, {Preflight Request}

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 다니!
미션 잘 구현해주셨어요. 👍

몇가지 생각할 거리 남겨뒀으니 확인 부탁드려요 🙇‍♂️

}

public PathResponse findShortestPaths(Long sourceStationId, Long targetStationId) {
Station sourceStation = stationDao.findById(sourceStationId)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PathService에서 StationDao를 사용해서 직접 Station 정보를 조회할 수도 있지만, StationService에게 위임해서 처리할 수도 있을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


import static java.util.stream.Collectors.toSet;

public class Lines {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일급 콜렉션 👍

this.shortestPath = new DijkstraShortestPath<>(graph(lines));
}

public WeightedMultigraph<Station, DefaultWeightedEdge> graph(Lines lines) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래와 같이 SubwayMap 객체를 만들어 보는 건 어떨까요?

public class SubwayMap {
    private final WeightedMultigraph<Station, DefaultWeightedEdge> map;
    private final ShortestPathFinder shortestPathFinder;
    ...

    public Path(도메인 객체로 리턴) getShortestPath(Station source, Station target) {
           ...
          List<Station> stations = shortestPathFinder.getShortestPath(source, target)
           ...
          return new Path(stations, distance);
    }
}

SubwayMap에게 최단 경로를 구하도록 요청하면 SubwayMap은 다시 ShortestPathFinder에게 작업을 위임하는 구조는 어떤가요?

무조건 이렇게 변경해야해라는 의미는 절대 아니고, 고민해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

게이츠가 말씀하신 구조로 바꾸니까 SubwayMap, ShortestPath 객체에게 각각 알맞는 책임이 부여된 것 같아요!
좋은 방법 제시해주셔서 감사합니다 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 제 개인적인 의견을 말씀드리면, 만약 저라면 SubwayMap에서는 PathFinder인터페이스를 의존하게 하고 PathFinder구현체인 DijkstraPathFinder(jgrapht 사용한 구현체)를 DI로 주입해줄 것 같아요.

당연히 DijkstraPathFinder에서 WeightedMultigraph도 의존하게 하구요.

이렇게 되면 추후 다른 라이브러리를 사용하더라도 SubwayMap은 PathFinder 인터페이스를 의존하기 때문에 영향이 없을 것이니까요.

참고만하세요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependency

게이츠가 제안하신 대로 클래스 의존 관계를 위와 같이 변경했습니다!

SubwayMap이 ShortestPath 인터페이스를 의존하고, 해당 인터페이스를 DijkstraPath가 구현하게 만들었어요 !
(PathFinder -> ShortestPath, DijkstraPathFinder -> DijkstraPath 로 네이밍을 했습니다 !)

이렇게 리팩토링 하니까 이전과 다르게 OCP, DIP를 지킬 수 있게 된 것 같아요!
이번에도 좋은 방법 제시해주셔서 감사합니다 ~!

Station targetStation = stationDao.findById(targetStationId)
.orElseThrow(StationNonexistenceException::new);

Path path = new Path(new Lines(lineDao.findAll()));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path라고 할 수도 있지만, 가독성을 위해 SubwayMap이라고 표현하는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@da-nyee
Copy link
Author

da-nyee commented May 26, 2021

안녕하세요, 게이츠!

코드 리뷰 감사합니다 !
남겨주신 코멘트 전부 반영했습니다 🙌

이 피드백을 읽다가 궁금한 점이 하나 생겼어요 !

Service에서 여러 DAO에 의존하는 구조 vs Service에서 여러 Service에 의존하는 구조

게이츠는 어떤 방식을 선호하시나요??

저번 지하철 노선도 관리 미션을 할 때는 전자의 구조로 구현을 했어요. 그래서 이번에도 동일한 구조로 코드를 작성했어요.
그런데 해당 피드백을 보고 후자의 구조로 구현할 수도 있겠다 생각했고, 여러 DAO 대신 여러 Service를 참조하게 변경했어요 !

두 구조에 대한 게이츠의 생각이 궁금해요! 답변해주시면 감사하겠습니다 ✨

항상 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

- SubwayMap -> ShortestPath 인터페이스 의존
- ShortestPath 인터페이스 <- DijkstraPath 구현체 생성
@da-nyee
Copy link
Author

da-nyee commented May 28, 2021

안녕하세요, 게이츠! 다니입니다 🙌

추가 리뷰 반영했습니다!
확인해주시면 감사하겠습니다 🙇‍♀️❣️

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 다니!

피드백 잘 반영해주셨네요 💯
특히 높은 응집도, 낮은 결합도(High Cohesion, Loose Coupling)를 반영하시려고 노력한 점 인상 깊었습니다.

이만 merge하겠습니다~
고생하셨습니다

Station target = stationService.findStationById(targetId);

Lines lines = new Lines(lineService.findLines());
SubwayMap subwayMap = new SubwayMap(new DijkstraPath(lines));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설계를 고민하신 부분이 느껴지는 부분이네요. 👍

@serverwizard serverwizard merged commit a910e32 into woowacourse:da-nyee Jun 1, 2021
@serverwizard
Copy link

serverwizard commented Jun 1, 2021

Service에서 여러 DAO에 의존하는 구조 vs Service에서 여러 Service에 의존하는 구조

게이츠는 어떤 방식을 선호하시나요??

A Service에서 B조회 기능이 필요한 경우 B DAO를 통해 직접 조회할 수도 있지만, B Service에게 위임하여 처리한다면
추후 다른 서비스에서 B 조회 기능이 필요할 때도 B Service에게 위임해서 처리할 수 있다고 생각해요.

즉, 하나의 기능을 한 곳에서만 관리할 수 있는 장점이 있지 않을까라는 개인적인 생각이 있어요.
따라서, 저는 Service에 의존하는 것을 선호합니다 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants