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

Step2 구현 완료 (Step1 마지막 피드백 반영) #124

Merged
merged 27 commits into from
Mar 25, 2019

Conversation

CheolHoJung
Copy link

안녕하세요 현식님! 2단계 구현에 대한 피드백 요청합니다.

  • 당첨통계를 구하기 위해 티켓의 당첨 랭크를 구하는 부분을 WinningTicket 에서 담당합니다.
    • 기존에는 LotteryTicket에서 담당했습니다.
    • 처음에는 LotteryTicket을 상속하는 WinningTicket을 만들려고 했으나 Is - a 관계가 아닌 것 같아서 포함관계로 구현했습니다.

1단계 마지막에 주신 피드백의 결과로 Money 클래스가 추가되었습니다.

final int ticketCount = price.divide(LotteryTicket.PRICE);
// ?? -> final Money ticketCount = price.divide(LotteryTicket.PRICE);

한 가지 궁금한 점이, 가격을 나눠서 티켓의 갯수의 타입이 Money가 되어야 할까요? 아니면 새로운 클래스를 추가해야 할까요? (갯수 != 돈)

// LotteryMachine
public LotteryWinningStatistics raffle(List<Integer> winningNumbers, int bonusNumber) {
    return new LotteryWinningStatistics(new WinningTicket(winningNumbers, bonusNumber), this.lotteryTickets);
}

// 클라이언트
List<Integer> winningNumbers = InputView.inputWinningNumbers();
int bonusNumber = InputView.inputBonusNumber();
machine.raffle(winningNumbers, bonusNumber)

마지막으로, LotteryMachine에서 당첨번호 목록과 보너스 목록을 인자로 받아서 추첨을 수행하는데, WinningTicket의 생성자에 정의되 인자와 똑같습니다. 이런 경우, raffle의 인자 타입은 WinningTicket가 되어야 하는건가요? 그렇게 한다면, InputView에서 WinningTicket을 반환하는 것이 더 나은 구조가 되는거겠죠?

- LotteryNumber, LotteryTicket, LotteryWinningStatistics의 멤버변수에 final 지정
- LotteryMachine의numbersGenerator 멤버변수에 final 지정
가격을 나눠서 티켓의 갯수를 구하는 부분에서도 Money를 사용해야 할까?
(갯수 != 돈)
Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. :) 리뷰어 박현식입니다.
1단계에서 드렸던 피드백이 많이 반영됬네요!
전체적인 값객체 사용 👍
피드백 몇개 추가했구요. 참고하시고 궁금한 점 있으시면 DM 주세요. :)

src/main/java/lottery/domain/LotteryRank.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/LotteryRank.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/Money.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/LotteryTicket.java Outdated Show resolved Hide resolved
src/main/java/lottery/machine/LotteryMachine.java Outdated Show resolved Hide resolved
@CheolHoJung
Copy link
Author

전체적으로 변수명을 조금 변경하고, 피드백 주신 사항들 반영했습니다.
다시 피드백 부탁드립니다!

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰어 박현식입니다.
로또 랭킹 추상 메서드 구현 👍
전체적으로 깔끔하게 잘 구현해주셨어요.
2단계 머지할게요. :)
다만 몇가지 신경쓰이는 점, 및 개선할 수 있는 점 피드백 추가했습니다!
다음 단계 진행하시면서 같이 개선하시면 좋을 것 같아요 :)

@phs1116 phs1116 merged commit a0a4525 into next-step:CheolHoJung Mar 25, 2019
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