-
Notifications
You must be signed in to change notification settings - Fork 1.1k
🚀 4단계 - 로또(수동) #4161
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
base: aj172019
Are you sure you want to change the base?
🚀 4단계 - 로또(수동) #4161
Conversation
피드백 잘 부탁드립니다~!😊 |
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.
안녕하세요 준헌님 😃
마지막 단계 잘 구현해 주셨네요 👍
몇 가지 코멘트 남겨두었는데 확인해 주시고 다시 리뷰 요청해 주세요.
public IntStream intStream() { | ||
return IntStream.range(0, value); | ||
} |
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.
평소에도 스트림을 반환하는 로직을 자주 사용하시나요?
어떤 장점이 있다고 판단하셨는지 궁금합니다.
public LottoCount minus(LottoCount other) { | ||
validateMinus(other); | ||
return LottoCount.of(this.value - other.value); | ||
} |
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.
불변 객체 활용 👍
@Override | ||
public LottoTickets read() { | ||
LottoTicketsManuelInputStrategy lottoTicketsManuelInputStrategy = LottoTicketsManuelInputStrategy.ofSystemIn(lottoManuelCount); | ||
LottoTicketsManualInput lottoTicketsManualInput = LottoTicketsManualInput.create(lottoTicketsManuelInputStrategy); | ||
LottoTickets manualLottoTickets = lottoTicketsManualInput.action(); | ||
|
||
LottoCount lottoAutoCount = lottoAmount.getLottoCount().minus(lottoManuelCount); | ||
|
||
LottoTicketsAutoInputStrategy lottoTicketsAutoInputStrategy = LottoTicketsAutoInputStrategy.of(lottoAutoCount); | ||
LottoTicketsAutoInput lottoTicketsAutoInput = LottoTicketsAutoInput.create(lottoTicketsAutoInputStrategy, lottoAutoCount, lottoManuelCount); | ||
LottoTickets autoLottoTickets = lottoTicketsAutoInput.action(); | ||
|
||
return LottoTickets.of(manualLottoTickets, autoLottoTickets); | ||
} |
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.
객체를 생성하는 로직이 View, Controller 어느쪽의 책임에 더 가깝다고 생각하시나요?
LottoCount lottoAutoCount = lottoAmount.getLottoCount().minus(lottoManuelCount);
메시지를 보내는 로직 또한 view에 있으니 controller의 역할도 겸하고 있는 것 같아요.
@@ -28,7 +28,7 @@ static Stream<org.junit.jupiter.params.provider.Arguments> validAmounts() { | |||
void valid_amount_creates_LottoAmount(int input, int expectedCount) { | |||
LottoAmount amount = LottoAmount.of(input); | |||
assertEquals(input, amount.getValue()); | |||
assertEquals(expectedCount, amount.lottoCount()); | |||
assertEquals(expectedCount, amount.getLottoCount().getCount()); |
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.
객체 그래프가 깊어지면 get..get..get..get.. 될 가능성이 높습니다.
디미터 법칙을 적용해 보시면 좋을 것 같아요
@Test | ||
@DisplayName("LottoCount.minus로 정상적으로 값을 뺄 수 있다") | ||
void minusShouldSubtractProperly() { | ||
LottoCount base = LottoCount.of(5); | ||
LottoCount subtract = LottoCount.of(2); | ||
|
||
LottoCount result = base.minus(subtract); | ||
assertEquals(3, result.getCount()); | ||
} |
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.
minus 메서드는 도메인 로직에선 사용되고 있지 않고 view의 InputStrategy 에서만 활용되고 있어요.
프로그래밍 요구사항대로 TDD 사이클을 시도해보셨을까요?
커밋도 작은 단위가 아니여서 구현 흐름을 파악하기 어렵네요.
## 기능 목록 및 commit 로그 요구사항 | ||
- 기능을 구현하기 전에 README.md 파일에 구현할 기능 목록을 정리해 추가한다. | ||
- git의 commit 단위는 앞 단계에서 README.md 파일에 정리한 기능 목록 단위로 추가한다. |
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.
요구사항대로 '기능 목록'을 작성해 보시면 좋을 것 같아요.
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.
public LottoTickets read() { | ||
List<LottoTicket> manualTickets = lottoCount.intStream() | ||
.mapToObj(count -> getLottoTicket()) | ||
.collect(Collectors.toList()); | ||
|
||
return LottoTickets.of(manualTickets); |
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.
ManualInputStrategy 도 FixedPicker를 활용하면 어떨까요?
AutoInputStrategy 는 AutoPicker를 활용하더라구요.
No description provided.