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

[1단계 - 지하철 정보 관리 기능] 제이(이재윤) 미션 제출합니다. #41

Merged
merged 86 commits into from May 14, 2023

Conversation

sosow0212
Copy link

에단 안녕하세요? 리뷰이 제이입니다~

이번 미션은 굉장히 복잡했던 것 같습니다.

페어와 함께 Entity, Domain에 대해서 고민을 많이 했습니다.
그리고 복잡한만큼 Domain을 통해서 비즈니스 로직을 수행하고 Entity에 값을 복사해서 DB에 저장하는 식으로 미션을 진행했습니다.

이런 과정에서 Service가 굉장히 복잡해졌습니다.
Service에서 도메인을 통해 비즈니스 로직을 수행시키기도 하고, Entity -> Domain / Domain -> Entity의 역할도 해서 이런 문제가 발생했던 것 같습니다.

그래서 복잡도를 줄이기 위해 Service <-> DAO 사이에 Repository를 두어서 Entity와 Domain의 매핑의 역할을 위임해주었습니다.
이에 대해 에단은 어떻게 생각하시나요?

또한 미션 제출 시간이 부족해서 당장은 E2E 테스트만 진행했습니다.
추가적인 테스트들은 리뷰를 받으면서 작성하도록 하겠습니다!

감사합니다 :)

sosow0212 and others added 30 commits May 9, 2023 14:51
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Co-authored-by: kokodak <kokodakadokok@gmail.com>
Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 제이.

피드백 빠르게 반영해주셨네요.

추가로 코멘트 남겨드렸습니다. 확인 부탁드려요.

import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
public class StationServiceMockTest {

Choose a reason for hiding this comment

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

단위테스트라면 다른 단위 테스트들처럼 XXXTest 형태로 네이밍하면 어떨까요?

src/main/java/subway/domain/SectionCase.java Outdated Show resolved Hide resolved
src/main/java/subway/controller/LineController.java Outdated Show resolved Hide resolved
src/main/java/subway/domain/common/Distance.java Outdated Show resolved Hide resolved
src/main/java/subway/dao/line/LineDao.java Show resolved Hide resolved
src/main/java/subway/repository/StationRepository.java Outdated Show resolved Hide resolved
Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 제이.

피드백 잘 반영해주셨어요.

이번 단계는 이만 머지하도록 할게요.

남겨드린 코멘트는 다음 단계 진행하면서 참고해주세요.

import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
public class StationServiceMockTest {

Choose a reason for hiding this comment

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

XXTestXXUnitTest의 차이는 무엇일까요?

Comment on lines +45 to +54
while (!queue.isEmpty()) {
Station station = queue.poll();
stations.add(station);
visited.put(station, true);
for (Station nextStation : lineMap.get(station)) {
if (!visited.get(nextStation)) {
queue.add(nextStation);
}
}
}

Choose a reason for hiding this comment

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

메서드의 들여쓰기를 줄여보면 어떨까요?

}

@Override
public boolean equals(final Object o) {

Choose a reason for hiding this comment

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

id 필드는 고려하지 않아도 될까요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 Section은 name으로 구분하고 있기 때문에(db에서도 name속성은 unique로 중복이 될 수 없습니다!) id 필드는 고려하지 않아도 될 것 같습니다!

}

@Transactional
public Long saveLine(final LineCreateRequest request) {

Choose a reason for hiding this comment

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

LineRepository.insertLine의 반환 타입은 long인데 이 메서드는 Long 타입을 반환하네요.

@Laterality Laterality merged commit 313da63 into woowacourse:sosow0212 May 14, 2023
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