Step4 - 로또 수동 #1070

gmltkddl1 · 2024-11-25T11:48:19Z

수동 기능 구현 및 원시값 포장을 구현했습니다.

객체에서 데이터를 받아오는 대신 객체에게 행동을 하도록 메세지를 보내게 수정 테스트 코드를 변경하여 일부 메소드 private으로 변경 람다에서 함수 레퍼런스로 변경

Readme에 요구사항 작성

수동 판매 테스트 케이스 작성 요구사항 '사용자가 수동으로 추첨 번호를 입력할 수 있도록 해야 한다'를 위한 함수 형태 변경 LottoNumber라는 wrapping class 추가

판매 메인함수 구현

wisemuji

안녕하세요 희상님!

4단계 구현을 잘 해주셨습니다 👍
마지막 단계인 만큼 몇 가지 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

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

wisemuji · 2024-11-26T07:31:58Z

src/main/kotlin/lotto/Main.kt

@@ -8,17 +10,23 @@ import lotto.view.LottoView
 fun main() {


main이 일종의 컨트롤러 역할을 한다면 View 역할을 하는 로직과 Domain 역할을 하는 로직이 혼재되어 있진 않나요?
이러한 로직들을 처리할 수 있는 더 적절한 객체에게 맡기면 어떨까요?

wisemuji · 2024-11-26T07:32:43Z

src/main/kotlin/lotto/Main.kt

+    val selectLottos: MutableList<LottoNumbers> = mutableListOf()
+    repeat(selectLottoCount) {
+        selectLottos.add(LottoNumbers(readln().split(",").map { LottoNumber(it.toInt()) }))
+    }


MutableList 없이 Kotlin의 Collections API를 활용해보면 어떨까요?

wisemuji · 2024-11-26T07:33:24Z

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

+
+class Lottos(val lottos: List<Lotto>) {
+    fun drawLottos() {
+        lottos.forEach(LottoView::drawLotto)


domain 로직에 LottoView가 포함된 것이 적절할까요?

#1051 (comment)
메세지로 던져해보라 하셔서 이렇게 했는데
로직이 view로직 뿐이라 일단 이렇게 했는데 결국 위 경우는 객체에게 시킬수있는 일은 단순히 Lottos 안에 리스트를 떠 간단하게 리턴시켜주는일 뿐으로 보이는데 추가적으로 할 수 있는게 있을까요?
lottoResult.lottos.lottos.forEach { drawLotto(it) } -> lottoResult.getLottos().forEach { drawLotto(it) }

wisemuji · 2024-11-26T07:56:24Z

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

@@ -0,0 +1,3 @@
+package lotto.domain
+
+class LottoNumber(val number: Int)


LottoNumber에도 단순히 래핑 클래스 외에도 적절한 역할과 책임을 부여할 수 있지 않을까요?
예를 들어 number로 초기화되는 과정에서 유효성 검사를 진행하는 것도 의미있는 객체의 역할이라고 볼 수 있습니다.

로또 번호는 1부터 45까지 번호 중 6개의 랜덤 조합이다

LottoNumber를 비롯하여 Lotto 객체에서도 유효성 검사 로직을 추가해보시면 어떨까요? 이는 테스트 시나리오로도 이어질 수 있겠네요!

또한 Kotlin의 Value Class를 적용해볼 수 있을 것 같아요!

wisemuji · 2024-11-26T08:00:51Z

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

            if (matchBonus) {
-                return entries.find { it.countOfMatch == countOfMatch } ?: MISS
+                return entries.find { it.countOfMatch == countOfMatch && it.isMatchBonusNeed }
+                    ?: entries.find { it.countOfMatch == countOfMatch } ?: MISS
            }
            return entries.filter { !it.isMatchBonusNeed }.find { it.countOfMatch == countOfMatch } ?: MISS


다른 개발자들도 이러한 중첩 조건문 구조를 한 눈에 이해할 수 있을까요? 🤔

wisemuji · 2024-11-26T08:01:31Z

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

-    }
+    companion object {
+        fun drawLottos(lottoResult: LottoResult) {
+            lottoResult.lottos.drawLottos()


메세지를 던지는 구조로 잘 만들어주셨네요 👍

wisemuji · 2024-11-26T08:02:11Z

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

-        println(lotto.numbers.joinToString(prefix = "[", postfix = "]", separator = ","))
-    }
+        fun drawLotto(lotto: Lotto) {
+            println(lotto.numbers.lottoNumbers.map { it.number }.joinToString(prefix = "[", postfix = "]", separator = ","))


다음 코멘트가 도움이 되길 바라요.
#1051 (comment)

wisemuji · 2024-11-26T08:03:01Z

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

-class LottoMatchMap(val lottoMatchMap: Map<Rank, Int>)
+class LottoMatchMap(val lottoMatchMap: Map<Rank, Int>) {
+    fun drawMatchMap() {
+        lottoMatchMap.forEach { println("${it.key}등 : ${it.value}개") }


domain 로직에 LottoView가 포함된 것이 적절할까요?

wisemuji · 2024-11-26T08:03:33Z

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

+    private fun getProfit(): Int {
+        return lottoMatchMap.map { it.key.winningMoney.times(it.value) }.sum()
+    }


다른 개발자들도 LottoMatchMap라는 객체가 수익률 계산을 해줄 것이라고 예상할 수 있을까요?

wisemuji · 2024-11-26T08:05:12Z

src/test/kotlin/lotto/LottoSellerTest.kt

+    @Test
+    fun `로또 판매자는 수동방식으로 로또를 판매한다`() {
+        val lottoSeller = LottoSeller()
+        val lottos =
+            lottoSeller.sellLottos(
+                3000,
+                listOf(
+                    LottoNumbers.of(listOf(1, 2, 3, 4, 5, 6)),
+                    LottoNumbers.of(listOf(1, 2, 3, 4, 5, 7)),
+                ),
+            )
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[0].number).isEqualTo(1)
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[1].number).isEqualTo(2)
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[2].number).isEqualTo(3)
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[3].number).isEqualTo(4)
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[4].number).isEqualTo(5)
+        assertThat(lottos.lottos[0].numbers.lottoNumbers[5].number).isEqualTo(6)
+
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[0].number).isEqualTo(1)
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[1].number).isEqualTo(2)
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[2].number).isEqualTo(3)
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[3].number).isEqualTo(4)
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[4].number).isEqualTo(5)
+        assertThat(lottos.lottos[1].numbers.lottoNumbers[5].number).isEqualTo(7)


AssertJ의 containsExactly를 활용해보시면 어떨까요?

피드백 반영 메세지 전달 위주로 동작 수정 클래스가 3개이상의 property를 가지지 않게 쪼개기 Main의 도메인 로직 제거 불변 객체로 변경

wisemuji

안녕하세요 희상님!

리뷰가 많았는데 반영하시느라 고생 많으셨습니다.
미션을 마무리하기 전에 고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.
특히 뷰 로직이 도메인 모델에서 분리될 수 있도록 개선이 필요할 것 같아요.

아마 다음 리뷰 요청에서는 머지할 수 있을 것 같은데요,
궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!

wisemuji · 2024-11-28T04:06:33Z

src/main/kotlin/lotto/Main.kt

-    val lottos = lottoSeller.sellLottos(purchasePrice, selectLottos)
-    println("수동 ${selectLottos.size}개 ${lottoSeller.getLottoPurchaseCount(purchasePrice) - selectLottos.size}개를 구매했습니다.")
+
+    val selectLottoNumbersList = List(selectLottoCount) { LottoNumbers(readln().split(",").map { LottoNumber(it.toInt()) }.toSet()) }


#1070 (comment)

위 도표를 참고하여 현재 main 함수에 있는 readln같은 로직도 Controller가 아닌, 별도의 역할을 담당하는 InputView와 같은 객체를 만들어보시면 어떨까요?

wisemuji · 2024-11-28T04:06:51Z

src/main/kotlin/lotto/Main.kt

+    val selectLottoNumbersList = List(selectLottoCount) { LottoNumbers(readln().split(",").map { LottoNumber(it.toInt()) }.toSet()) }
+    val lottoCustomerInput = LottoCustomerInput(purchasePrice, selectLottoNumbersList)
+
+    println("수동 ${lottoCustomerInput.getLottoSelectCount()}개 ${lottoCustomerInput.getLottoAutoCount()}개를 구매했습니다.")


이러한 로직도 컨트롤러가 아니라 뷰 로직으로 보는 것이 적절해보입니다 🙂

wisemuji · 2024-11-28T04:09:17Z

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

+    fun asJoinString(): String {
+        return lottoNumbers.map { it.number }.sorted().joinToString(prefix = "[", postfix = "]", separator = ",")
+    }


이 로직이 domain 모델에 있는 것이 적절할까요?

wisemuji · 2024-11-28T04:09:40Z

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

+    fun getProfitRateDescription(): String {
+        if (profitRate > 1.0) {
+            return "이득이에요"
+        }
+        return "손해에요"
+    }


이 로직이 domain 모델에 있는 것이 적절할까요?

wisemuji · 2024-11-28T04:22:40Z

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

 import lotto.domain.LottoResult

-class LottoView {
+object LottoView {
    fun drawLottos(lottoResult: LottoResult) {
        lottoResult.lottos.lottos.forEach { drawLotto(it) }


메세지로 던져해보라 하셔서 이렇게 했는데
로직이 view로직 뿐이라 일단 이렇게 했는데 결국 위 경우는 객체에게 시킬수있는 일은 단순히 Lottos 안에 리스트를 떠 간단하게 리턴시켜주는일 뿐으로 보이는데 추가적으로 할 수 있는게 있을까요?
lottoResult.lottos.lottos.forEach { drawLotto(it) } -> lottoResult.getLottos().forEach { drawLotto(it) }

원문 코멘트의 의도는 아래와 같이 값 자체를 타고 타고 들어가서 꺼내어 오는 것이 아니라, 적절한 객체의 책임으로 넘기길 바라는 마음이였습니다.

lottoResult.lottos.lottos.forEach { drawLotto(it) }

희상님이 제안해주신 아래와 같은 방법도 좋습니다.

lottoResult.getLottos().forEach { drawLotto(it) }

또는 내가 어떤 로또를 구매했는지를 알기 위해서 꼭 LottoResult를 참조하지 않아도 된다고 생각합니다.

wisemuji · 2024-11-28T04:24:29Z

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

+
+class LottoNumber(val number: Int) {
+    init {
+        require(number in Lotto.MINIMUM_NUMBER..Lotto.MAXIMUM_NUMBER) { "LottoNumber must be between 1 and 45" }


이 로직에 대한 테스트 코드도 추가하면 어떨까요?

wisemuji · 2024-11-28T04:25:59Z

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

+        const val PRICE = 1_000
+
+        fun of(lottoNumbers: LottoNumbers): Lotto {
+            return Lotto { lottoNumbers }


#1070 (comment)
위 코멘트 반영이 누락된 것 같아요.

gmltkddl1 added 4 commits November 24, 2024 21:15

refactor: step3 로또(2등) 피드백 반영

1a447fa

객체에서 데이터를 받아오는 대신 객체에게 행동을 하도록 메세지를 보내게 수정 테스트 코드를 변경하여 일부 메소드 private으로 변경 람다에서 함수 레퍼런스로 변경

feat: step4 로또(수동)

7176789

Readme에 요구사항 작성

feat: step4 로또(수동)

d87cc14

수동 판매 테스트 케이스 작성 요구사항 '사용자가 수동으로 추첨 번호를 입력할 수 있도록 해야 한다'를 위한 함수 형태 변경 LottoNumber라는 wrapping class 추가

feat: step4 로또(수동)

e3fe902

판매 메인함수 구현

wisemuji requested changes Nov 26, 2024

View reviewed changes

refactor: step4 로또(수동)

b5ef4d4

피드백 반영 메세지 전달 위주로 동작 수정 클래스가 3개이상의 property를 가지지 않게 쪼개기 Main의 도메인 로직 제거 불변 객체로 변경

wisemuji requested changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step4 - 로또 수동 #1070

Step4 - 로또 수동 #1070

gmltkddl1 commented Nov 25, 2024

wisemuji left a comment

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

gmltkddl1 Nov 27, 2024 •

edited

Loading

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji Nov 26, 2024

wisemuji left a comment

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

wisemuji Nov 28, 2024

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

		class LottoNumber(val number: Int)

Step4 - 로또 수동 #1070

Are you sure you want to change the base?

Step4 - 로또 수동 #1070

Conversation

gmltkddl1 commented Nov 25, 2024

wisemuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmltkddl1 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisemuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmltkddl1 Nov 27, 2024 •

edited

Loading