[Step2] 로또(자동) #1099

devtaebong · 2024-11-29T01:59:20Z

안녕하세요! 부족하지만 리뷰 잘 부탁 드립니다🙇‍♂️

Flamme1004K

안녕하세요!

2단계 미션 매우 잘 해주셨습니다. 코멘트 남겼으니 반영 후 다시 리뷰 요청 부탁드립니다.

감사합니다.

src/main/kotlin/calculator/StringAddCalculatorManager.kt

docs/step2.md

Flamme1004K · 2024-11-29T11:31:02Z

src/main/kotlin/lotto/LottoApplication.kt

+    val numberOfLotto = Cashier.purchaseLotto(amount)
+    val userLottos = List(numberOfLotto) { Lotto.createLotto(randomNumberListGenerator.generate()) }


cashier를 추가하셨군요~ 그렇다면, cashier가 lotto까지 주면 어떨까요?

src/main/kotlin/lotto/domain/Cashier.kt

Flamme1004K · 2024-11-29T11:36:26Z

src/main/kotlin/lotto/stretagy/RandomLottoNumberListGenerator.kt

+        private const val MINIMUM_NUMBER = 1
+        private const val MAXIMUM_NUMBER = 45


1과 45는 로또에서 매우 의미있는 값이라고 생각해요. 각 클래스마다 선언하지 말고, 한 곳에서 관리하면 어떨까요?

Flamme1004K · 2024-11-29T11:36:47Z

src/main/kotlin/lotto/domain/LottoNumber.kt

+        private val NUMBERS: Map<Int, LottoNumber> = (MINIMUM_NUMBER..MAXIMUM_NUMBER).associateWith { LottoNumber(it) }
+
+        fun from(number: Int): LottoNumber {
+            return NUMBERS[number] ?: throw IllegalArgumentException()


예외에 대한 메시지는 있으면 좋겠죠?

Flamme1004K · 2024-11-29T11:37:05Z

src/main/kotlin/lotto/domain/Statistics.kt

+            winningLotto: Lotto,
+        ): List<Statistics> {
+            val groupByRanking: Map<Int, List<Lotto>> =
+                (5 downTo 1).associateWith { emptyList<Lotto>() } +


(5 downTo 1) 은 무엇을 의미하는 걸까요?

의미있는 상수로 분리했습니다 🙇‍♂️

Flamme1004K · 2024-11-29T11:37:41Z

src/main/kotlin/lotto/view/InputView.kt

+            println(WINNING_NUMBER_INPUT_MESSAGE)
+            return readln().replace(REPLACEMENT_SOURCE, REPLACEMENT_TARGET)
+                .split(WINNING_NUMBER_DELIMITERS)
+                .also { if (it.size != 6) throw IllegalArgumentException(ERROR_WRONG_NUMBER_COUNT) }


if문 대신에 require()를 사용해 볼까요?

Flamme1004K · 2024-11-29T11:38:18Z

src/main/kotlin/lotto/view/OutputView.kt

+            println(LOTTO_STATISTICS_SEPARATOR)
+            statisticsList.filter { it.rank in 1..5 && it.rank != 2 }
+                .forEach { printLottoResult(it) }
+            val earningRatio = Statistics.calculateEarningRatio(statisticsList, amount)


도메인에 대한 로직이 뷰에 있네요. 분리해 볼까요?

Flamme1004K

안녕하세요!

2단계 미션 피드백 매우 잘 반영해 주셨어요! 이만 머지하도록 하겠습니다.

3단계 미션 화이팅입니다!

Flamme1004K · 2024-12-01T12:23:04Z

src/main/kotlin/lotto/domain/Cashier.kt

+        fun purchaseLotto(
+            amount: Int,
+            lottoNumberListGenerator: LottoNumberListGenerator,
+        ): List<Lotto> {
            require(amount >= LOTTO_PRICE)
-            return amount / LOTTO_PRICE
+            val numberOfLotto = calculateNumberOfLotto(amount)
+            return List(numberOfLotto) { Lotto.createLotto(lottoNumberListGenerator.generate()) }
        }


피드백 반영 잘 해주셨어요! 👍

Flamme1004K · 2024-12-01T12:24:27Z

src/main/kotlin/lotto/constant/LottoRankConstant.kt

+const val MINIMUM_NUMBER = 1
+const val MAXIMUM_NUMBER = 45
+const val FIRST_RANK = 1
+const val SECOND_RANK = 2
+const val THIRD_RANK = 3
+const val FOURTH_RANK = 4
+const val FIFTH_RANK = 5
+const val NO_RANK = 0
+const val MATCH_COUNT_SIX = 6
+const val MATCH_COUNT_FIVE = 5
+const val MATCH_COUNT_FOUR = 4
+const val MATCH_COUNT_THREE = 3


3단계 미션 때 많은 변화 기대하겠습니다 😊

Flamme1004K · 2024-12-01T12:30:13Z

src/main/kotlin/lotto/domain/LottoNumber.kt

+@JvmInline
+value class LottoNumber(val number: Int) {
+    companion object {
+        private const val MINIMUM_NUMBER = 1
+        private const val MAXIMUM_NUMBER = 45
+        private const val CANNOT_CREATE_LOTTO_NUMBER_MESSAGE = "Lotto number must be between %s and %s."
+
+        private val NUMBERS: Map<Int, LottoNumber> = (MINIMUM_NUMBER..MAXIMUM_NUMBER).associateWith { LottoNumber(it) }
+
+        fun from(number: Int): LottoNumber {
+            return NUMBERS[number] ?: throw IllegalArgumentException(
+                CANNOT_CREATE_LOTTO_NUMBER_MESSAGE.format(MINIMUM_NUMBER, MAXIMUM_NUMBER),
+            )
+        }
+    }


Suggested change

@JvmInline

value class LottoNumber(val number: Int) {

companion object {

private const val MINIMUM_NUMBER = 1

private const val MAXIMUM_NUMBER = 45

private const val CANNOT_CREATE_LOTTO_NUMBER_MESSAGE = "Lotto number must be between %s and %s."

private val NUMBERS: Map<Int, LottoNumber> = (MINIMUM_NUMBER..MAXIMUM_NUMBER).associateWith { LottoNumber(it) }

fun from(number: Int): LottoNumber {

return NUMBERS[number] ?: throw IllegalArgumentException(

CANNOT_CREATE_LOTTO_NUMBER_MESSAGE.format(MINIMUM_NUMBER, MAXIMUM_NUMBER),

)

}

}

@JvmInline

value class LottoNumber(val number: Int) {

init {

require(NUMBERS[number] != null) {

CANNOT_CREATE_LOTTO_NUMBER_MESSAGE.format(MINIMUM_NUMBER, MAXIMUM_NUMBER)

}

}

companion object {

private const val MINIMUM_NUMBER = 1

private const val MAXIMUM_NUMBER = 45

private const val CANNOT_CREATE_LOTTO_NUMBER_MESSAGE = "Lotto number must be between %s and %s."

private val NUMBERS: Map<Int, LottoNumber> = (MINIMUM_NUMBER..MAXIMUM_NUMBER).associateWith { LottoNumber(it) }

}

...

저는 코틀린을 사용하면서 정적 팩토리 메서드는 좀 더 명확하게 쓰이게 되더라구요. 기본 프로퍼티인 경우에는 기본 생성자를 쓰게 되고, 그 외 기본 프로퍼티와 다른 타입의 프로퍼티가 있으면 정적 팩토리 메서드를 사용하는 게 더 어울린다고 생각합니다. 그럼으로써 기본 프로퍼티를 적극적으로 사용하면, 초기 검증 로직도 init에 넣을 수 있어 더 직관적으로 보이는 부분도 있었어요.

이처럼 기본 생성자를 적극적으로 사용하고, init을 적극적으로 사용해보는 건 어떨까요?

Flamme1004K · 2024-12-01T12:30:24Z

src/main/kotlin/lotto/domain/LottoNumber.kt

+    override fun toString(): String {
+        return number.toString()
+    }


toString은 제거해도 좋아보이네요~

Flamme1004K · 2024-12-01T12:31:15Z

src/main/kotlin/lotto/domain/Match.kt

+import lotto.constant.NO_RANK
+import lotto.constant.THIRD_RANK
+
+class Match {


모든 함수가 정적 함수로 되어있네요~ object class를 사용해 보는 건 어떨까요?

Flamme1004K · 2024-12-01T12:34:38Z

src/main/kotlin/lotto/stretagy/RandomLottoNumberListGenerator.kt

+import lotto.constant.MINIMUM_NUMBER
+
+class RandomLottoNumberListGenerator : LottoNumberListGenerator {
+    override fun generate(): List<Int> {


Suggested change

override fun generate(): List<Int> {

override fun generate(): List<LottoNumber> {

Int가 아닌, LottoNumber를 주면 어떨까요?

Flamme1004K · 2024-12-01T12:35:26Z

src/main/kotlin/lotto/domain/Lotto.kt

@@ -0,0 +1,11 @@
+package lotto.domain
+
+class Lotto private constructor(val lottoNumbers: Set<LottoNumber>) {


여기도, init 메서드를 통하여 lottoNumbers가 6개로 이루어졌는지 검증 로직을 추가하면 어떨까요?

Flamme1004K · 2024-12-01T12:38:08Z

src/test/kotlin/lotto/domain/LottoTest.kt

+            sut.lottoNumbers.size shouldBe 6
+        }
+
+        it("로또의 각 숫자는 오름차순으로 정렬 되어 있어야 한다.") {


임의의 숫자가 아닌, 직접 숫자(Lotto.createLotto(List.of(1,2,3,4,5,6)))를 넣어서 검증하는 테스트도 있으면 좋겠네요~

Flamme1004K · 2024-12-01T12:39:15Z

src/test/kotlin/lotto/strategy/FifthRankLottoNumberGenerator.kt

+class FifthRankLottoNumberGenerator : LottoNumberListGenerator {
+    override fun generate(): List<Int> {
+        return listOf(1, 2, 3, 43, 44, 45)
+    }
+}


코틀린 파일에는 여러개의 클래스를 넣을 수 있어요. Fake 객체에 대해서는 하나의 코틀린 파일에 넣고 관리하는 게 어떨까요?

Flamme1004K · 2024-12-01T12:41:01Z

src/test/kotlin/lotto/domain/CashierTest.kt

@@ -0,0 +1,25 @@
+package lotto.domain


테스트에 대해서 대부분 성공에 대한 테스트로 이루어져 있네요.

예외에 대한 테스트 케이스도 있었으면 좋겠어요. 예외에 대해서 빠르게 체크하기 위해서도 테스트 케이스를 만드는 거라고 생각합니다.
항상 해피 케이스만 있는 건 아니잖아요!? 😊

Flamme1004K · 2024-12-01T12:44:05Z

#1099 (comment)

에 대한 코멘트 확인 부탁드려요!

그리고, 커밋 로그에 대해서 fix: 피드백 반영 가 아닌, 좀 더 기능적으로 어떻게 반영했는지 나누어서 커밋 로그를 만들었으면 좋겠어요!

ex)

apply: 로또 번호 상수 이동
apply: cashier를 통해 로또 구매 시 구매 한 로또를 반환하도록 변경

devtaebong added 24 commits November 27, 2024 21:10

fix: step1 피드백 반영

5ca0a2f

fix: step1 피드백 반영

68b68e3

docs: step2 요구사항 정리

b2b9a60

study: associate, associateWith, groupBy

aa96bb0

study: associate, associateWith, groupBy

84f5d63

test: 6자리 로또 번호를 생성한다.

bb10a4b

feat: 서로다른 6자리 로또 번호를 생성한다.

6742efb

feat: 로또를 구매한다

6d87c21

feat: 금액만큼 로또를 생성한다.

6125636

feat: 로또 구매 결과를 출력한다.

1afb5d2

feat: 로또 당첨번호를 입력받는다.

e2595e9

fix: 인스턴스 함수 동반객체로 이동

87eee8c

docs: 요구사항 문서 현행화

b3ee990

refactor: 로또 생성 전략패턴 적용

4dec135

test: 사용자의 로또번호와 당첨 로또번호를 기준으로 당첨 등수를 계산한다.

98a5815

feat: 사용자의 로또번호와 당첨 로또번호를 기준으로 당첨 등수를 계산한다.

44683a7

refactor: 상수, 함수 위치 변경

678b657

refactor: 로또 생성 로직 변경

7e7069b

feat: 로또번호와 당첨번호를 비교해서 로또 등수를 계산한다.

99f9bfc

refactor: 상수 분리

324a927

feat: 사용자가 구매한 로또번호 리스트의 등수를 계산한다

fef6a00

feat: 수익률 계산

4ad232c

feat: 로또 결과 메세지 출력

159dac5

test: 테스트 수정

f3cdfb4

Flamme1004K requested changes Nov 29, 2024

View reviewed changes

fix: 피드백 반영

ea6e757

devtaebong force-pushed the step2 branch from 3d0ce20 to ea6e757 Compare December 1, 2024 08:49

Flamme1004K approved these changes Dec 1, 2024

View reviewed changes

Flamme1004K merged commit 48c5fef into next-step:devtaebong Dec 1, 2024

		val numberOfLotto = Cashier.purchaseLotto(amount)
		val userLottos = List(numberOfLotto) { Lotto.createLotto(randomNumberListGenerator.generate()) }

		private const val MINIMUM_NUMBER = 1
		private const val MAXIMUM_NUMBER = 45

	override fun generate(): List<Int> {
	override fun generate(): List<LottoNumber> {

		@@ -0,0 +1,11 @@
		package lotto.domain

		class Lotto private constructor(val lottoNumbers: Set<LottoNumber>) {

[Step2] 로또(자동) #1099

[Step2] 로또(자동) #1099

Uh oh!

Conversation

devtaebong commented Nov 29, 2024

Uh oh!

Flamme1004K left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamme1004K left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamme1004K commented Dec 1, 2024

Uh oh!

Uh oh!