[Review] ATDD 2주차 4차 PR

권승철 리뷰어 님께 감사의 말을 전하고 싶다!!

 

1. 질문

1-1) Exception과 Domain

#323 (comment)
지난 번 리뷰에 대한 저의 생각입니다 ㅎㅎ 의견 달아주시면 감사하겠습니다!

 

예외를 나타내는 클래스가 http status를 상태로 갖는 게 괜찮을까요?
=> 사실상 Spring에서 벗어날 일이 없을 거라 거의 100% 확신하기 때문에 Spring에 의존적인 부분들을 가져다 사용해도 무방하다 생각합니다!

 

비즈니스 로직에 대한 예외처리인데, http 라는 프로토콜에 의존하게 된다고 생각합니다.
가장 중요한 도메인에 관련한 클래스들은 외부 기술에 종속되지 않도록 구현하는 것을 추천드립니다
=> Entity Class 즉, Domain안에서는 HttpStatus에 대한 import가 없으니 의존성이 없다 할 수 있지 않나요?

 

    public void deleteSection(Station station) {
        if (isSizeLessThanOne()) {
            throw SectionsDeleteException.CANT_DELETE_LAST_ONE_SECTION_EXCEPTION();
        }

        // 생략...
    }

위와 같이 사용하기 때문에, 위 코드가 있는 Entity 상에서는 HttpStatus에 대한 의존(import)문이 없습니다.

물론 CANT_DELETE_LAST_ONE_SECTION_EXCEPTION() 메서드 내부에서는 HttpStatus에 대한 의존이 있지만, 이 부분은 사실 도메인이 아니지 않을까요?

 

답변:

추이적으로의 의존성이라.... 흠 리뷰어가 하시는 말씀은 이해는 간다만?

너무 추상적인 부분 아닐까? 진짜 import로 인식되지 않는 의존성 까지 다 생각하면서 코드를 작성해야 할까?

약간은 아직도 물음표 상태인것 같다.

 

다만 예외 또한 비지니스 로직의 일부, 즉 도메인에서 예외를 던지니

그 예외 Class에서도 최대한 외부의 의존을 줄여야 한다 생각하시는 것 같다.

 

이를 수용하여 모두 세부적인 예외로 만들도록 처리하였다.

 

2. 리뷰 정리

2-1) 메서드의 동일한 추상화 Level

나도 동의하는 내용이다. 사실 나도 deleteMiddleStation 과 동일한 추상화 수준을 갖도록 하고싶싶었다.

하지만 생각보다 힘든 부분이 많다.

메서드로 deleteFirstStation, deleteLastStation을 만들어 추상화 수준을 맞추면, 조건문 없이는
deleteFirstStation, deleteLastStation, deleteMiddleStation 모두를 다 수행해야만 함수가 끝나게 돼버립니다.

 

deleteFirstStation를 수행했으면 거기서 끝나야 하는데, 나머지 메서드까지 실행해버리니... 불필요한 행동을 수행하게 된다 생각합니다...

 

이에 대한 해결은 명확하게는 못할 것 같다.

 

답변:

 

2-2) 경로 검색 전략의 변경

 

이런 경우는 전략 패턴이 적합할 것 같습니다. 외부에서 알고리즘 객체를 주입받도록 변경하면 될 것 같습니다 ㅎㅎ
(ps 헤드퍼스트 책의 오리와, 고무 오리 예제가 떠오릅니다 ㅎㅎ, 전략 패턴을 적용해보기 좋은 부분인 것 같습니다!!)

 

https://blogshine.tistory.com/5

[Design Patterns] Strategy Pattern : 스트래티지 패턴

 

2-3) 예외 처리에 대한 장단점 고민

 

▶ Exception에 세부 정도가 강한 경우 (이번 구현)
-> 장점: 예외 컨트롤을 Handler에서 세부적으로 할 수 있어 좋다. 예외 처리 자체를 아예 Handler에게 위임시키는 것이다.
-> 단점: 예외마다 만들려니까 너무 중복되는 느낌이 있다.

 

▶ Exception에 세부적 정도가 중간인 경우(직전 구현)
-> 장점: 중간 정도의 세분화된 Exception, 예를 들어 StationAddException은 Station을 Add 하는 과정에서 발생하는 모든 예외를 가지고 있기 때문에 관리에 편하다.
-> 단점: Handler에서 세부적으로 처리하기 힘들다. 애당초 Exception내부에서 HttpStatus를 지정해줘야 한다. Handler에서 StationAddException 내부의 예외들을 구별할 수 없기 때문이다.

 

▶ Exception에 세부적 정도가 약한 경우(지지난 구현)
-> 장점: 프로젝트가 작은 경우 관리하기는 매우 편함
-> 단점: 지금 생각해보니 위 2가지 케이스들의 장점이 모두 단점으로 되는 것 같다. 가장 별로인 방식인 것 같다.

 

답변:

 

2-4) Graph의 결과 객체 반환

기존에는 최단거리를 구한 결과(경로, 거리)의 값을 각각 받게 되었다.

이를 PathRsult를 만들어 반환하도록 처리하였다. 이는 더욱 객체지향적으로 변경된 방식이라 생각된다.

 

우선 변경 전의 코드는 다음과 같다.

public class Graph {

    private WeightedMultigraph<Station, DefaultWeightedEdge> graph;
    private DijkstraShortestPath dijkstraShortestPath;

    public Graph(List<Line> lines) {
        if (lines.isEmpty()) {
            throw new EmptyLineException();
        }

        this.graph = new WeightedMultigraph(DefaultWeightedEdge.class);
        this.dijkstraShortestPath = new DijkstraShortestPath(graph);
        initGraph(lines);
    }

    public List<Station> getShortestPath(Station startStation, Station arrivalStation) {
        GraphPath path = createGraphPath(startStation, arrivalStation);
        return path.getVertexList();
    }

    public int getShortestDistance(Station startStation, Station arrivalStation) {
        GraphPath path = createGraphPath(startStation, arrivalStation);
        return (int) path.getWeight();
    }
    
    // 생략...
}

이를 다음과 같이 변경하였다.

 

우선 다음과 같이 PathResult를 만들었다.

public class PathResult {

    private List<Station> path;
    private int distance;

    public PathResult(List<Station> path, int distance) {
        this.path = path;
        this.distance = distance;
    }

    public List<Station> getPath() {
        return path;
    }

    public int getDistance() {
        return distance;
    }
}

 

이후 코드를 다음과 같이 변경하였다.

public class Graph {

    private WeightedMultigraph<Station, DefaultWeightedEdge> graph;
    private DijkstraShortestPath dijkstraShortestPath;

    public Graph(List<Line> lines) {
        if (lines.isEmpty()) {
            throw new EmptyLineException();
        }

        this.graph = new WeightedMultigraph(DefaultWeightedEdge.class);
        this.dijkstraShortestPath = new DijkstraShortestPath(graph);
        initGraph(lines);
    }

    public PathResult getShortestPathResult(Station startStation, Station arrivalStation) {
        GraphPath path = createGraphPath(startStation, arrivalStation);
        return new PathResult(path.getVertexList(), (int) path.getWeight());
    }
    
    // 생략...
}

getShortestPathResult 메서드를 만들어 결과 객체인 PathResult를 반환하도록 변경하였다.

 

2-5) 테스트 코드는 일종의 명세서이다

 

"테스트 코드는 프로덕션 코드의 명세"이다. 이는 어쩌면 ATDD의 핵심 부분에 해당된다.

사실 리뷰어가 언급해주신 것처럼, path 구하는 메서드랑 너무 동일해서 이걸 꼭 만들어줘야 할까? 란 생각이 들어 만들지 않았는데,
추후 다른 사람이 저의 코드를 볼 경우도 생각하면 테스트를 작성해주는 것이 적합하다 생각하다.

 

따라서 distance를 구하는 테스트 또한 작성하게 되었다.