-
Notifications
You must be signed in to change notification settings - Fork 357
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] 로또 제출합니다. #1132
base: 2chang5
Are you sure you want to change the base?
[Step3] 로또 제출합니다. #1132
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단계 구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다
몇 가지 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.
궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!
MATCHED_FIVE(1_500_000, 5, BonusMatchResult.NOT_MATCH), | ||
MATCHED_FIVE_WITH_BONUS(30_000_000, 5, BonusMatchResult.MATCH), | ||
MATCHED_SIX(2_000_000_000, 6, BonusMatchResult.NO_EFFECT), ; | ||
|
||
companion object { | ||
private const val MATCH_NUMBER_TRANSFER_ERROR_MESSAGE = "로또 결과 이넘값 변환 오류가 발생하였습니다." | ||
private val matchNumberToMatchResultMap = entries.associateBy { it.matchNumber } | ||
private const val RELATED_BONUS_MATCH_NUMBER = 5 |
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.
5라는 숫자가 이렇게 3번 선언될 필요가 있을까요? 정답은 없을 것 같은데 어떻게 생각하시나요?
enum class MatchingResult(val prizeAmount: Int, val matchNumber: Int, val bonusMatchResult: BonusMatchResult) { | ||
MATCHED_THREE(5_000, 3, BonusMatchResult.NO_EFFECT), | ||
MATCHED_FOUR(50_000, 4, BonusMatchResult.NO_EFFECT), | ||
MATCHED_FIVE(1_500_000, 5, BonusMatchResult.NOT_MATCH), |
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.
NOT_MATCH와 NO_EFFECT가 의미있게 구분되어야 하는 이유가 있을까요?
purchaseAmount: Int, | ||
): Double { | ||
val matchResults: Map<MatchingResult, Int> = getMatchLottoResult(winningNumbers) | ||
val matchResults: Map<MatchingResult, Int> = getMatchLottoResult(winningNumbers, bonusNumber) |
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.
다른 개발자들도 어떤 의미인지 한 눈에 이해할 수 있도록 Kotlin의 named arguments를 활용해보시면 어떨까요~?
private fun getByAuto(): Set<LottoNumber> { | ||
lottoNumberGenerator ?: return setOf() | ||
return buildSet { while (size < 6) add(LottoNumber.get(lottoNumberGenerator.generateLottoNumber())) } | ||
private fun getByAuto(): Pair<Set<LottoNumber>, LottoNumber> { |
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.
다른 개발자들도 Pair<Set<LottoNumber>, LottoNumber>
을 보고 어떤 의미를 갖는 자료형인지 한눈에 이해할 수 있을까요?
} | ||
}) { | ||
private companion object { | ||
fun getLottoBunch(numbers: List<List<Int>>): LottoBunch = | ||
LottoBunch(numbers.map { Lotto(RandomGenerator).apply { setLottoByManual(*it.toIntArray()) } }) | ||
fun getLottoBunch(numbers: List<Pair<List<Int>, Int>>): LottoBunch = |
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.
다른 개발자들도 List<Pair<List<Int>, Int>>
을 보고 어떤 의미를 갖는 자료형인지 한눈에 이해할 수 있을까요?
fun match( | ||
winningNumber: List<LottoNumber>, | ||
bonusNumber: LottoNumber, | ||
): MatchingResult? { | ||
return MatchingResult.getResult(lottoNumbers.intersect(winningNumber).size, bonusNumber == this.bonusNumber) | ||
} |
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.
데이터를 꺼내어(get) 조작하지 않고, 메세지를 던져 객체가 일하도록 구조를 잘 만들어주셨네요!
} | ||
} | ||
|
||
fun setLottoByManual( |
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.
저는 개인적으로 private 함수보다는 public 함수를 먼저 위치시키는 것을 선호합니다. 다른 개발자들이 이 클래스의 역할을 파악할 때 public으로 외부에 드러난 함수를 먼저 보고, 그 다음에 숨겨진 함수를 보는 것이 자연스럽다고 느껴질 것 같아요.
(이 내용을 정리한 레퍼런스가 있었는데 못찾겠네요 😂
class Lotto(private val lottoNumberGenerator: LottoNumberGenerator) { | ||
var lottoNumbers: Set<LottoNumber> = getByAuto() | ||
var lottoNumbers: Set<LottoNumber> | ||
private set | ||
var bonusNumber: LottoNumber | ||
private set |
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.
우리가 일반적으로 로또를 구매할 때 보너스 번호를 같이 들고 있나요?
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단계 구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다
몇 가지 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.
궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!
안녕하세요 리뷰어님
로또 보너스 번호 구현해보았습니다.
리뷰 부탁드립니다. 감사합니다.