[Step2] 로또 제출합니다. #1093
[Step2] 로또 제출합니다. #1093
Conversation
| @@ -0,0 +1,20 @@ | |||
| # 로또 | |||
| 로또를 만드는게 아니라 로또 당첨됐으면 좋겠다. | |||
| val resultView = ResultView() | ||
|
|
||
| val purchaseAmount = inputView.getPurchaseAmount() ?: return | ||
| val purchaseCount = purchaseAmount / LOTTO_PRICE |
There was a problem hiding this comment.
도메인의 책임이라 생각하여 PurchaseAmount 객체를 신설하여 내부로 로직을 밀어넣었습니다.
| val purchaseCount = purchaseAmount / LOTTO_PRICE | ||
|
|
||
| resultView.showPurchaseCount(purchaseCount) | ||
| val lottoBunch = LottoBunch(List(purchaseCount) { Lotto(RandomGenerator) }) |
There was a problem hiding this comment.
수업 시간에도 다뤄졌던 것처럼 부생성자를 활용을 고려해볼 수 있겠네요
There was a problem hiding this comment.
훨씬 깔끔해지고 좋은것 같습니다.
부생성자를 평소에 활용을 잘안하게 되었었는데 좋은 리뷰 감사드립니다.
| lottoNumberGenerator ?: return mutableSetOf() | ||
| val lotto: MutableSet<LottoNumber> = mutableSetOf() | ||
| while (lotto.size < LOTTO_NUMBER_COUNT) { | ||
| lotto.add(LottoNumber.get(lottoNumberGenerator.generateLottoNumber())) | ||
| } | ||
| return lotto |
There was a problem hiding this comment.
Mutable 타입 없이도 원하는 바를 구현하실 수 있습니다 🙂 Kotlin의 Collections API를 활용해보세요.
There was a problem hiding this comment.
힌트 또한 참고해보시면 어떨까요?
로또 자동 생성은 shuffled()을 활용한다.
sorted()를 활용해 정렬 가능하다.
contains()를 활용하면 어떤 값이 존재하는지 유무를 판단할 수 있다.
There was a problem hiding this comment.
(1..45).shuffled().take(6).toSet()힌트를 참고한다면 이런식으로 구현할 수 있을거것 같지만
buildSet이라는 api가 깔끔해보여서 해당 api를 사용해서 변경해보았습니다.
| @@ -0,0 +1,41 @@ | |||
| package lotto.domain | |||
|
|
|||
| class Lotto(private val lottoNumberGenerator: LottoNumberGenerator? = null, vararg lottoNumber: Int) { | |||
There was a problem hiding this comment.
책 엘레강트 오브젝트의 인자의 값으로 NULL을 절대 허용하지 마세요 파트를 참고해보시면 좋을 것 같아요.
더불어 현재의 init-require 로직은 어떤 것을 검증하고자 하는지 직관적으로 이해하기 어렵습니다. 다른 개발자들은 해당 로직을 보고 어떻게 이해할지 고민해보면 좋겠습니다.
There was a problem hiding this comment.
만약 lottoNumber가 10자리 들어오게 되면 어떨까요? 🤔
| val matchResults: Map<MatchingResult, Int> = getMatchLottoResult(winningNumbers) | ||
| var totalPrize = 0 | ||
| matchResults.forEach { (matchingResult, winCount) -> | ||
| totalPrize += (matchingResult.prizeAmount * winCount) |
| @JvmInline | ||
| value class LottoNumber private constructor(val value: Int) { | ||
| init { | ||
| require(value in 1..45) |
There was a problem hiding this comment.
LottoNumber를 새로 만들 때마다 Range 객체가 다시 만들어지고 있어요.
어떻게 개선할 수 있을까요?
There was a problem hiding this comment.
오 range가 객체생성이라는 감각조차 없었네요 좋은 리뷰 감사합니다.
1회만 생성하도록 변경하였습니다.
| } | ||
|
|
||
| "발급된 로또의 로또번호 갯수는 6개이다." { | ||
| Lotto(RandomGenerator).lottoNumbers.size shouldBe 6 |
There was a problem hiding this comment.
랜덤 로직을 참조하고 있는 테스트는 매번 실행할때마다 어떤 결과가 나올지 보장할 수 없습니다.
실제 객체에 버그가 있다고 하더라도 운이 좋게 통과할수도, 운이 나쁘게 실패할 수도 있다는 의미입니다.
| interface LottoNumberGenerator { | ||
| fun generateLottoNumber(): Int | ||
| } |
There was a problem hiding this comment.
테스트 가능성을 높이기 위한 인터페이스를 잘 설계해주셨지만, 막상 테스트에서는 LottoNumberGenerator의 구현체를 따로 만들어 활용하지 않고 있네요!?
| private val lottoNumbers: MutableMap<Int, LottoNumber> = mutableMapOf() | ||
|
|
||
| fun get(lottoNumber: Int): LottoNumber = | ||
| lottoNumbers[lottoNumber] ?: LottoNumber(lottoNumber).apply { | ||
| lottoNumbers[lottoNumber] = this | ||
| } |
There was a problem hiding this comment.
일종의 지연 초기화를 구현해주신 것 같은데요,
사실 로또 프로그램처럼 실제로 발생할 수 있는 로또 숫자가 제한적인 경우 최초에 참조될 때 1~45까지의 숫자를 모두 초기화해도 괜찮다고 생각하긴 합니다. 이 경우 굳이 MutableMap을 활용하지 않아도 되어요. 주관적인 의견이니 참고만 해주셔도 좋아요!
안녕하세요 리뷰어님
이번 미션은 최대한 담백하게 다 제외하고 요구사항만 맞추는 방향으로 단순하게 구현해보았습니다.
구체적이고 예쁜 구현은 리뷰 받은것을 철저히 지키며 적용해보려 합니다.
감사합니다.