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 경로 조회 - 1단계] 유콩(김유빈) 미션 제출합니다. #179

Merged
merged 36 commits into from
May 23, 2022

Conversation

kyukong
Copy link

@kyukong kyukong commented May 17, 2022

안녕하세요 스티치! 리뷰를 받게 된 유콩입니다!!!
1단계에서는 크게 구현한 부분이 없어 질문이 없네요.
다음 2단계에서 질문이 생기면 하겠습니다!

피드백 감사합니다. 🙇‍♀️

skullkim and others added 13 commits May 17, 2022 14:33
지하철역들 정보를 통해 최단경로를 구해 반환하는 기능 완료
출발역과 도착역을 통해 최단 경로의 거리를 계산하는 기능 완료
여러 메서드에서 중복 생성되는 인스턴스를 필드로 변경

BREAKING CHANGE: dijkstraShortestPath를 필드로 이동
as-is:
public class PathCalculator {

    private final WeightedMultigraph<Station, DefaultWeightedEdge> graph = new WeightedMultigraph<>(
            DefaultWeightedEdge.class);

    public PathCalculator(final List<Line> lines) {
        ...
    }

    ...

    public List<Station> findShortestPath(final Station source, final Station target) {
        DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath = new DijkstraShortestPath<>(graph);
        ...
    }

    public int findShortestDistance(final Station source, final Station target) {
        DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath = new DijkstraShortestPath<>(graph);
        ...
    }
}

to-be:
public class PathCalculator {

    private final DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath;

    public PathCalculator(final List<Line> lines) {
        final WeightedMultigraph<Station, DefaultWeightedEdge> graph = new WeightedMultigraph<>(
                DefaultWeightedEdge.class);
        for (Line line : lines) {
            final List<Section> sections = line.getSections();
            addSectionInGraph(graph, sections);
        }
        dijkstraShortestPath = new DijkstraShortestPath<>(graph);
    }

    private void addSectionInGraph(final WeightedMultigraph<Station, DefaultWeightedEdge> graph,
                                   final List<Section> sections) {
        for (Section section : sections) {
            final Station upStation = section.getUpStation();
            final Station downStation = section.getDownStation();

            graph.addVertex(upStation);
            graph.addVertex(downStation);
            graph.setEdgeWeight(graph.addEdge(upStation, downStation), section.getDistance());
            graph.setEdgeWeight(graph.addEdge(downStation, upStation), section.getDistance());
        }
    }

    public List<Station> findShortestPath(final Station source, final Station target) {
        ...
    }

    public int findShortestDistance(final Station source, final Station target) {
        ...
    }
}
경로를 조회하는데 사용하는 경로 수정

BREAKING CHANGE: API 문서에 맞는 경로로 수정
as-is:
GET: /path

to-be:
GET: /paths
Copy link

@lxxjn0 lxxjn0 left a comment

Choose a reason for hiding this comment

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

유콩, 이번에 지하철 경로 조회 미션을 리뷰하게 된 스티치입니다 :)

전체적으로 구현도 잘 하시고 테스트도 잘 작성해주신 것 같은데 몇 가지 부분에 대해서 피드백 남겨놨으니 한 번 확인해보시고 반영 부탁드릴게요 👍🏼

추가로 .DS_Store와 같은 버전 관리에 불필요한 파일들도 함께 관리되고 있어요! .gitignore 등을 활용해서 버전 관리에 필요하지 않은 파일들은 제거 부탁드리겠습니다 :)

src/main/java/wooteco/subway/dao/CommonLineDao.java Outdated Show resolved Hide resolved
@@ -0,0 +1,28 @@
package wooteco.subway.domain;

public class Fare {
Copy link

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.

1단계에서의 요금 계산은 거리에 따른 결과라서 따로 객체를 만들지 고민했었습니다. 현재 단계에서는 굳이 필요하지 않은 객체이지만 이후에 2단계에서 요금관련 정책이 생겨 나중에 리팩토링할때를 위해 미리 분리해두었습니다.

Copy link

Choose a reason for hiding this comment

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

아, 제가 말씀드린건 Fare라는 클래스를 클래스가 아닌 객체로 사용하는 것에 대한 질문이었어요!! 값을 필드로 가지고 있거나 하는 경우에는 객체로 관리하는 것이 맞다고 생각하는데 현재는 메서드만 존재해서요!! 그렇기에 굳이 객체로 사용해야 하나? 싶은 생각이 들었어요!

Copy link
Author

Choose a reason for hiding this comment

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

앗 잘못 이해했군요!! 🤣 스티치의 말대로 요금을 계산하는 클래스는 딱히 관리할 상태가 없네요. static 메서드로 수정하고 생성자를 호출하지 못하도록 막아두었습니다!

src/main/java/wooteco/subway/domain/Fare.java Outdated Show resolved Hide resolved
src/main/java/wooteco/subway/domain/PathCalculator.java Outdated Show resolved Hide resolved
src/main/java/wooteco/subway/domain/PathCalculator.java Outdated Show resolved Hide resolved
src/main/java/wooteco/subway/service/LineService.java Outdated Show resolved Hide resolved
src/main/java/wooteco/subway/service/LineService.java Outdated Show resolved Hide resolved
src/main/java/wooteco/subway/util/Converter.java Outdated Show resolved Hide resolved
@kyukong
Copy link
Author

kyukong commented May 21, 2022

안녕하세요 스티치!! 다시 코드리뷰 요청 드립니다.

디엠으로도 말씀드렸지만 repository 계층을 삭제하였습니다. 하나의 계층을 삭제하여 코드가 대거 수정되어 간단하게나마 계층 설명을 남기려고 합니다.

이전 코드의 계층은 다음과 같습니다.

controller -> service -> repository -> dao

현재 수정한 계층은 다음과 같습니다.

controller -> service -> dao

repository 를 사용해보려 했으나 아직까진 service 와 repository 의 차이점이 크게 와닿지 않아 현재로썬 불필요하다 생각해 제거하였습니다. 코드를 수정하는 과정에서 도메인끼리 의존하던 것들을 끊어주었습니다. (Line 이 Sections 정보를 가지지 않음)

스티치가 repository 에 관해 상세한 피드백을 남겨주셨지만 아직 repository 에 대해 정확하게 이해하고 있지 않다보니 이해하기 어려웠습니다. 나름 피드백에 대해 고민해보았으나 답변하기 어려운 것들은 답변하지 못했습니다.. 😭

코드가 크게 수정되어 죄송합니다. 다시 한번 코드 리뷰 부탁드립니다!!!! 🙇‍♀️

Copy link

@lxxjn0 lxxjn0 left a comment

Choose a reason for hiding this comment

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

유콩, 1단계 피드백 반영해주신 부분 잘 봤습니다!!

유효성 검증 로직을 validation 어노테이션을 통해서 처리하는 시도도 좋은 것 같아요!! 전체적으로 피드백 잘 반영해주셔서 2단계 진행해주셔도 될 것 같아요!! 몇가지 부분만 피드백 남겨두었습니다!

src/main/java/wooteco/subway/dao/LineDao.java Show resolved Hide resolved
@@ -0,0 +1,28 @@
package wooteco.subway.domain;

public class Fare {
Copy link

Choose a reason for hiding this comment

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

아, 제가 말씀드린건 Fare라는 클래스를 클래스가 아닌 객체로 사용하는 것에 대한 질문이었어요!! 값을 필드로 가지고 있거나 하는 경우에는 객체로 관리하는 것이 맞다고 생각하는데 현재는 메서드만 존재해서요!! 그렇기에 굳이 객체로 사용해야 하나? 싶은 생각이 들었어요!

@lxxjn0 lxxjn0 merged commit 2797b62 into woowacourse:kyukong May 23, 2022
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.

3 participants