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

[김대겸] Step2 PR #765

Merged
merged 13 commits into from Nov 27, 2022
Merged

[김대겸] Step2 PR #765

merged 13 commits into from Nov 27, 2022

Conversation

Gyeom
Copy link

@Gyeom Gyeom commented Nov 27, 2022

안녕하세요 리뷰어님,

2단계 - 경로 조회 기능 PR 입니다.

이번 리뷰도 잘 부탁드립니다 🙏 🙏 🙏

Copy link

@wooobo wooobo left a comment

Choose a reason for hiding this comment

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

2단계 잘 진행해주셨습니다 👍

별다르게 드릴 코멘트가 없네요 😄
몇가지 코멘트 남겨드렸는데 다음단계시 참고해주시면 좋을것 같아요!

다음단계도 기대하겠습니다!

public Distance(final int value) {
this.value = value;
protected Distance() {
this(0);
Copy link

Choose a reason for hiding this comment

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

매직넘버 상수화 해보면 좋을것 같아요!

private final int ZERO 이런식으로요!

Copy link
Author

Choose a reason for hiding this comment

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

넵 매직넘버 사용하면 더 좋겠네요 ㅎㅎ 감사합니다~!

Optional<Section> originalSection = sections.findSameDownStation(section);
originalSection.ifPresent(it -> it.updateDownStation(section));
Section originalSection = sections.findSameDownStation(section);
if (Objects.nonNull(originalSection)) {
Copy link

Choose a reason for hiding this comment

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

- indent(들여쓰기) depth를 2단계에서 1단계로 줄여라.
  - depth의 경우 if문을 사용하는 경우 1단계의 depth가 증가한다. if문 안에 while문을 사용한다면 depth가 2단계가 된다.

프로그래밍 요구사항에 맞지 않네요 ㅜ

Copy link
Author

Choose a reason for hiding this comment

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

indent 가 2단계로 되어 있었군요 😢 😢 수정하도록 하겠습니다.


assertThatThrownBy(() -> pathFinder.getShortestPath(강남역, 강남역))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("출발역과 도착역이 " + 강남역.getName() + "으로 동일합니다.");
Copy link

Choose a reason for hiding this comment

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

예외상황 테스트 잘 진행해주셨네요 👍


import static org.assertj.core.api.Assertions.assertThat;

public class LineSectionAcceptanceStep {
Copy link

Choose a reason for hiding this comment

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

잘 분리해주셨네요 👍

@wooobo wooobo merged commit bb35344 into next-step:gyeom Nov 27, 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.

None yet

2 participants