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

step3 Section과 Sections 구현 및 테스트 완료 #826

Open
wants to merge 1 commit into
base: nocomet
Choose a base branch
from

Conversation

nocomet
Copy link

@nocomet nocomet commented Jun 26, 2022

안녕하세요 멘토님~
단계가 진행될수록 재밌는 과제인 것 같습니다!
다대다 구조를 매끄럽게 처리하는 방법도 배운 것 같아 좋은 경험이 된 것 같아요!
게다가 atdd 방식을 계속해서 학습하고 있다는게 정말 좋은 것 같습니다!

피드백 주시면 잘 검토하도록 하겠습니다. 감사합니다.

Copy link

@jaeyeonling jaeyeonling left a comment

Choose a reason for hiding this comment

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

미션을 잘 진행해주셨습니다 👍
코멘트 남겨두었으니 확인 부탁드릴게요~

@@ -29,9 +31,9 @@ public LineService(LineRepository lineRepository, StationService stationService)
public LineResponse saveLine(LineRequest lineRequest) {
Station upStation = stationService.findStationEntityById(lineRequest.getUpStationId());
Station downStation = stationService.findStationEntityById(lineRequest.getDownStationId());
Line line = lineRepository.save(new Line(lineRequest.getName(), lineRequest.getColor()));
line.initSections(new Section(upStation, downStation, lineRequest.getDistance(), line));

Choose a reason for hiding this comment

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

Line의 생성과 initSections를 분리해주신 이유가 있을까요?

}

public boolean canAddFirstSection(Section newSection) {
if (isNotValidNewSection(newSection)) {

Choose a reason for hiding this comment

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

Suggested change
if (isNotValidNewSection(newSection)) {
if (newSection.isNotValidNewSection()) {

두 방식은 어떠한 차이가 있을까요?

public void add(Section newSection) {
checkAlreadyExist(newSection);
if (addIfFirstSection(newSection)) {
return ;

Choose a reason for hiding this comment

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

Suggested change
return ;
return;

공백을 추가해주신 이유가 있을까요?

addIfIntersection(newSection);
}

private boolean addIfFirstSection(Section newSection) {

Choose a reason for hiding this comment

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

해당 메서드는 변경과 조회의 역할 두 가지 모두 하고 있는데요.
변경의 메서드와 조회의 메서드를 분리하여 유지보수성을 높여보면 어떨까요?
관련 키워드로는 CQS(Command Query Separation) 입니다 :)

StationResponse 정자역 = StationTestHelper.지하철역_생성("정자역").as(StationResponse.class);

// when
ExtractableResponse<Response> addSectionResponse = SectionTestHelper.지하철에_노선_지하철역_등록(신분당선.getId(), new SectionRequest(강남역.getId(), 정자역.getId(), 신분당선_distance + 1));

Choose a reason for hiding this comment

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

120자가 넘는다면 줄바꿈을 해주면 어떨까요?

}

public boolean isSameSection(Section newSection) {
return isInterSection()

Choose a reason for hiding this comment

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

isSameSection이라는 메서드명으로 봤을 때 isInterSection을 검증하는 것은 오해의 소지가 될 것 같은데 어떻게 생각하시나요?

@JoinColumn(name = "down_station_id", foreignKey = @ForeignKey(name = "fk_section_to_down_station"))
private Station downStation;

@Column(name = "distance")

Choose a reason for hiding this comment

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

Suggested change
@Column(name = "distance")
@Column

변수명과 동일하다면 name을 따로 지정하지 않아도 됩니다 ㅎㅎ

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

3 participants