-
Notifications
You must be signed in to change notification settings - Fork 355
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
Step3: 보너스 볼 추가 #996
base: baek0318
Are you sure you want to change the base?
Step3: 보너스 볼 추가 #996
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.
승화님 안녕하세요.
3단계 구현 하시느라 수고하셨습니다.
몇 가지 코멘트 남겼으니 확인 부탁드립니다.
질문 주신 내용은 코멘트로 남긴 내용을 반영 하시다 보면 해결될 것으로 보여요!
객체의 역할에 대해 조금 더 고민해 보시고 테스트 케이스도 좀 더 추가해 보면 좋을 것 같습니다.
@JvmInline | ||
value class LottoAnswer( | ||
private val answer: List<Int> | ||
data class LottoAnswer( |
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.
LottoAnswer라고 하니 어떤 역할인지 떠오르지 않았습니다.
저희는 이 도메인에 대해 지속적으로 고민하니 사용자에게 입력받은 값이라는 것을 압니다.
하지만 당첨번호를 의미하는 말로 바꾸면 좀 더 명확할 것 같다는 생각이 드네요.
private val answer: List<Int>, | ||
private val bonusNumber: Int |
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.
입력받은 값(당첨번호)은 이렇게 별도로 관리할 필요는 없을 것 같습니다.
현재 Lotto 클래스가 대체할 수 있지 않을까요?
당첨번호나 로또나 똑같이 6개의 번호를 가지는 동일한 형태니까요.
현재 객체들의 역할에 대해 조금 더 고민하시면서 변경해 보면 좋을 것 같아요.
.forEach { println(it) } | ||
println("총 수익률은 ${result.earningRate}입니다.") | ||
} | ||
|
||
private fun outputLottoResultSeperate(matchCount: MatchCount, amount: Int, earnResult: Map<MatchCount, Int>): String { |
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.
함수명은 보통 동사를 앞에 두죠.
val earningStrategy = mapOf( | ||
MatchCount.THREE to 5000, | ||
MatchCount.FOUR to 50000, | ||
MatchCount.FIVE to 1500000, | ||
MatchCount.FIVE_WITH_BONUS to 30000000, | ||
MatchCount.SIX to 2000000000 | ||
) |
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.
이렇게 매핑을 하기보다 힌트에도 있듯이 Enum으로 관리해 보면 어떨까요?
그리고 지금처럼 컨트롤러에 이 로직이 있는 것이 적합한지도 고민해 보시면 좋을 것 같아요.
|
||
object OutputView { | ||
|
||
fun outputBuyResult(lottoCount: Int, lotteries: List<Lotto>) { | ||
println("${lottoCount}개를 구매했습니다.") | ||
lotteries.forEach { | ||
println(it.numbers) | ||
println(it.numbers.sorted()) |
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.
로또 번호에 대한 중복 검사도 없는 것 같습니다.
확인 부탁드려요.
step3도 잘 부탁드립니다
고민사항
기존에는 LottoAnswer가 일급 컬렉션 이였는데 bonusNumber가 추가되게 되어서 일급컬렉션에서 벗어나게 되었는데 별도의 일급컬렉션을 만들어야 할까요?