Skip to content

Conversation

@owen-q
Copy link

@owen-q owen-q commented Jul 22, 2023

step3 진행하였습니다~!

@hiblue02
Copy link

  1. 지난 pr에서 요청드린 내용 반영 부탁드려요! 미션 2: 지하철 구간 제거 기능 개선  #595 (review)
  2. Sections의 possibleToAddSection, possibleToDeleteSection if문이 많은데요. 요구사항이 추가되면 계속 해서 if문이 추가될 것 같아요. 다른 구조로 개선해볼 수 있을 것 같아요! 😄

Copy link

@hiblue02 hiblue02 left a comment

Choose a reason for hiding this comment

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

3단계 미션도 빠르게 진행해주셨네요! 👍
코멘트 확인해주시고, 다시 리뷰 요청 주세요!

Comment on lines +85 to +89
Assertions.assertThat(shortestPath.getDistance()).isEqualTo(16);
Assertions.assertThat(shortestPath.getStations().get(0)).isEqualTo(stationD);
Assertions.assertThat(shortestPath.getStations().get(1)).isEqualTo(stationA);
Assertions.assertThat(shortestPath.getStations().get(2)).isEqualTo(stationB);
Assertions.assertThat(shortestPath.getStations().get(3)).isEqualTo(stationC);

Choose a reason for hiding this comment

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

  1. assertThat 앞에 Assertions가 붙는데요, static import를 사용해보면 어떨까요?
  2. assertThat에서 리스트 내부 객체의 순서를 검증할 땐 containsExactly를 사용해면 어떨까요?

Comment on lines +121 to +127
void 오류가발생한다() {
ExtractableResponse<Response> response = RestAssured.given().log().all()
.when().get(getPathUrl(stationC2.getId(), stationC2.getId()))
.then().log().all().extract();

Assertions.assertThat(response.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST.value());
}

Choose a reason for hiding this comment

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

오류 케이스까지 꼼꼼히 작성해주셨네요! 💯

Comment on lines +6 to +13
@Configuration
public class ShortedPathFinderConfiguration {

@Bean
public ShortestPathFinder shortestPathFinder() {
return new DijkstraShortestPathFinder();
}
}

Choose a reason for hiding this comment

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

DI로 조립해주셨군요! 👍

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