[2단계 - 경로 조회 기능] 마코(이규성) 미션 제출합니다.

[aak2075]

안녕하세요 웨지! 마코입니다.

우선 기능이 동작하는 것을 목표로! 열심히 작성해봤습니다.

file changed가 많아서 부담되실 것 같네요.....ㅎㅎㅎ

리뷰 잘 부탁드립니다!!

구조 설계

1단계에서 구현했던 도메인 구조가 복잡하고 책임 분리가 잘 안되었던 것 같아서 다음과 같이 구조를 변경했습니다.
Line -> Sections -> Station
구조를 변경하다 보니 코드를 처음부터 다시 짜게 되었습니다....😅

2단계에서 추가된 경로와 요금 조회를 추가하면서 SubwayGraph, FareCalculator 객체를 추가했습니다.
경로 조회와 관련된 PathService 에서 이 객체들과 협력하는 객체들(Station, Line)을 관리하는 서비스를 의존하여 StationService나 LineService에서 필요한 객체들을 불러와서 협력하도록 로직을 구성했습니다.

궁금한 점

통합 테스트에서 직접 request를 보내서 데이터를 셋팅하고 그 다음에 기능을 테스트하도록 했습니다.
기능을 실행하기까지의 시나리오가 포함되어있어서 좋다고 생각하지만
데이터를 셋팅하는데 너무 많은 코드가 포함되어서 테스트 코드가 복잡해지기도 하는 것 같습니다.
이번 미션 정도의 테스트는 직접 request를 날려서 데이터를 셋팅하는 것은 괜찮지만 시나리오가 복잡해진다면 직접 request를 보내는 것이 아니라 sql파일로 데이터를 셋팅해놓고 통합 테스트를 진행할 것 같습니다.
이에 대한 웨지의 의견이 궁금합니다!

[sihyung92]

안녕하세요 마코~!
코드가 몰라보게 달라졌네요. 성공적인 리팩토링이라고 생각해요 ㅎㅎ 수고하셨습니다
잘해주셨지만 여기저기 참견해두었으니 참고만 해주셔요.

질답

• Q. Request 일일이 보내서 테스트 구성하니까 복잡해용 data.sql이 나을까용
• A. 현재는 DB 구조가 간단해서 data.sql을 쓰는게 더 편하실 거에요. 어플리케이션이 하는 일이 많은 레포지토리라 그런거 같아요 ㅎㅎ 제가 다루는 코드는 연관된 DB 테이블이나 이벤트가 많아서 data.sql로 처리하는게 되려 더 어렵습니다. 그래서 그냥 Request 만드는게 (Fixture 라는 네이밍으로 몇개 샘플을 미리 만들어둠) 더 편해요 ㅎㅎ 그래서 sql 보다는 Request 방식을 선호합니다. Fixture 만들기 힘들면 그냥 json파일 만들고 해당 json을 파싱해서 객체화해 쓰는 방법도 있어요. (json파일 유지보수가 어렵긴 합니다) 그때 그때 상황에 맞는 방법을 써주시면 되겠습니당.

또 요청주세요! 기다리고 있겠습니당

[sihyung92]

알고리즘 부분을 잘 감추어 추상화 해주셨네요 ㅎㅎ

[aak2075]

알고리즘이 바뀌더라도 변경이 일어나지 않아 좋을 것 같습니다 👍

[sihyung92]

WeightedMultigraph는 WeightedGraph의 구현체인데요, 조금 더 확장성 있게 구현하려면 WeightedGraph를 사용하고 클라이언트 (현재는 SubwayGraph의 정적 팩토리 메서드)가 결정하도록 하는 게 좋습니다.

[aak2075]

추상화에 의존하라는 말은 많이 들어서 익숙한 컬렉션은 잘 사용하고 있었습니다.
List<T> list = new ArrayList<>()
막상 처음 보는 라이브러리를 만나니 �어느 추상화를 의존해야 할지 고민이었습니다.
다음과 같이 상속도 하고 구현도 하고 있었기 때문이죠.

public class WeightedMultigraph<V, E>
    extends Multigraph<V, E>
    implements WeightedGraph<V, E>

최단 경로를 구하는 데에는 Multigraph와 WeightedGraph가 둘 다 필요하다는 생각이 들어서 구현체에 의존하도록 했으나
다시 고민해본 결과 인터페이스를 의존하는 것이 좋다는 결론을 내렸습니다. 이유는 다음과 같습니다.

1. 상속은 어느 정도 구현이 되어있기 때문에 구현 세부사항에 묶일 수 있습니다.
2. MultiGraph는 그래프의 종류이고, WeightedGraph는 최단 경로를 구하기 위한 속성입니다. 이 알고리즘을 구현하기 위해서는 속성이 꼭 필요하지만 그래프의 종류는 얼마든지 바뀔 수 있습니다.

어느 레벨의 추상화를 의존해야 할지도 고민이였습니다.
최상위 인터페이스인 Graph를 의존할 수도 있기 때문이죠.
하지만 목적의 명확성을 위해 가장 가끼이에 있는 인터페이스를 의존하는 것이 좋다는 결론을 내렸습니다.

따라서, 말씀해주신대로 WeightedGraph를 의존하도록 변경했습니다.
근거가 충분하다고 생각하시나요? :)

[sihyung92]

네 저두 다른 리뷰를 보다가 최상위 Graph를 사용해도 되겠다는 걸 나중에 알았네요 ㅎㅎ

가끔은 너무 과도한 추상화가 오히려 생산성을 해치는 경우도 있는데, 지하철 노선도에서 Weighted, 비중은 빼놓고 생각할 수 없는 부분이니 WeightedGraph 추상화를 적용한건 근거가 충분하다는 생각하다는 생각이 듭니다 ㅎㅎ

[sihyung92]

서비스에서 해당 객체를 매번 생성중인데요, 만약 성능이 안 나온다면 해당 객체를 bean(= 싱글톤)으로 만들어 최적화할 만한 여지가 있습니다. 미리 모든 line을 넣어두고 거리만 구하도록요 :)

서버가 부팅할 때 넣어두어도 되고, 최초 요청시에 넣어두어도 되고... 방법은 상황에 맞게 쓸 수 있겠습니당

@Configuration
public class Config {

  @Bean
  public SubwayGraph subwayGraph(){
    return new SubwayGraph(new WeightedMultigraph<>(DefaultWeightedEdge.class));
  }
}

[aak2075]

고민이 정말 많았던 부분입니다.
기존에는 상태가 없는 객체를 빈으로 등록한다 라는 개인적인 기준을 가지고 있었기 때문입니다.
빈으로 등록하면 싱글톤으로 관리되어 동시성 문제가 발생할 수 있어서 이러한 기준을 세웠습니다.

일단은 구현을 해봤습니다.
역이 추가되거나 삭제될 때 마다 다음 update 메서드를 호출하도록 했습니다

    public void update(final Lines lines) {
        final List<Station> stations = lines.getAllStations();
        final Paths paths = Paths.from(lines);

        final WeightedGraph<Station, DefaultWeightedEdge> graph =
                new WeightedMultigraph<>(DefaultWeightedEdge.class);
        stations.forEach(graph::addVertex);
        paths.addAllToGraph(graph);

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

역이 추가될 때 마다 graph를 다시 셋팅하고, 최단 경로를 새로 구하도록 했습니다.

구현을 하면서 든 생각은 이 도메인은 동시성 문제가 일어나도 상관이 없다고 생각했습니다.
역 추가, 삭제가 빈번하게 일어나지도 않을 것이고, 역을 추가, 삭제하는 사이에 경로 조회를 해서 잘못된 결과를 얻게된 사용자가 크게 불만을 품을 만한 문제도 아닐 것 같습니다.
정리하면, 삽입, 삭제보다 조회가 훨씬 빈번하게 일어날 것이고, 조회에서 성능이 좋지 않았기 때문에 성능을 개선하며 동시성 문제는 품고 가는 trade-off의 영역이라고 볼 수 있을 것 같습니다.

[sihyung92]

넵 말씀해주신 것처럼 상태를 가지게 하는 건 동시성 이슈가 발생할 수 있는 부분이고, 또 분산환경(n 대의 인스턴스)에선 대응이 안 되는 방식이라 잘 쓰이지 않습니다 ㅎㅎ 현업에선 redis 같은 싱글 스레드 스토리지나 RDB의 트랜잭션을 활용해 제어하는 경우가 많을 거에요.
리뷰의 목적은 Bean에 익숙해지게 만듦 + 입력/수정/삭제보다 조회의 비중이 높은 OLTP 프로그램의 특성에 대해 고민해볼 기회를 만드는 것이었는데요, 기존에 의도한 것 보다 깊게 숙고하시고 학습해주신거 같네요. 👍

[sihyung92]

DefaultWeightedEdge를 사용한다고 전제한 int 맵핑이라 버그가능성이 있어보입니다. 다른 가중치를 사용할 때를 대비하려면 Double로 Distance가 계산되게 두고, 응답객체에서 Int로 맵핑해주는 게 버그 가능성이 줄 것 같네요.

[aak2075]

확장성을 고려하지 못했네요.
좋은 방법입니다. 👍

[sihyung92]

1. DijkstraShortestPath 구현체 대신 ShortestPathAlgorithm라는 추상에 의존하도록 할 수 있습니다.
2. DijkstraShortestPath의 생명주기가 subwayGraph와 동일하면 되므로(subway graph가 갱신될 때만 갱신하면 되므로) 필드로 만들어두고 subwayGraph가 변화할 때만 갱신해주면 성능을 조금더 챙길 수 있겠습니다.

[aak2075]

인터페이스를 의존한다고 생각하니 ShortestPathAlgorithm를 의존해야 하는 것이 딱 보이는군요 😄 반영했습니다.

2번의 내용을 위 코멘트부분에서 반영하면서 같이 적용해봤습니다.
필드를 final키워드로 관리하지 못한다는 부분이 조금 아쉽네요.

[sihyung92]

ConnectionRequest.connectionType을 ConnectionType으로 사용했어도 될 것 같아요 ㅎ
혹은 getConnectionType 내부에서 ConnectionType.from() 을 수행해주거나요.

[aak2075]

처음엔 Enum을 파싱해준다는 생각을 하지 못했습니다.
Enum에 @JsonCreator 를 사용하여 jackson이 어떻게 파싱해줄지 결정해주도록 했습니다.
request에서 enum으로 사용하니 더 앞단에서 타입 검증이 일어나 더 안정적이네요.

[sihyung92]

네에 단점으로는 enum에 정의되지 않은 값이 파라미터로 넘어오면 Jackson에서 500을 내버린다는 단점은 있어요 ㅎㅎ
그걸 방지하기 위해 enum을 자체구현하는 느낌으로 틀어서 쓰는 경우도 봤습니다. 좀 과하다는 생각은 있어서 따로 적진 않을게요. 궁금하면 DM 주세용 ㅎㅎ

[aak2075]

@JsonCreator를 붙여 정의한 fromValue() 메서드에서 제가 지정했던 예외를 던져서 400이 반한될 줄 알았으나
jackson이 예외를 전환시켜 500이 반환되는군요.
예외 핸들링으로 사용자에게 정확한 메시지를 주는 것이 중요하다고 생각해서 400을 내리도록 처리할 것 같습니다.

그렇다면 String 타입으로 역직렬화 하고 getter에서 EndpointType.from()으로 반환하도록 하는 것이 훨씬 간단할 것 같습니다.
이렇게 했을 때 테스트에서 Request를 생성할 때 Enum을 못쓰지 않을까 생각했지만 EndpointType.UP.getValue() 요렇게 사용하면 충분할 것 같습니다.

enum을 자체 구현하는 느낌은 왠지 배보다 배꼽이 더 커지는 느낌이네요ㅎㅎ

[sihyung92]

넵 그것도 괜찮은 방법입니다
다만 Enum에 없는 값일 때 처리를 위해 Optional을 반환할지, null을 반환할지, 즉시 예외처리 할지, Null 객체를 만들지 등등 새로운 고민이 생기긴 하는데요 ㅎㅎ 각각은 정하기 나름인 것 같습니다

[sihyung92]

getPrevStationId() 같은 경우 ConnectionType과의 논리적 의존이 있습니다.
(ConnectionType이면 필수값이고, 아니면 없어도 됨)

이런 경우 API를 가지고 프론트엔드 개발자와 소통할 때나, 향후 ConnectionRequest를 유지보수하는 다른 개발자(혹은 3주 후의 나)기 혼동하기 쉽습니다. 이런 경우는 ConnectionType의 각 분기를 별도의 API로 분리하고, 엔드포인트 path를 통해 표현하는게 좋습니다.

해당 경우는 INIT, UP&DOWN, MID 세가지 경우에 대한 API가 있으면 좋겠네요.

[aak2075]

하나의 api에서 전부 다 받으려고 하니 검증도 어렵고 저도 헷갈렸습니다.
api를 분리하고 다음과 같이 자원에 대한 행위를 경로에 나타내도록 했습니다.
/lines/{lineId}/stations/{stationId}/(init | endpoint | mid)
또한, ConnectionType도 up인지 down인지를 결정하는데만 사용하게 되어서 EndpointType으로 변경했습니다.

[sihyung92]

List<StationResponse> 를 활용하는 게 더 좋을 것 같아요.

1. 실제론 "역 이름"인데 stations라는 필드명은 다소 모호합니다.
2. id를 가지고 있으면 역에 대한 추가정보가 필요할 경우 StationController를 통해 질의하거나, 프론트엔드가 알고있는 다른 정보와 조합해서 쓰기 용이합니다.
3. 나중에 StationResponse로 교체했을 때 API 하위호환성이 깨집니다. (break change라고도 부르는데용) 서버만 사전 배포하는 방식이 아니라 서버와 프론트가 동시에 배포해야하는 경우가 생깁니다. Json으로 봤을 때 String타입 이다가 Object 타입으로 변하기 때문이에요. 처음부터 StationResponse였다면 내부에 station과 관련된 필드가 여럿 추가 된 내용이 사전 배포되어도 프론트엔드 서버가 터지진 않습니다

[aak2075]

말씀해주신 2번의 이유로 자원을 나타낼 때 가능하면 Id를 포함하는 것이 좋겠네요.
프론트와의 호환성 문제는 생각해보지 못했네요! 👍

[sihyung92]

네 개발조직이 분할되어 있는 경우 (== 다른 팀이면) 간단한 수정이라도 협업에서 에너지가 많이 소비됩니다 ㅎㅎ
또 외부망에 노출된 public api라면 break change가 불가능에 가까운 경우도 많고요.
API도 하위 호환성 (예전 버전 API와 똑같이 요청해도 문제가 없도록)를 지켜지도록 유연한 API 설계를 한다면 장점이 많습니다.

[sihyung92]

네이밍에서 어떤 역할인지 추론할 수 없습니다. 또한 First, Second, Thrid가 절차적인 로직을 암시하고 있어서 이쁘게 추상화 해주신 장점인 확장성을 누리기 어렵습니다. (각 chain을 분리하거나 조합하기 부담스러워용)

각 Chain에서 어떤 계산을 하는건지 네이밍을 통해 잡아주시고, ordering이 필요하다면 빈 주입하는 곳에서 처리하거나 Chain 필드에 order를 추가해 어떤 순서로 처리되어야 하는지 명시해주는 방법도 있습니다.

[aak2075]

네이밍 때문에 나중에 순서를 바꿔서 chain을 조립하기 어렵겠군요.
클래스명을 고민하다가 결국 절차적인 네이밍으로 결정하게 되었습니다.

아래 코멘트의 방식을 적용해보면서
BasicFarePolicy, EightUnitFarePolicy, FiveUnitFarePolicy로 수정했습니다!

[sihyung92]

특정한 가격 정책이 추가될 때마다 이 부분도 함께 수정해주어야 합니다. 제 아이디어로는 Composite 패턴이 있는데, 아래 같은 방법도 있으니 참고해주셔요 ㅎㅎ

->

1. 각 Chain은 하는 역할에 맞게 xxxDistancePolicy로 구현하고, chain과 관련된 로직 및 필드를 삭제합니다.
2. 각 DiscountPolicy를 @component 로 만듭니다.
3. DiscountChain 이라는 컴포짓 객체를 아래처럼 만듭니다.

// 순환참조 오류 생기므로 Component 안 붙임
public class DiscountChain implements DistancePolicy {
    
    private final List<DistancePolicy> distanceChains;

    public DiscountChain(List<DistancePolicy> distanceChains) {
        this.distanceChains = distanceChains;
    }

    @Override
    public int calculate(final int distance) {
       // 만약 순서가 중요했다면, 각 체인에 order라는 필드를 넣고 order로 sorting합니다. 정렬 기준은 숫자로 넣어도 되고, 숫자에 의존하기 싫다면 Priority Enum(제일 먼저, 가장 나중에)같은 방법을 사용해도 무방합니다.
        int fare = 0;
        for (DistancePolicy distanceChain : distanceChains) {
            fare += distanceChain.calculate(distance);
        }
        return fare;
    }
}

4. 해당 Configuration은 아래와 같이 리팩토링해 스프링 꿀을 빱니다.

@Configuration
public class FareConfiguration {

    // Component 설정해준 애들 여기로 다 모임
    private final List<DistancePolicy> distancePolicies;

    public FareConfiguration(List<DistancePolicy> distancePolicies) {
        this.distancePolicies = distancePolicies;
    }

    @Bean
    public FareCalculator fareCalculator() {
        return new DiscountChain(distancePolicies);
    }
}

요러면 차후에 가격 정책이 추가되어도 DiscountPolicy만 구현해도 될거 같아요.

[aak2075]

제가 처음에 생각했던 방식이랑 비슷한데 Composite 패턴이였군요!
물론 스프링 꿀을 빠는 것은 생각하지 못했지만요 😄
제가 생각했던 방식은 컴포지트가 아니라 외부의 객체가 합을 계산하도록 했던 방식이라 알고리즘의 캡슐화가 깨진다고 생각해서 책임 연쇄 패턴으로 변경했었습니다.

직접 적용을 해보았습니다

    @Override
    public int calculate(final int distance) {
        if (distance >= MAX_CALCULATE_RANGE) {
            return (int) ((Math.ceil((MAX_CALCULATE_RANGE - 1) / UNIT) + 1) * FARE);
        }
        if (distance <= MIN_CALCULATE_RANGE) {
            return 0;
        }
        return (int) ((Math.ceil((distance - 1) / UNIT) + 1) * FARE);
    }

각 구현체가 자신의 범위에 해당하는 부분만 계산하도록 하였고, 순서에 의존하지 않도록 되었습니다.

DistancePolicy가 추가되거나 변경되더라도 @configuration 파일을 수정하지 않아도 된다는 것이 큰 장점인 것 같습니다.

[sihyung92]

넵 적용을 잘 해 주셨군요 ㅎㅎ
사실 저는 Composite까지는 잘 안하고 DistancePolicy 를 예로들면 다음처럼 만들어서 쓰는 편입니다 (취향적인 부분이랄까요)

public interface DistancePolicy {
    boolean isProper(int distance);

    int calculate(int distance);
}

그리고 DistancePolicy의 1급 컬렉션을 만들거나 사용처에서 그대로 List를 받아씁니다.

public class DistancePolicies {
    List<DistancePolicy> policies;
    ...

    public int calculate(int distance) {
         for(DistancePolicy policy:policies){
             if(policy.isProper(distance)){
                  return policy.calculate(distance);
             }
         }
     }
}

composite 패턴이 동일한 인터페이스로 bulk 연산도 처리해주니 더 확장성이 좋은데, 전 이상하게 위 방식으로 짜게 되더라구요 ㅋ

[aak2075]

안녕하세요 웨지! 마코입니다!
고민할 내용이 많아서 즐거웠던 피드백 반영이었습니다. :)
웨지가 참고하라고 말씀해주신 방법들 모두 직접 장단점을 느껴보고 싶어서 적용을 해봤습니다.
피드백을 반영하면서 근거들이 충분했는지, 웨지의 의견은 어떤지 궁금한 부분도 있으니 확인해주시면 감사하겠습니다.

테스트 리팩토링과 커스텀 예외 적용하는 것은 반복적인 작업이 많을 것 같아 먼저 리뷰 요청을 보내 놓고 천천히 반영해보겠습니다.
(의견을 더 주고 받고 싶어서요ㅎㅎㅎ)

리뷰 잘 부탁드리며 행복한 주말 보내시기 바랍니당 😄

[sihyung92]

안녕하세요 마코~~
리뷰 반영 잘 해주셔서 머지 하려다가.. 코멘트를 보니 아직 하고 싶으신게 남으신 것 같아 한번 더 request change를 보냅니다 ㅎㅎ
코드 리뷰보단 기존 코멘트에 질문 주신 거에 답변을 다는 형식이 되었네요. 확인해보시고 또 요청 주세요~~

[sihyung92]

Patch인걸 이제 봤네요 ㅎㅎ 저는 station이란 자원이 추가되는 거라 POST라고 느껴지긴 합니다. Patch로 두신건 Line의 상태를 변화하는 거라고 보셨기 때문일까요?

[aak2075]

제가 구현한 바로는 라인 등록 | 역 등록 | 라인에 역을 등록하는 section 생성 이렇게 세 기능을 독립적으로 구현했습니다.
section을 삭제하거나 추가하더라도 db상에서 line과 station에 변경사항은 없죠.
하지만 사용자 입장에서는 section이라는 객체의 존재를 모르고 station과 line으로 요청을 보내는 것이 자연스럽게 사용할 수 있다고 생각했습니다.
그래서 컨트롤러의 이름도 LineStationController 로 정했습니다.
따라서 이미 등록되어 있는 역을 라인에 등록(사용자는 상태 변경으로 인식하도록)만 한다는 의미에서 Patch를 사용했습니다.

[sihyung92]

넵 Section의 개념이 구현 세부사항이라면 굳이 외부에 알릴 필요가 없다는 부분에 공감합니다 ㅎ

이미 등록되어 있는 역을 라인에 엮어놓음 이라는 관점에서 Patch라고 보는 것도 일리가 있네요. Patch로 두셔도 될 것 같습니다.
다만 "라인에 시작역을 등록" (마지막 init 이라는 자원에 주목하여, URL 규칙상 마지막에 오는 path가 자원이라고 인식되는 경향이 있습니다)이라는 관점에서 POST라는 주장도 가능할 것 같은데요,
사실 http 규칙은 아주 엄격하게 지켜지고 있지는 않는 경우가 많아서 이런 부분은 협업시에 토의하여 조율해 주시면 되겠습니다.

[sihyung92]

이 분기는 서비스 내부에서 했으면 어떨까 싶어요. service에서 addUp이나, addDown이냐 빼고는 공통 로직을 쓰고있네요. service를 이용하는 모든 클라이언트가 해당 분기를 해줘야한다는 단점도 있고요.

[aak2075]

서비스에서 중복 코드를 줄일 수 있게되네요.
만약 원래 구현했던 대로 사용한다면 ~/endpoint 가 아닌 ~/up ~/down으로 아예 api를 분리해서 사용하는게 나았을 것 같습니다.

[aak2075]

안녕하세요 웨지~
웨지의 생각을 듣고 고민이었던 부분도 많이 해결 되고 추가로 고려해야 할 것들을 많이 알게 되었습니다.

피드백 이외에 추가로 커스텀 예외와 테스트 데이터 셋팅을 적용했습니다.
테스트 데이터는 간단한 역 등록, 라인 등록은 dummy.sql로 셋팅하였습니다.
Section을 추가하는 것은 db에 직접 값을 넣기가 불편해서 직접 request를 보내도록 했고, request를 보내는 것이 중복이 많아서 static 메서드로 따로 분리했습니다.

[sihyung92]

안녕하세요 마코~~
후딱 반영해주셨네요
커스텀 예외에 대한 리뷰와 Http Method에 대한 의견을 하나 적었으니 확인해주셔용~! ㅎㅎ
리뷰와 별개로 미션 요구사항은 만족한 것 같아 이제 머지하겠습니당
2단계 마지막으로 남은 협업미션도 빠이팅!!

[sihyung92]

비슷한 성격의 커스텀 예외라면 상속을 활용해 두어 처리하는 방법도 있습니다.
LineIllegalStateExcetpion을 만들고(이름 막 지었어용 ㅎㅎ), 핸들러에선 해당 예외를 처리하도록 만듭니다.

IllegalDistanceException, IllegalStationException, SectionStateException는 LineIllegalStateExcetpion 예외를 상속합니다.

공통 처리가 필요한 (400 or 500 등) 부모 예외를 만들어주면 됩니다.
그러면 커스텀 예외가 추가될 때마다 핸들러에 추가해주는 작업이 덜어집니다.