-
Notifications
You must be signed in to change notification settings - Fork 305
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리뷰 요청드립니다. #501
base: sang-eun
Are you sure you want to change the base?
Step3리뷰 요청드립니다. #501
Conversation
# Conflicts: # src/test/java/nextstep/subway/acceptance/AcceptanceTest.java # src/test/java/nextstep/subway/acceptance/StationAcceptanceTest.java
시작, 종점 역 마킹 제거
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 상은님!
Step 2 PR 주신 것과 현재 Step3 PR 주신게 충돌이 나서 해당 부분 정리만 해주시고 다시 리뷰 요청해주시면 감사하겠습니다!
Step 2 수행해 주시고 리뷰 요청을 해주시지 않아서 제가 확인을 못 드린 것 같네요 ㅜㅜ 🙇
더불어 Step3 에서 추가해주신 코드에 대한 코멘트를 남겼으니 확인해 주시고 리뷰 요청 해주시면 감사하겠습니다 😊
src/main/java/nextstep/subway/applicaion/section/SectionService.java
Outdated
Show resolved
Hide resolved
|
||
@Service | ||
@Transactional(readOnly = true) | ||
public class SectionService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 Section 은 Line 에 속해 있는 도메인으로 보이는데요
SectionService 를 선언하지 않아도 해당 로직을 Sections 와 Line 에서 할 수 있을 것으로 보이는데 어떻게 생각하시나요? 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클린아키텍쳐에서처럼 서비스가 비대해지는 것을 방지하기 위해 한 컨트롤러마다 GetSectionUseCase 와 같이 나누는 것까지는 아니더라도 최소한 중심이 되는 도메인(Section)별로 나누는 것이 좋다고 생각했습니다
응집시켜두는 것이 좋다고 생각하시는 이유가 있으실까요?
현재 step2가 머지되지 않아 step2가 머지되면 좀더 깔끔하게 Pr 정리하겠습니다.
늦었지만 확인해주시면 감사하겠습니다 🙏