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

3단계 - 로또(2등) #1114

Open
wants to merge 7 commits into
base: eshc123
Choose a base branch
from
Open

3단계 - 로또(2등) #1114

wants to merge 7 commits into from

Conversation

eshc123
Copy link

@eshc123 eshc123 commented Dec 3, 2024

주요 내용

  • 로또(2등) 추가 구현 하였습니다.
  • step-2 피드백에 대해 step-2 PR 코멘트에 커밋 아이디 남겨두었습니다.

Copy link

@BeokBeok BeokBeok left a comment

Choose a reason for hiding this comment

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

이전 단계 피드백과 로또(2등) 미션하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니, 충분히 고민해보시고 도전해보세요. 💪

Comment on lines +14 to +15
val matchingNumbers = lottoNumbers.numbers.count { winnerNumbers.contains(it) }
val matchingBonus = lottoNumbers.numbers.contains(bonusNumber)
Copy link

Choose a reason for hiding this comment

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

Lotto는 LottoNumbers에 대해 너무 잘 알고 있습니다.
LottoNumbers에 메시지를 보내도록 개선해보면 어떨까요?


import lotto.domain.lottonumber.RandomLottoNumbersGenerator

object LottoVendor {
Copy link

Choose a reason for hiding this comment

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

네이밍이 훨씬 좋아졌네요! 👍

companion object {
const val START_INDEX = 0
const val LOTTO_NUMBER_COUNT = 6
val LOTTO_NUMBER_RANGE = 1..45
Copy link

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

일급 컬렉션을 쓴다.

하나의 로또번호와 관련된 일급 컬렉션을 만들어보면 어떨까요?

private val LOTTO_NUMBER_RANGE = 1..45
}
class RandomLottoNumbersGenerator : LottoNumbersGenerator {
override fun generate(): LottoNumbers = LottoNumbers(LOTTO_NUMBER_RANGE.shuffled().subList(START_INDEX, LOTTO_NUMBER_COUNT).sorted())
Copy link

Choose a reason for hiding this comment

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

로또 번호가 중복으로 만들어질수는 없을까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

로또 번호가 중복으로 만들어 질 수 있다는 의미가 정확히 어떤 의미인지 궁금합니다! 현재 코드가 로또 번호가 중복으로 만들어 질 수 있다는 의미인지, 로또 번호가 중복으로 만들어질 수 있어야한다는 의미인지 궁금합니다!

Copy link

@BeokBeok BeokBeok Dec 10, 2024

Choose a reason for hiding this comment

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

@eshc123
제가 설명이 자세하지 못했네요 😅
로또 번호가 중복으로 만들어질 수 있다는 의미입니다!
1에서 45까지의 숫자를 랜덤으로 섞어서 추출하고 있지만, 랜덤으로 추출한 번호 앞 6자리가 동일하다면 로또 번호가 중복될 가능성이 있지 않을까요? 🤔

Copy link
Author

@eshc123 eshc123 Dec 11, 2024

Choose a reason for hiding this comment

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

중복도 가능하지 않을까 해서 따로 처리를 하지는 않았습니다! 로또 구매 시 자동으로 번호를 선택했을 때 똑같은 6개의 번호가 나올 수도 있을 것 같다고 생각은 하였는데, 말씀해주신대로 똑같은 6개 번호 묶음이 나오지 않도록 하는게 맞을 것 같네요!

@@ -20,7 +23,7 @@ object ResultView {
println(TEXT_RESULT_PURCHASE_NUMBER.format(lottos.size))
lottos.forEach { lotto ->
println(
lotto.lottoNumbers.joinToString(
lotto.lottoNumbers.numbers.joinToString(
Copy link

Choose a reason for hiding this comment

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

ResultView는 Lotto와 LottoNumbers에 대해서 너무 잘 알고 있습니다.
Lotto에 메시지를 보내도록 개선해보면 어떨까요?

println(TEXT_PRIZE_RESULT.format(prize.matchCount, prize.money, count))

lottoResult.rankCounts.filter { it.key != LottoRank.UNRANKED }.forEach { (rank, count) ->
val matchBonusText = if (rank == LottoRank.SECOND) TEXT_MATCH_BONUS_NUMBER else TEXT_EMPTY
Copy link

Choose a reason for hiding this comment

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

LottoRank에 값이 추가 또는 변경되었을 때 변경사항을 인지하기가 어려워보입니다.
변경사항을 쉽게 인지할 수 있도록 enum 비교를 exhausitive하게 개선해보면 어떨까요?

FIRST(6, 2_000_000_000),
;

companion object {
fun from(matchCount: Int) = LottoRank.entries.find { it.matchCount == matchCount }
fun from(
Copy link

Choose a reason for hiding this comment

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

from 함수를 만들어주셨네요. 👍
다만, 함수명만 보고서는 LottoRank가 어떤 방식으로 만들어지는지 이해가 잘 되지 않는데요.
함수명에 의도가 드러나도록 개선해보면 어떨까요?

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