인수 테스트와 TDD 회고

Step1 구간 추가 요구사항 반영

Step1 구간 추가 요구사항 반영 PR

 

2주차부터 본격 적인 미션의 시작이였다. 난이도가 올랐기 때문이다.

 

Line line = findBy(lineId);
Sections sections = findBy(line);

지하철역의 리스트를 검증하는 부분에서 

containsOnly를 사용했다.

 

아래와 같은 피드백을 받았다.

💡 같은 findBy인데 lineId를 넣으면 line이, line을 넣으면 sections가 나오는게 어색하게 느껴져요!

 

받는 변수타입으로 구분할 수 있지만 메소드명으로  구분해주는게 코드 가독성에 좋을 것 같아 아래와 같이 수정하였다.

 

Line line = findLineBy(lineId);
Sections sections = findSectionsBy(line);

 

 

지하철 구간을 등록시 이미 존재하는 구간이면 등록할 수 없는 요구사항이 있어 관련 테스트 코드를 작성하였다.

   @Test
    void 실패_구간_등록시_신규_구간과_기존_구간이_일치할_경우_구간을_등록할_수_없다() {
        assertThatThrownBy(() -> 구간.validateRegisterStationBy(강남역, 선릉역))
                .isInstanceOf(ApplicationException.class)
                .hasMessage("신규 구간이 기존 구간과 일치하여 구간을 생성할 수 없습니다.");
    }

 

 

아래와 같은 피드백을 받았다.

💡 강남역 - 선릉역 구간이 있을 때, 선릉역 - 강남역을 등록하려고 해도 예외가 발생해야하는 것 아닐까요? (두 역이 모두 기존에 등록되어있을 때)

 

순서를 고려한 엣지 케이스를 놓쳐 테스트 메소드를 하나 더 만들고 순서만 바꿔서 테스트를 작성하였다.

 

 

노선에 구간을 추가시 처음과 끝에도 추가할 수 있고, 가운데에도 추가할 수 있다. 가운데에 추가하는 케이스를 수행하는 메서드의 네이밍을 아래와 같이 생각했다.

 //Sections 클래스
     public void addSectionInMiddle(Station upStation, Station downStation, long distance) {
        if (!isTerminalAddStation(upStation, downStation)) {
            updateExistingSection(upStation, downStation, distance);
        }
    }

 

아래와 같은 피드백을 받았다.

💡 구간의 처음/끝에 생성되어 노선의 길이를 늘릴 수도 있고, 구간의 중간에 삽입될 수 있는데 일괄로 InMiddle로 네이밍해주신 이유가 궁금합니당~

 

당시에 답변을 아래와 같이 하였다. (참고로 두달전에 작성한 글인데 읽자마자 무슨소리야? 란 생각이 들었다.. ㅋㅋㅋ ㅠ)

💡 경우의 수를 분석하다 보니 구간의 처음과 끝에 생성이 되든 구간의 중간에 삽입이 되는 요청받은 값 그대로 save를 해야 했습니다!
추가적으로 구간의 중간에 삽입이 되는 경우는 기존 구간의 값을 변경해야 했기 때문에 해당 로직만을 구현하는 메소드다 보니 InMiddle로 네이밍을 했습니다!!

 

좀더 가독성 좋은 메소드명이 있을까요 ㅎ..

 

요구사항을 설명 하자면..

 

구간의 처음/중간/끝에 삽입시 공통적으로 구간을 저장 해야 했다.

이때, 중간에 삽입되는 경우 기존 구간의 값을 변경해야 했다. 

 

 

https://edu.nextstep.camp/c/R89PYi5H

현재 라인의 총 구간이 A -- C 일때

A역과 C역 사이에 B역을 추가하여 A -- B 구간과 B -- C 구간을 만들려고 한다.

구간 추가시 요청값은 라인, 상행역, 하행역이다. 상행역이든 하행역이든 둘중 하나는 기존 노선에 존재하는 역을 보내줘야 한다. 기준점이 필요하기 때문이다.

 

구간 정보는 아래와 같이 설계 하였다. 

// 구간
public class Section {
    private Long id;
    private Line line;
    private Station upStation;
    private Station downStation;
    private Long distance;

 

즉, A -- C 로 구간 정보 한개가 저장 되어 있는걸

A -- B

B -- C 

총 2개의 구간 정보로 저장을 해야 했다.

요청값이 A -- B로 들어오면 A -- B는 그대로 저장하고, C역을 상행역을 기존 A가 아닌 B역으로 변경해야 했다.

 

서비스 로직은 아래와 같이 작성했다.

// SectionService.java
sections.addSectionInMiddle(upStation, downStation, request.getDistance());
Section section = sectionRepository.save(
    new Section(line, upStation, downStation, request.getDistance())
);

// Sections.java
public void addSectionInMiddle(Station upStation, Station downStation, long distance) {
    if (!isTerminalAddStation(upStation, downStation)) {
        updateExistingSection(upStation, downStation, distance);
    }
}

 

중간역에 추가할 경우 기존 구간의 정보를 변경하는 로직이 addSectionInMiddle에서 수행된다.

 

지금와서 돌이켜보면 아래와 같이 코드를 작성한게 의도가 좀더 명확하게 나타났을 것 같다.

// SectionService.java
Section section = addSection(request, line, upStation, downStation);
if(!sections.isTerminalAddStation){
    updateExistingSection(upStation, downStation, distance);
}

 

예외처리시 http status는 200으로 넘겨주었다.

    @Test
    void 실패_지하철_구간_제거시_구간이_한개만_있는_경우_구간을_제거할_수_없다() {
        // when
        String message = 구간_제거_요청(OK.value(), Map.of("stationId", "1"))

 

아래는 의견을 나눈 전문이다.

💡
리뷰어님: 여기서 OK를 넘겨주시는 이유가 궁금해요!

나: 예외처리 같은 경우 GlobalExceptionHandler를 타도록 구현했습니다.
예외 처리 같은 경우 200 http status를 반환하도록 구현해서 OK를 넘겨줍니다!

리뷰어님: 오홍 왜 200 인가요?!

나: 평소 예외처리 할때는 약속한 json 형식으로 예외별로 지정된 code값을 반환하도록 구현 합니다.
그래서 http status는 200으로 넘겨줘도 괜찮다는 생각을 했습니다!!
민정님은 예외처리시 http status를 적극적으로 활용하시나요? 어떤식으로 사용하시는지 궁금합니다!!

리뷰어님: 오호! 저는 http status도 항상 그에 맞게 내려주는 편이에요!
예를 들어 404 not found라고 친다면 http status도 404, body에는
아래는 예시인데 message, code를 담을 때도 있는 것 같아요!

{
  "message": "station id with 1 is not found"
}

이렇게 하는 이유는 datadog이나 alert를 받을 때 200OK로 떨어지는 경우에는 실제 어떤 경우에서 예외가 발생하는지 확인하려면 매번 response를 까봐야하는 불편함도 있고, api 응답 자체에 대한 status code가 가장 최상위로 그 응답의 상태를 나타내준다고 생각하기 때문이에요!

 

datalog 단어를 보자마자 머리가 띵했다. 현재 회사에선 모니터링 시스템이 전혀 구축되어 있지 않다. ssh로 서버 접속해서 tail -f로 로그를 확인한다. 정말 레거시 그 잡채 아닌가? 아무튼.. 그래서 리뷰어님과 같은 생각을 하기가 쉽지 않았다.

어쨌든, 리뷰어님의 의견에 적극 공감하는 바이다. 앞으로는 http status를 적극 활용해야겠다.

 

 

Step2 구간 제거 요구사항 반영

Step2 구간 제거 요구사항 반영 PR

 

해당 미션은 설날에 본가에 내려가서 카페에서 즐거운 코딩을 했다!! 히히

 

구간 정보를 일급 객체로 보관하는 Sections의 기능이 많다고 느껴졌다. public 메소드가 8개고 private 메소드가 15개 정도 되었다. 클래스 분리의 신호인지 리뷰어님께 여쭤봤다.

💡 고민이 되네용ㅎㅎ 오히려 Sections 안에 있어서 응집도가 높아 보이기도 하고, 말씀주신대로 점점 기능이 많아지다보니 라인 수도 길어지기도 하네요.
분리해본다면 어떤 식으로 해볼 수 있을까요?
제가 떠오르는 방식은 아예 Terminal만 관리하는 객체와 Sections를 관리하는 것을 분리해보아도 좋을 것 같다는 생각이 들긴 하는데, 오히려 더 복잡해질 수도 있을 것 같아요 🤔
우선 저는 지금도 괜찮다는 생각이긴 합니당! 우철님 생각도 궁금해요~!

 

괜찮다고 하셔서 우선 안심이 되었다!! 응집도의 관점을 생각치 못했다니.. (충격)

구간의 CRUD 기준으로 SectionsAdd, SectionsDelete를 만들면 어떨까 하다가 괜스레 복잡해질 것 같아서 하지 않았다.

 

정책상 예외처리를 해야하는 케이스일 경우 http status를 어떻게 내려줘야 하는지 여쭤 봤다.

        // then
        assertThat(message).isEqualTo("신규 구간이 기존 구간과 일치하여 구간을 생성할 수 없습니다.");

 

💡 
나: 예외처리 관련해서 step1에 이어서 질문드립니다!!
http status를 적극적으로 활용한다고 하셨는데요.
위와 같은 정책상으로 예외 처리되는 케이스인 경우에는 어떤 status 값으로 던져주시나요?

리뷰어님: 저는 400 BadRequest로 내려요!
정책 상 예외 케이스라고 하는 것은 정책에 위반된 값이 들어왔다고 생각하기 때문입니다! 우철님이라면 어떤 코드를 내리실 것 같으신가요?

나: 400은 필수값 혹은 형식 체크(이메일 등)로만 처리하고 정책상 예외케이스는 409 Conflict 상태로 내릴 것 같습니다!! 이유는 요청값 자체로만 봤을 때는 문제가 없지만, 현재 가지고 있는 리소스(데이터)의 상태와 충돌이 발생한다는 의미가 개인적으로는 좀더 와 닿는것 같아서욤..!
다시한번 생각할 수 있게 역으로 질문해주셔서 감사합니다 ㅎㅎ 🤗

 

속한 팀에서 규칙을 잘 조율하면 400 혹은 409든 문제는 되지 않을 것 같다. 지금은 409보단 400이 더 끌리긴 한다!!

 

요청값으로 들어온 상행역, 하행역과 기존 노선의 상행역, 하행역을 기반으로 비교하는 로직이 있었다.

     return (section.isUpStation(upStation) && section.isDownStation(downStation)) ||
                (section.isUpStation(downStation) && section.isDownStation(upStation));

 

 

아래와 같은 피드백을 받았다.

💡 한번 더 숨겨보는 건 어때요?

 private boolean isSameSection(Station upStation, Station downStation, Section section) {
    return section.isSame(upStation, downStation);
}

 

기존 로직과 같이 논리 연산자가 장황하게 되어 있으면 코드 해석하기가 쉽지않다. 메소드명으로 의미를 나타낼 수 있으면 최대한 메소드로 분리하는게 좋다!! 

 

// AS-IS
public Section findMatchingSection(Station upStation) {

// TO-BE
public Section findMatchingSection(Station station) {

 

메소드 인자에 반드시 상행역만 온다는 보장이 있을까? 좀더 범용적으로 사용하는것임을 나타내기 위해 station이라는 이름으로 변경하였다.

 

 

Service에서 Repository를 참조한다.

Repository에서 반환하는 Optional 핸들링을 서비스에서 하고 있었다.

// SectionService.java
Station deleteTargetStation = stationRepository.findById(stationId)
                .orElseThrow(() -> new ApplicationContextException("제거할 역이 존재하지 않습니다."));

아래와 같은 피드백을 받았다.

💡 p5; 이런 부분, 뭔가 stationQuery.findById(stationId) 와 같은 것을 만들어서 그 내부에서 throw를 하도록 숨겨보는 건 어때요?

 

※ p5 는 뱅크 샐러드 코드리뷰의 pn 룰 코드 리뷰 in 뱅크샐러드 개발 문화 이다.

 

케이타운 포유에 재직중이신 이중석님의 유튜브를 보다가 괜찮다고 생각한 기법이 있어 적용해보았다.

(영상 너무 좋습니다. 주니어 개발자 분들에게 강추합니다!!)

 

public interface StationRepository extends JpaRepository<Station, Long> {
    List<Station> findByIdIn(List<Long> ids);
    default Station getBy(Long id) {
        return findById(id)
                .orElseThrow(() -> new ApplicationException(
                        "지하철역이 존재하지 않습니다. id=" + id));
    }

}

 

처음 봤을때 'default 메소드를 저런식으로 활용할 수도 있구나' 라는 적잖은 충격을 받았다. 

개인적으론 좋은 방법이라고 생각하지만 개인마다 견해가 다른것 같다.

💡 
찬성(나의 의견): 오히려 서비스에 혼재되어 있는 예외처리 로직을 중앙집중화 할 수 있어 일관성을 유지시켜 줍니다. 도메인 내부에서 상태값 조건에 따라 예외처리를 던지듯 예외처리는 혼재될 수도 있다고 생각합니다. 예외처리를 반드시 Service에서만 처리하는 것은 코드의 복잡성을  증가시킬 수 있습니다.

반대: Repository에서 예외처리를 하는게 맞을까? 비즈니스 로직상의 에러 처리는 Service에서 명시적으로 해야 흐름을 파악하기 더 쉽지 않을까요? 다른 로직상의 예외처리는 Service에서 불가피하게 할 수 밖에 없는 경우도 있는데 예외 처리가 혼재되어 있어 헷갈릴 수 있을 것 같아요.

 

여러분의 선택은 ?! 두둥!!

 

단위 테스트와 인수테스트를 작성해서 서비스  테스트는 크게 의미가 없을 것 같아서 진행하지 않았다.

(물론 있으면 좋다)

 

💡
리뷰어님: SectionServiceTest도 있으면 좋을 것 같단 생각이 들어요!
전체적인 로직 점검 및 distance 등도 검증할 수 있을 것 같아요

나: 현재 코드에서 인수 테스트와 단위 테스트로는 충분하지 않은 걸까요?

저의 짧은 식견으로는.. 서비스 레이어까지 모든 케이스를 고려해서 테스트를 작성해야 하는데 그만큼 리소스가 많이 들것 같아서요 😂
미션을 진행하며서 Service 레이어에서 비즈니스 로직을 최대한 제외 시켰고 인수 테스트의 검증 범위에 미치지 않는 부분을 단위 테스트를 작성함으로서, 충분히 검증이 되었다는 생각이 들었습니다!!

민정님은 실무에서 테스트 작성시 테스트 대상을 선정하는 기준이나 혹은 어떤 레이어 위주로 작성하시는지 궁금합니다!!

리뷰어님: 저는 왜 멘션되어도 깃허브 앱에서 알림이 안올까요ㅠㅠ

제 생각엔.......ㅎㅎㅎㅎ 우철님이 말씀해주신 것처럼 현재는 인수테스트와 단위테스트로도 충분하다고 생각해요! 특히 테스트케이스를 지금도 잘 작성해주고 계시기 때문에요ㅎㅎ 그런데 제안드려본 이유는, 저는 미션은 연습이라고 생각하기 때문이에요. 실무에서는 우리가 리소스나 일정 상 할 수 없게 되는 것들도 있어서, 미션을 통해서 다양한 케이스를 연습해보고 실무에서 가장 적합한 솔루션을 적용해볼 수 있을 거라 기대해요!
미션도 마찬가지로 어쨌든 요구사항의 만족이 우선이기 때문에 부담이 되신다면 지금도 충분하니 서비스 테스트는 제외해주셔도 괜찮아요~!

저는 실무에서.. 유즈케이스와 관련된 핵심 비즈니스 로직만 mockk으로 간간히 테스트합니다....ㅋㅋㅋㅋㅋ

 

사실 부담이 되었던건 사실이다 ㅎ.. 인수테스트 만으로도 조금 벅찼는데 서비스 테스트 까지 하면 미션을 진행하는데 영향이 미칠 것 같았다 ㅠㅠ

리뷰어님은 테스트를 꼼꼼히 작성하실 줄 알았는데 그게 아니였다!! (충격)(공포)(소름)

농담이고 실무 경험이 있으신분들이면 다들 공감하실 것이다. 온전히 테스트 코드를 작성할 시간이 부족하다는 사실을..!!

 

 

Step3 경로 조회 기능

Step3 경로 조회 기능

 

경로 조회 기능을 구현하기 위해 다익스트라 알고리즘을 적용하였다.

향후 다양한 솔루션을 적용할 수 있도록 인터페이스화 하였다.

 

구현하는데 있어 한가지 고민 사항이 있었다. 전략 패턴을 구현할때 아래와 같이 구현 하였다.

public interface ShortestPathStrategy {
    List<Station> findShortestPath(Station source, Station target);
    int findShortestDistance(Station source, Station target);
    boolean support();
}

public enum ShortestPathType {
    DIJKSTRA, FLOYD
}

public class Dijkstra implements ShortestPathStrategy {
    private final DijkstraShortestPath dijkstraShortestPath;
    
    public Dijkstra(List<Section> sections) {
        //..
    }
    
    boolean support(ShortestPathType type) {
      return ShortestPathType.DIJKSTRA == type
    }

@Component
public class ShortestPathFactory {

    private List<ShortestPathStrategy> shortestPathStrategies;

    public ShortestPathStrategy generateStrategy(ShortestPathType shortestPathType, List<Section> sections) {
        ShortestPathStrategy strategy = shortestPathStrategies.stream()
                .filter(shortestPath -> shortestPath.support(shortestPathType))
                .findFirst()
                .orElseThrow(() -> new ApplicationException("Unsupported shortest path type: " + shortestPathType));
        return strategy;
    }
}

 

요청값으로 들어온 타입을 support 메서드에 넘겨 구현체를 구할 수 있는 로직이다.

 

고민 포인트는 구현체인 Dijkstra에 생성자 주입으로 List<Sections>를 넣어주려고 했다.

하지만 ShortestPathFactory빈이 생성될때  private List<ShortestPathStrategy> shortestPathStrategies; 요 필드 에는구현체들이 생성되어 있다.

따라서 구현체 생성 시점에 동적으로 sections값을 세팅해 줄수 없었다.

 

그래서 아래와 같이 직접 new 키워드를 사용해서 생성을 하였다. 

public class Dijkstra implements ShortestPathStrategy {
    private final DijkstraShortestPath dijkstraShortestPath;
    
    public Dijkstra(List<Section> sections) {
        //..
    }
    

public class ShortestPathFactory {
    public ShortestPathStrategy generateStrategy(ShortestPathType shortestPathType, List<Section> sections) {
        if (Objects.requireNonNull(shortestPathType) == ShortestPathType.DIJKSTRA) {
            return new Dijkstra(sections);
        }
        throw new ApplicationException("존재하지 않는 알고리즘 최단 전략 입니다.");
    }
}

 

다른 방법이 없나 리뷰어 님께 여쭤보았다.

 

💡 
sections는 변경 가능성이 있어서 경로 조회 요청이 올 때마다 넣는게 맞는 것 같다는 생각이 들어서요.

결국에 sections를 넣는 형태로 가려면 Configuration을 통해서 bean으로 등록해주던 ApplicationContext를 활용해서 빈으로 등록해주던 간에 sections를 전달해야해서 오히려 복잡해질 것 같아요.

Dijkstra를 동적으로 가져가고 싶다면 차라리 sections 없이 그냥 빈 Dijkstra를 생성하고 조회 요청이 왔을 때 dijkstra 타입인 경우 전달받은 sections를 넘겨서 graph를 초기화하는 수밖에 없을 것 같아요.! 🤔

 

왜 인터페이스의 메서드의 인자로 추가할 생각은 못했을꼬!!!!!!

코드에 적용은 하진 않았지만 궁금즘이 말끔히 해소되어서 기분이 좋았다.

 

2주차 미션도 끝!!