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

3단계 구현 #3973

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

3단계 구현 #3973

wants to merge 1 commit into from

Conversation

byh0923
Copy link

@byh0923 byh0923 commented Nov 7, 2024

너무 늦었네요.. 리뷰 부탁드립니다.. ㅠ

Copy link

@csh0034 csh0034 left a comment

Choose a reason for hiding this comment

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

3단계 진행하시느라 수고 많으셨습니다.
현재 모두 한 커밋에 되어 있고 추가된 기능에 대해서
테스트 코드가 작성되어 있지 않네요 😭
코멘트 확인해주시고 함께 고민해보시면 좋을것 같습니다.

@@ -34,8 +34,8 @@ public List<Integer> checkMatchingNumber(List<LottoNumber> inputWinningNumbers)
return totalLottoTicket.getMatchingCounts(new Lotto(inputWinningNumbers));
}

public Map<PrizePolicy, Integer> winningResult(List<Integer> matchingLottoTickets) {
return totalLottoTicket.calculateWinLottoTicket(matchingLottoTickets);
public Map<PrizePolicy, Integer> winningResult(List<Integer> matchingLottoTickets, LottoNumber bonusBall) {
Copy link

Choose a reason for hiding this comment

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

이전에 드린 코멘트와 같이 컨트롤러는 도메인을 상태로 관리하지 않아야합니다.
현재 LottoDisplay 가 컨트롤러의 역할처럼 보이네요!
현재 Lottos 를 통해 로직이 모두 처리되므로 옮겨보시는것도 좋을것 같아요.

추가로 checkMatchingNumber 의 반환값이 winningResult 로 반드시 전달 된다면
메서드를 분리하지 않는것이 명확하지 않을까요?

@@ -25,10 +25,13 @@ public List<Integer> getMatchingCounts(Lotto winningLotto) {
.collect(Collectors.toList());
}

public Map<PrizePolicy, Integer> calculateWinLottoTicket(List<Integer> matchingLottoTickets) {
public Map<PrizePolicy, Integer> calculateWinLottoTicket(List<Integer> matchingLottoTickets, LottoNumber bonusBall) {
Copy link

Choose a reason for hiding this comment

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

List<Integer> matchingLottoTickets, LottoNumber bonusBall

뒤의 인자는 입력한 보너스볼인데 앞의 인자는 입력한 로또 숫자가아니네요!
위의 드린 코멘트 참고하셔서 고민해보시면 좋을것 같습니다.

@@ -3,9 +3,13 @@
import java.util.Arrays;

public enum PrizePolicy {
ZERO(0, 0),
Copy link

Choose a reason for hiding this comment

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

당첨이 아닌 0,1,2 개 에 대해서 관리할필요가 있을까요? 🤔

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.

2 participants