[아크] 2단계 로또 제출합니다. #31

re4rk · 2023-02-20T07:35:00Z

입력 부분에 있어서 수정을 많이 하였고 논리적인 오류가 아니면 null을 반환하여 다시 처리하도록 하였습니다!

malibinYun

2단계 미션 고생 많으셨어요 :)
피드백을 잘 반영해주셨네요 👍
코드가 이미 깔끔하고 잘 분리되어있어서, 추가 요구사항을 반영하는 데 큰 어려움이 없으셨을 것 같아요 🙂
고민해보면 좋을 점들에 코멘트 남겨두었습니다.
피드백 반영하시고 다시 리뷰 요청 부탁드릴게요!
질문이 있으시다면 언제든지 코멘트나 슬랙 DM으로 질문 주셔도 좋습니다 😊

malibinYun · 2023-02-20T15:00:12Z

src/main/kotlin/controller/LottoController.kt

+    private fun getChargeMoney(money: Money, manualLottos: List<Lotto>): Money =
+        Money.from(money.value - lottoStore.lottoPrice * manualLottos.size)


남은 돈을 구하기 위해서 수동 티켓 전체가 필요할까요? :)

사이즈를 전달하도록 수정하였습니다! 반영 커밋 : e4c8bf2

malibinYun · 2023-02-20T15:06:10Z

src/main/kotlin/domain/LottoStore.kt


-    fun buyLotto(amount: Int): List<Lotto> = LottoFactory(lottoGenerator).create(getCount(Amount.from(amount)))
+    fun buyManualLotto(money: Money, vararg lottos: Lotto): List<Lotto> {


이미 Lotto가 파라미터로 들어오고 있어 큰 의미를 부여하기 어려워 보입니다.
Lotto로 변환하는 역할을 이 객체에 위임해보는 것은 어떨까요?

말씀하신대로 InputView에서 객체 생성은 아닌 것같습니다! 반영 커밋 : fd6fe10

malibinYun · 2023-02-20T15:06:12Z

src/main/kotlin/domain/LottoStore.kt

+    fun buyAutoLotto(money: Money): List<Lotto> {
+        require(money.value >= LOTTO_PRICE) { ERROR_INVALID_MONEY.format(money.value) }
+        return LottoFactory(lottoGenerator).createLottos(getCount(money))
+    }


LottoFactory와 LottoStore는 하는 역할이 겹쳐보이네요.
또, 자동 티켓을 만들 때마다 LottoFactory 객체를 생성하는 것은 불필요해보입니다 :)

객체 간 역할을 다시 생각해보시고, 어느 한 쪽을 통폐합 하면 좋겠네요 🙂

저도 해당 내용에 대해 고민하고 있었어서 바로 수정하였습니다! 반영 커밋 : 50f2bee

malibinYun · 2023-02-20T15:11:40Z

src/main/kotlin/domain/WinningLotto.kt

            Rank.values().associateWith { rank ->
-                lottos.count { rank == Rank.valueOf(getCountOfMatch(it), matchBonus(it)) }
-            },
+                lottos.count { rank == Rank.valueOf(it.countMatch(lotto), it.contains(bonusNumber)) }
+            }.filterValues { it > 0 },


Rank.values() 로 먼저 값을 채우지 않고 시도해보세요 :)
갯수를 0이 아닌 것을 필터링 하더라도, 결국 불필요한 연산을 하고 있습니다.

먼저, 1장의 티켓을 비교하는 메서드를 분리해보세요 :)

생각해보니 불필요한 연산이 많이 있던 것같았습니다! 그룹화 하여 값을 생성하도록 변경 하였습니다! 반영 커밋 : bc2ef3e

malibinYun · 2023-02-20T15:14:02Z

src/test/kotlin/domain/LottoStoreTest.kt

+    @Test
+    fun `수동 로또를 구매한다`() {
+        val money = Money.from(1000)
+        val lotto: List<Lotto> = lottoStore.buyManualLotto(money, Lotto(1, 2, 3, 4, 5, 6))
+        assertThat(lotto.size).isEqualTo(1)
+    }
+
+    @Test
+    fun `자동 로또를 구매한다`() {
+        val money = Money.from(10000)
+        val lotto: List<Lotto> = lottoStore.buyAutoLotto(money)
+        assertThat(lotto.size).isEqualTo(10)
+    }


LottoStore는 Lotto를 생성하는 객체이기에, size 뿐만 아니라 원하는 티켓을 생성했는지도 검증해보는 것은 어떨까요?

해당 내용을 이해한대로 나올 수 있는 경우가 유효한 경우인지 판단하였는데 이게 맞는 방법인지는 잘 모르겠습니다!
반영 커밋 : 7740c74

malibinYun

피드백 반영 고생 많으셨어요!
코드가 깔끔히 잘 구현되어있어 리뷰 하기 편했습니다 :)
여러 고민해보면 좋을 점들에 코멘트 남겨두었어요!
피드백 반영하시고 다시 리뷰 요청 부탁드릴게요! 🔥

malibinYun · 2023-02-21T13:44:53Z

src/main/kotlin/domain/LottoNumber.kt

+@JvmInline
+value class LottoNumber private constructor(val number: Int) {


value class를 활용해주셨네요 👍

malibinYun · 2023-02-21T13:46:30Z

src/main/kotlin/domain/LottoStore.kt

+    val lottoPrice: Int
+        get() = LOTTO_PRICE


멤버변수에 할당하기 보다, 직접 LOTTO_PRICE를 접근해보는 것은 어떨까요?

malibinYun · 2023-02-21T13:47:11Z

src/main/kotlin/domain/LottoStore.kt

+    fun buyManualLotto(money: Money, vararg lottos: IntArray): List<Lotto> {
+        require(money.value >= LOTTO_PRICE * lottos.size) { ERROR_INVALID_COUNT.format(money.value) }
+        return lottos.map(::Lotto)
+    }
+
+    fun buyAutoLotto(money: Money): List<Lotto> {
+        require(money.value >= LOTTO_PRICE) { ERROR_INVALID_MONEY.format(money.value) }
+        return createLottos(getCount(money))
+    }


List<Lotto>를 일급 컬렉션으로 만들어보세요 🙂

수동과 자동이 나눠졌기 때문에 일급 컬렉션을 쓰니 훨씬 편해진 것같습니다! 감사합니다! 반영 커밋 : 6bf5fdf

malibinYun · 2023-02-21T13:48:15Z

src/main/kotlin/domain/LottoStore.kt

+    fun createLottos(count: Int): List<Lotto> {
+        require(count in MINIMUM_COUNT..MAXIMUM_COUNT) { ERROR_CREATE_COUNT.format(count) }
+        return List(count) { createLotto() }
+    }


이 메서드는 이 객체에서만 쓰이니 private으로 가리거나, 통폐합해보는 것은 어떨까요?

해당 기능 반영 하였습니다! 반영 커밋 : b415044

malibinYun · 2023-02-21T13:49:46Z

src/main/kotlin/domain/LottoStore.kt

+    private fun getCount(money: Money): Int = money.value / LOTTO_PRICE



LottoStore에서는 딱 로또의 발급 기능만 분리해보는 것은 어떨지 제안해봐요 :)
발급 받는 데 반드시 Money가 필요하다는 현실 세계의 반영이지만,
사실 발급기 그 자체에게는 돈이 필요하지 않지요 😊

해당 기능을 Money 객체에게 넘기고 Store에는 Money가 들어가지 않고 Count가 들어가도록 수정하였습니다! 반영 커밋 : b415044

malibinYun · 2023-02-21T13:54:21Z

src/main/kotlin/domain/WinningLotto.kt

        LottoResult(
-            Rank.values().associateWith { rank ->
-                lottos.count { rank == Rank.valueOf(getCountOfMatch(it), matchBonus(it)) }
-            },
+            lottos
+                .map { Rank.valueOf(it.countMatch(lotto), it.contains(bonusNumber)) }
+                .groupBy { it }
+                .mapValues { it.value.size },
        )


이 부분 고민 정말 많이 하셨을 것 같은데, 제가 의도하는 대로 구현해주셨네요 👍👍

아래 처럼도 구성해볼 수 있답니다 :) 참고해보세요!

return lottos.asSequence() .map { Rank.valueOf(it.countMatch(lotto), it.contains(bonusNumber)) } .groupingBy { it } .eachCount() .let { LottoResult(it) }

찾아본 결과 시퀀스는 대용량에 좋다고 이해할 수 있었습니다! 감사합니다! 반영 커밋 : 5b2633e

malibinYun · 2023-02-21T13:56:28Z

src/test/kotlin/domain/LottoStoreTest.kt

+        assertThat(lottos.size).isEqualTo(2)
+        assertThat(lottos[0].toList().map { it.number }).isEqualTo(listOf(1, 2, 3, 4, 5, 6))
+        assertThat(lottos[1].toList().map { it.number }).isEqualTo(listOf(1, 11, 22, 33, 44, 45))


검증문을 여러 개 쓰는 것과 assertAll로 감싸는 것은 어떤 차이점이 있을까요?

이 피드백은 조금 어려우셨을까요?
혹은 더 자세한 설명이 필요하신가요?

malibinYun · 2023-02-21T13:58:17Z

src/test/kotlin/domain/LottoStoreTest.kt

+        lottos.forEach { lotto ->
+            assertThat(lotto.toList()).hasSize(6)
+            lotto.toList().forEach { assertThat(it.number).isBetween(1, 45) }
+        }


반복문으로 검증문을 실행하는 것은 가독성을 해칩니다.
어떤 시점에 어떤 값을 비교하는 지 어렵기 때문이에요.

사이즈와 숫자의 범위를 검증하는 것은 큰 의미를 갖기 어렵습니다.
실제로 생성한 lotto가 정확하게 예측된 값을 가지는 지 비교해보는 것은 어떨까요?

예측할 수 있도록 수정하였습니다! 반영 커밋 : fd70c6b

malibinYun

2단계 미션 고생 많으셨어요!
아직 미션 기간이 많이 남았는데, 굉장히 빠르게 미션을 진행해주신 게 인상 깊었습니다 👍
이미 피드백을 잘 반영해주시기도 하셨고, 코드를 깔끔하게 구성해주셔서 이번에는 크게 리뷰 드릴게 없었네요 🙂
이번 미션은 이만 마무리하도록 하겠습니다 😊
다른 미션들도 잘 완주하시길 응원할게요! 💪

malibinYun · 2023-02-22T14:24:00Z

src/main/kotlin/domain/LottoTickets.kt

+
+class LottoTickets(private val lottoTickets: List<Lotto> = emptyList()) {


일급 컬렉션으로 잘 분리해주셨네요 👍

malibinYun · 2023-02-22T14:24:02Z

src/main/kotlin/domain/LottoTickets.kt

+    operator fun plus(lotto: LottoTickets): LottoTickets = LottoTickets(lottoTickets + lotto.lottoTickets)
+


plus 연산자 오버로딩을 잘 해주셨네요! 👍

malibinYun · 2023-02-22T14:25:32Z

src/test/kotlin/domain/LottoStoreTest.kt

+        assertAll({
+            assertThat(lottoTickets.size).isEqualTo(2)
+            assertThat(lottoTickets[0].toList().map { it.toInt() }).isEqualTo(listOf(1, 2, 3, 4, 5, 6))
+            assertThat(lottoTickets[1].toList().map { it.toInt() }).isEqualTo(listOf(1, 11, 22, 33, 44, 45))
+        })


이렇게 각 검증문들의 람다를 넘겨주면 됩니다 :)

Suggested change

assertAll({

assertThat(lottoTickets.size).isEqualTo(2)

assertThat(lottoTickets[0].toList().map { it.toInt() }).isEqualTo(listOf(1, 2, 3, 4, 5, 6))

assertThat(lottoTickets[1].toList().map { it.toInt() }).isEqualTo(listOf(1, 11, 22, 33, 44, 45))

})

assertAll(

{ assertThat(lottoTickets.size).isEqualTo(2) },

{ assertThat(lottoTickets[0].toList().map { it.toInt() }).isEqualTo(listOf(1, 2, 3, 4, 5, 6)) },

{ assertThat(lottoTickets[1].toList().map { it.toInt() }).isEqualTo(listOf(1, 11, 22, 33, 44, 45)) },

)

방금 함수 정의 부분 열어보고 vararg을 전달하는 것으로 해당 내용에 대해서 이해하였습니다!

fun assertAll(vararg executables: () -> Unit) = assertAll(executables.toList().stream())

지금 제 코드처럼 하게 된다면 assertAll의 역할을 수행하지 못한다고 이해할 수 있었습니다!
알려주셔서 감사합니다!

re4rk added 20 commits February 20, 2023 16:33

refactor (Money): 직관적인 class 명으로 변경

be36df8

refactor (LottoResult): 불필요한 메소드 제거

cd6a9e5

refactor (Lotto): 메세지를 전달하여 값을 가져오도록 변 값 비교 메커니즘 수정

089d464

refactor (LottoResultTest): 테스트 값 비교시 하드코딩을 이용

b34fcb6

refactor (LottoResult): 사용하지 않는 메소드 제거

70a2987

refactor (WinningLotto): 0인 value를 가진 key값은 전달 되지 않도록 수정

15c84be

refactor (WinningLotto): 불필요한 메소드 제거

2cf74fd

refactor (InputView): 잘못된 값을 입력 받으면 null을 사용하여 다시 입력받도록 수정

d371888

refactor (View): 역할에 맞는 메소드명으로 변경

d716589

feat (LottoController): 내부적으로 생기는 Error들을 잡아서 다시 실행 할 수 있도록 변경

c7ca0e8

refactor (Money): 돈의 소지범위를 0원부터 시작하도록 변경

b2d39a3

docs (README): 2단계 기능구현 추가

3bc76ae

feat (LottoStore): 수동 로또 번호 구매기능 추가

f290c39

docs(README): 구현 목록 수정

36af2ad

feat(Lotto): 로또 번호를 입력받은 후에 구매하고 잔돈을 반환한다.

2844831

refactor(Lotto): 출력 결과에 맞춰 포맷팅

64db157

refactor(Lotto): Lotto를 data class로 변경

7da25c4

refactor(Lotto): LottoResult에서 함수에서 프로퍼티로 값을 처리하도록 변경

666790a

refactor(LottoFactory): create에서 메소드 역할에 맞는 createLottos로 변경

16db666

style(Lotto): 코틀린 컨벤션에 맞춰서 정렬

99eaf63

malibinYun requested changes Feb 20, 2023

View reviewed changes

re4rk added 7 commits February 21, 2023 00:41

Refactor(LottoStore): 비슷한 역할 클래스끼리 통합

50f2bee

Refactor(LottoStoreTest): 사이즈 이외에 객체가 문제 없이 생성되는지 확인하도록 테스트 코드 수정

7740c74

Refactor(LottoStore): 숫자를 입력받아 객체를 생성하도록 역할 위임

fd6fe10

Refactor(LottoStore): 불필요한 인자 전달 수정

e4c8bf2

Refactor(LottoStore): 그룹화 시켜서 결과 생성하도록 변경

bc2ef3e

Refactor(LottoNumber): data class에서 value class로 변경

6e4b84c

Refactor(Money): data class에서 value class로 변경

b4ef046

malibinYun requested changes Feb 21, 2023

View reviewed changes

Refactor(Money): List<Lotto> 일급 객체 생성

6bf5fdf

re4rk added 7 commits February 22, 2023 01:27

Refactor(LottoTickets): 시퀀스 적용

5b2633e

Refactor(LottoTickets): LottoStore에서 돈관리 되지 않도록 수정

2c39379

Refactor(LottoStore): 외부에서 사용하지 않는 public 메소드 제거 및 역할에 나누기

b415044

Refactor(LottoStoreTest): TestLottoGenerator 생성으로 예측할 수 있도록 수정

fd70c6b

Refactor(LottoStore): 메소드 개수 간소화

ace0638

Refactor(LottoNumber): int형을 가져오기 위해 toInt 적용

b4ef7f8

Refactor(LottoNumber): int형을 가져오기 위해 toInt 적용

6790510

malibinYun approved these changes Feb 22, 2023

View reviewed changes

malibinYun merged commit 4dddc5b into woowacourse:re4rk Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[아크] 2단계 로또 제출합니다. #31

[아크] 2단계 로또 제출합니다. #31

re4rk commented Feb 20, 2023

malibinYun left a comment

malibinYun Feb 20, 2023

re4rk Feb 20, 2023

malibinYun Feb 20, 2023

re4rk Feb 20, 2023

malibinYun Feb 20, 2023

re4rk Feb 20, 2023

malibinYun Feb 20, 2023

re4rk Feb 20, 2023

malibinYun Feb 20, 2023

re4rk Feb 20, 2023

malibinYun left a comment

malibinYun Feb 21, 2023

malibinYun Feb 21, 2023

malibinYun Feb 21, 2023

re4rk Feb 21, 2023

malibinYun Feb 21, 2023

re4rk Feb 21, 2023

malibinYun Feb 21, 2023

re4rk Feb 21, 2023

malibinYun Feb 21, 2023

re4rk Feb 21, 2023

malibinYun Feb 21, 2023

malibinYun Feb 22, 2023

malibinYun Feb 21, 2023

re4rk Feb 21, 2023

malibinYun left a comment

malibinYun Feb 22, 2023

malibinYun Feb 22, 2023

malibinYun Feb 22, 2023

re4rk Feb 23, 2023

		private fun getChargeMoney(money: Money, manualLottos: List<Lotto>): Money =
		Money.from(money.value - lottoStore.lottoPrice * manualLottos.size)


		fun buyLotto(amount: Int): List<Lotto> = LottoFactory(lottoGenerator).create(getCount(Amount.from(amount)))
		fun buyManualLotto(money: Money, vararg lottos: Lotto): List<Lotto> {

		@JvmInline
		value class LottoNumber private constructor(val number: Int) {

		private fun getCount(money: Money): Int = money.value / LOTTO_PRICE


		class LottoTickets(private val lottoTickets: List<Lotto> = emptyList()) {

		operator fun plus(lotto: LottoTickets): LottoTickets = LottoTickets(lottoTickets + lotto.lottoTickets)

[아크] 2단계 로또 제출합니다. #31

[아크] 2단계 로또 제출합니다. #31

Conversation

re4rk commented Feb 20, 2023

malibinYun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malibinYun 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

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

malibinYun 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