[OptimistLabyrinth] 🚀 5단계 - 로또(수동) #949
Conversation
LottoGeneratorFromWinningNumbers 클래스에서 확인하던 로직을 Lotto 클래스 생성자에서 확인하도록 이동
LottoCalculationUtils 클래스에 있던 getMatchCount 메서드를 Lotto 클래스로 이동 setCountForEachWinningRank 메서드에서 예외 처리 부족으로 발생하던 버그 수정
seokhongkim
left a comment
seokhongkim
left a comment
There was a problem hiding this comment.
기성님, 5단계 미션 잘 구현해주셨습니다. 👍
코드를 보고 궁금한 점과 고민해보시면 도움이 될 것 같은 내용을 코멘트로 남겼습니다.
코멘트 확인 부탁드립니다. 🙏
| this.lottoNumbers = lottoNumbers; | ||
| } | ||
|
|
||
| private void checkDuplicateNumberInList(List<LottoNumber> winningLottoNumbers) throws IllegalArgumentException { |
There was a problem hiding this comment.
IllegalArgumentException은RuntimeException이니 명시적인 throws 구문이 없어도 될 것 같아요.- 우승 번호를 저장할 때도
Lotto를 사용하지만, 구매한 로또를 관리할 때도Lotto를 사용하는 것 같아요. 그럼winningLottoNumbers보다는lottoNumbers가 조금 더 잘 설명해주는 변수 이름일 것 같네요! 예외 메시지도 당첨 번호가 아닌 로또 번호가 적절할 것 같습니다. :)
| return lottoNumbers; | ||
| } | ||
|
|
||
| public int getMatchCount(Lotto other) { |
There was a problem hiding this comment.
로또가 스스로 다른 로또와 일치하는 번호의 개수를 카운트할 수 있어서, 능동적인 객체 역할을 할 수 있게 된 것 같네요!
| @Override | ||
| public String toString() { | ||
| return lottoNumbers.toString(); | ||
| final List<LottoNumber> listToPrint = new ArrayList<>(lottoNumbers); |
There was a problem hiding this comment.
로또의 입출력을 콘솔에서 웹으로 변경한다고 상상해보면 어떨까요?
현재는 콘솔에 출력하고 있기에 toString에서 구현해도 괜찮지만, 웹으로 변경이 된다면 정렬해서 보여주는 것은 화면의 요구사항이 될 수 있겠네요. 아니면 화면에 보여줄 데이터를 조회할 때(e.g., dto 로 변환할 때, 또는 getSortedLottoNumbers와 같은 명시적인 메서드를 통해 호출될 때) 정렬을 하는 방법도 있을 것 같아요.
제 생각에는 toString에 화면 출력을 위한 요구사항을 구현하게 되면, 도메인이 화면에 대한 내용을 알게 되므로 ui에 의존적인 코드가 될 수 있습니다. :) 이 로직을 그대로 OutputView에서 구현해보셔도 좋을 것 같아요.
| return bonusNumber; | ||
| } | ||
|
|
||
| public void setBonusNumber(LottoNumber bonusNumber) { |
There was a problem hiding this comment.
WinningLotto 객체를 생성하는 시점에 bonusNumber도 인자로 전달 받으면 어떨까요?
그럼 WinningLotto를 불변 객체로 관리할 수 있고, bonusNumber는 lotto에 있는 번호가 될 수 없다(당첨번호와 보너스 번호는 중복될 수 없으니까요)는 밸리데이션도 할 수 있으며, 생성자는 객체가 생성될 때 필요한 조건을 모두 관리할 수 있겠네요!
보너스 번호가 당첨번호와 중복될 수 없다는 밸리데이션도 추가해보시면 좋겠습니다. :)
| return inputView.inputManyManualLotto(lottoCount); | ||
| } | ||
|
|
||
| private int getAutomaticLottoCount(MoneyToBuy moneyToBuy, ManualLottoCount manualLottoCount) { |
There was a problem hiding this comment.
수동 구매 수량만큼 제외하고 자동 구매 수량을 계산하는 것 잘 구현해주셨습니다. 👍
수동 구매 수량은 값 객체로 만드시고, 자동 구매 수량은 int 타입으로 관리하신 이유가 있는지 궁금하네요!
| return inputView.inputManualCount(moneyToBuy); | ||
| } | ||
|
|
||
| private List<Lotto> acceptManyManualLotto(ManualLottoCount lottoCount) { |
There was a problem hiding this comment.
Many는 몇 개를 의미할까요? 0개를 구매하는 경우에도 이 메서드는 호출될 수 있지 않을까요? :)
보통 복수개를 나타낼 때는 s를 붙여서 표현하는 것 같습니다. 예를 들면, acceptManualLottos, generateAutomaticLottos, displayLottos와 같이 변경해볼 수 있겠습니다.
| displayAllLotto(manualLottoCount, automaticLottoCount, lottoBucket); | ||
| final WinningLotto winningLotto = acceptWinningNumbers(); | ||
| acceptBonusNumber(winningLotto); | ||
| final Map<WinningRank, Integer> result = calculateLotto(lottoBucket, winningLotto); |
There was a problem hiding this comment.
이전 PR에 코멘트 남겨주신 부분에 대해 답변 드립니다. :)
그런데 WinningRank Enum 에서 LottoPrize Enum 에 있는 값을 사용한다는게
어떤건지 잘 감이 안 옵니다.
조금 더 힌트를 주실 수 있나요???
현재 calculateLotto에서 등수 계산을 저장하기 위해 WinningRank를 사용하고 있는데, WinningRank 대신 LottoPrize를 키로 사용해도 괜찮을 것 같아요. WinningRank를 사용하고 있는 모든 부분에 LottoPrize가 들어가도 코드가 잘 동작하도록 구현이 가능할 것 같습니다. :)
WinningRank에는 등수와 매치되는 개수를 관리하고 있는데요. 1. calculateLotto에서 이 값을 사용하지 않고 있습니다. 2. 이 값을 사용하더라도, LottoPrize에서도 동일하게 등수와 매치되는 개수를 가지고 있으므로, LottoPrize에 있는 값을 사용할 수 있습니다. 따라서, WinningRank 대신 LottoPrize의 값을 사용하고, WinningRank 클래스는 삭제해도 괜찮을 것 같습니다.
코드에 대한 코멘트와 별개로 enum 의 활용법은 나중에 시간이 되실 때 학습해보시면 도움이 될 것 같습니다.
enum을 주로 상수를 정의할 때 사용하기도 하지만, enum에 정의된 값은 모두 하나의 객체이므로, 이 점을 잘 사용하면 조금 더 변경에 유연하게 대응할 수 있는 코드를 작성할 수 있습니다. enum 으로 싱글턴 패턴 구현하기, enum 으로 전략 패턴 구현하기 등의 키워드로 학습해보시면 도움이 될 것 같습니다. :)
2 participants
No description provided.