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

[Step3] 로또 제출합니다. #1132

Open
wants to merge 4 commits into
base: 2chang5
Choose a base branch
from
Open

Conversation

2chang5
Copy link

@2chang5 2chang5 commented Dec 15, 2024

안녕하세요 리뷰어님
로또 보너스 번호 구현해보았습니다.
리뷰 부탁드립니다. 감사합니다.

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

안녕하세요 창환님!

3단계 구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다
몇 가지 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!

Comment on lines +8 to +13
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
Copy link
Member

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),
Copy link
Member

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)
Copy link
Member

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> {
Copy link
Member

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 =
Copy link
Member

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>>을 보고 어떤 의미를 갖는 자료형인지 한눈에 이해할 수 있을까요?

Comment on lines +40 to +45
fun match(
winningNumber: List<LottoNumber>,
bonusNumber: LottoNumber,
): MatchingResult? {
return MatchingResult.getResult(lottoNumbers.intersect(winningNumber).size, bonusNumber == this.bonusNumber)
}
Copy link
Member

Choose a reason for hiding this comment

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

데이터를 꺼내어(get) 조작하지 않고, 메세지를 던져 객체가 일하도록 구조를 잘 만들어주셨네요!

}
}

fun setLottoByManual(
Copy link
Member

Choose a reason for hiding this comment

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

저는 개인적으로 private 함수보다는 public 함수를 먼저 위치시키는 것을 선호합니다. 다른 개발자들이 이 클래스의 역할을 파악할 때 public으로 외부에 드러난 함수를 먼저 보고, 그 다음에 숨겨진 함수를 보는 것이 자연스럽다고 느껴질 것 같아요.

(이 내용을 정리한 레퍼런스가 있었는데 못찾겠네요 😂

Comment on lines 3 to +7
class Lotto(private val lottoNumberGenerator: LottoNumberGenerator) {
var lottoNumbers: Set<LottoNumber> = getByAuto()
var lottoNumbers: Set<LottoNumber>
private set
var bonusNumber: LottoNumber
private set
Copy link
Member

Choose a reason for hiding this comment

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

우리가 일반적으로 로또를 구매할 때 보너스 번호를 같이 들고 있나요?

https://dhlottery.co.kr/common.do?method=main

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

안녕하세요 창환님!

3단계 구현을 잘 해주셨습니다 💯 깔끔한 구현이 좋았습니다
몇 가지 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!

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.

2 participants