[Step3] 로또(2등) 구현 #1084

Seokho-Ham · 2024-11-26T06:47:30Z

안녕하세요 남재님! Step3 - 로또(2등) 피드백 요청드립니다.
2단계에서 주신 피드백까지 함께 반영해서 코드 변경이 많이 발생했습니다😓

<구현 사항>

클래스가 많아져서 구분을 위해 패키지로 나누었습니다.
Prize Enum 클래스를 Rank로 변경하였고 내부에 일치 여부를 확인하는 메서드를 추가하였습니다.
객체들의 책임을 각각의 역할에 맞게 분리하고자 노력했습니다.
- 이에 맞게 OrderService, WinningLottoService를 추가하였습니다.
- 기존의 LottoSystem의 클래스명을 LottoCreator로 변경하였습니다.
3단계 요구사항에 맞게 2등을 판별하는 기능을 구현하였습니다.

이번 미션을 통해서 객체와 각가의 책임에 대해 많이 생각해볼 수 있어서 재밌으면서도 어렵네요ㅎㅎ
그럼 이번 리뷰도 잘 부탁드리겠습니다!

Seokho-Ham · 2024-11-26T06:49:12Z

src/test/kotlin/lotto/WinningLottoServiceTest.kt

+        val result = winningLottoService.checkAndGetResult(order, winNumbers)
+
+        assertSoftly {
+            result.winningMatchCounts[0].rank shouldBe Rank.FIFTH
+            result.winningMatchCounts[0].totalCount shouldBe 1
+
+            result.winningMatchCounts[1].rank shouldBe Rank.FOURTH
+            result.winningMatchCounts[1].totalCount shouldBe 0
+
+            result.winningMatchCounts[2].rank shouldBe Rank.THIRD
+            result.winningMatchCounts[2].totalCount shouldBe 0
+
+            result.winningMatchCounts[3].rank shouldBe Rank.SECOND
+            result.winningMatchCounts[3].totalCount shouldBe 0
+
+            result.winningMatchCounts[4].rank shouldBe Rank.FIRST
+            result.winningMatchCounts[4].totalCount shouldBe 0
+        }
+    }


이런 부분은 forEach로 테스트를 해도 되는 부분일까요?
ParameterizedTest를 적용하기에는 만들어진 List의 결과값을 체크하는거라 애매한거 같아서 고민입니다.

val expected = listOf( Rank.FIRST to 0, Rank.SECOND to 0, ... ) expected.forEach{ ...

expected를 활용하는 방법도 있을거같아요 :)

namjackson

안녕하세요 석호님!
로또 3단계 고생하셨습니다
몇가지 추가적인 코멘트 남겻으니 확인해주세요! :)

namjackson · 2024-11-26T14:45:10Z

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

+            return entries.firstOrNull { it.match(matchCount, isBonusMatch) }
+                ?: throw RuntimeException("일치하는 숫자의 개수에 해당하는 상품이 존재하지 않습니다.")


에러를 던지는것보다 null 이나 MISS 를 리턴함으로써
에러를 대체할수 있어요 :)
실제로 코틀린에서는 에러를 던지기보단 null을 반환하기도 해요!

음 이건 궁금증이 생기는 부분이네요!
null을 반환하면 해당 결과값을 받는 코드에서는 nullable하게 처리가 될거 같습니다.
그럼 접근할때 NullPointException이 발생하는 경우가 발생할수도 있을텐데 예외를 던지는것보다 null로 던지는게 권장되는걸까요?! 실전에서는 이런 부분에 대해서는 보통 어떻게 처리를 하나요?

추가적으로 이번 과정을 하면서 예외를 명시적으로 던지는것을 권장하지 않는듯한 느낌을 받았는데 혹시 이유가 있을까요?!
(코틀린을 처음 사용해봐서 아직 모르는게 많아서 질문드립니다ㅎㅎ)

코틀린에서는 Checked Exception를 강제하지 않는 등의 이유로
예외를 명시적으로 던지는걸 권장하지는 않아요
경우에 따라 Null이나 sealed 클래스(ex> Result)를 많이 사용하곤 해요,

코틀린에서는 nullable인 경우에도 ?. , ?: 같은 키워드를 사용해서 nullSafe한 방법을 많이 활용하긴합니다!

namjackson · 2024-11-26T14:51:49Z

src/main/kotlin/lotto/service/WinningLottoService.kt

+                Rank.findByMatchCount(
+                    winningLotto.countMatchingNumbers(it),
+                    winningLotto.matchBonusNumber(it),
+                )


winningLotto의 함수들을 호출하고, 결과값으로 Rank를 생성하기보단
val rank = winningLotto.matchLotto(lotto)
와 같이 winningLotto에게 좀더 책임을 위임하는 구조는 어떠신가요?
조금더 객체끼리 협력할수 있는 구조는 아닐까요?

말씀해주신걸 듣고 나니 그게 더 나은 방식인거 같네요! 적용하겠습니다!

namjackson · 2024-11-26T15:55:03Z

src/main/kotlin/lotto/service/OrderService.kt

+import lotto.const.LottoConst.UNIT_OF_AMOUNT
+import lotto.domain.Order
+
+class OrderService() {


Service는 Spring의 레이어를 의미하는 접미사이고,
사실 애매한 네이밍인거 같아요
현실세계의 사물을 객체화해보면 어떨까요?
로또머신, 로또가게 등의 사물을 객체화한 네이밍은 어떨까요?

�그런거 같네요! 바로 수정하겠습니다😀

namjackson · 2024-11-26T15:55:36Z

src/main/kotlin/lotto/service/OrderService.kt

+import lotto.domain.Order
+
+class OrderService() {
+    private val lottoCreator = LottoCreator()


내부 프로퍼티로 선언하기보단,
생성시점 주입하는 구조는 어떤 장점이 있을까요?

생각해보니 스프링의 의존성 주입처럼 생성자를 통해서 주입해주는게 좋을거같네요!

해당 클래스를 테스트하기 쉬워진다?

결합도가 낮아져서 유연해진다?

생각나는 장점들은 이정도인것 같습니다!

namjackson · 2024-11-26T15:57:17Z

src/main/kotlin/lotto/service/WinningLottoService.kt

+    private val lottoCreator = LottoCreator()
+
+    fun createWinningLotto(
+        winningNumbers: Set<Int>,
+        bonusNumber: Int,
+    ): WinningLotto {
+        return lottoCreator.createWinningLotto(winningNumbers, bonusNumber)


WinningLottoService내에서 LottoCreator의 의존을 가질 필요가 있을까요?
val winningLotto = lottoCreator.createWinnerLotto(.. 같은 구조는 어떨까요?

winningLotto를 관리하는 서비스라고 생각해서 생성하는 책임까지 억지로 넣은것 같습니다😅
개선해보겠습니다!

namjackson · 2024-11-26T16:00:56Z

src/main/kotlin/lotto/service/WinningLottoService.kt

+        val winningMatchCounts = aggregateLottoResult(order.lottos, winningLotto)
+        val revenue = calculateRevenue(winningMatchCounts)
+        val rate = (revenue.toDouble() / order.amount.toDouble()).roundToInt()
+
+        return WinningResult(winningMatchCounts, revenue, rate)


총수익과 수익률을 계산해서 주기 보단,
WinningResult에게 책임을 위임해보는건 어떨까요?
비대해진 WinningLottoService의 책임을 줄일수 있을거같아요!

namjackson · 2024-11-26T16:04:45Z

src/test/kotlin/lotto/WinningLottoServiceTest.kt

+                setOf(
+                    LottoNumber(1),
+                    LottoNumber(2),
+                    LottoNumber(3),
+                    LottoNumber(7),
+                    LottoNumber(8),
+                    LottoNumber(9),
+                ),


테스트 코드내에 로또 객체 생성 코드가 너무 길어진거 같아요!
객체 생성을 위한 테스트 헬퍼 함수 를 작성하면 어떨까요?
테스트 가독성이 늘어날거 같아요 :)

namjackson · 2024-11-26T17:11:33Z

src/test/kotlin/lotto/WinningLottoServiceTest.kt

+        val result = winningLottoService.checkAndGetResult(order, winNumbers)
+
+        assertSoftly {
+            result.winningMatchCounts[0].rank shouldBe Rank.FIFTH
+            result.winningMatchCounts[0].totalCount shouldBe 1
+
+            result.winningMatchCounts[1].rank shouldBe Rank.FOURTH
+            result.winningMatchCounts[1].totalCount shouldBe 0
+
+            result.winningMatchCounts[2].rank shouldBe Rank.THIRD
+            result.winningMatchCounts[2].totalCount shouldBe 0
+
+            result.winningMatchCounts[3].rank shouldBe Rank.SECOND
+            result.winningMatchCounts[3].totalCount shouldBe 0
+
+            result.winningMatchCounts[4].rank shouldBe Rank.FIRST
+            result.winningMatchCounts[4].totalCount shouldBe 0
+        }
+    }


val expected = listOf( Rank.FIRST to 0, Rank.SECOND to 0, ... ) expected.forEach{ ...

expected를 활용하는 방법도 있을거같아요 :)

Seokho-Ham · 2024-11-27T06:42:29Z

말씀해주신 내용들을 바탕으로 개선하였습니다! 다시 피드백 요청 드립니다:)

예외 대신 null을 반환하는거에 대해서는 코멘트로 질문을 남겨두었습니다!
그리고 질문이 하나 더 있는데 현재 공통으로 사용되는 상수값들을 LottoConst라는 object를 만들어서 모아두고 사용중입니다. 남재님은 상수값들이 여러곳에서 필요할때 어떻게 처리하시는지 궁금합니다!

namjackson

안녕하세요 석호님!
3단계 잘작성해주셨네요 👍 👍
몇가지 추가적인 코멘트 남겼으니, 확인해주세요 :)
코멘트는 다음단계에서 적용해주세요!

namjackson · 2024-11-28T02:20:14Z

src/main/kotlin/lotto/const/LottoConst.kt

+
+import lotto.domain.LottoNumber
+
+object LottoConst {


현재 공통으로 사용되는 상수값들을 LottoConst라는 object를 만들어서 모아두고 사용중입니다. 남재님은 상수값들이 여러곳에서 필요할때 어떻게 처리하시는지 궁금합니다!

저는 개인적으로 Const같은 상수 집합을 만드는것을 선호하지는 않아요!
프로젝트가 커지다보면, 상수가 많아져서 관리가 힘들기도 하고
관리의 범위가 애매해지는것 같아요 :)

LottoConst라는 상수집합을 만들기보단, 상수값은 보통 책임있는곳에서 정의하는 편 입니다!
UNIT_OF_AMOUNT로 선언하기 보단 Lotto.Price LottoShop.Price 와 같이 좀더 상수가 명확해지는 효과도 노릴수 있어요!
추가적으로 LOTTO_NUMBERS 관련된 기능은 Creator 객체의 책임은 아닐까요?

namjackson · 2024-11-28T02:50:13Z

src/main/kotlin/lotto/LottoCreator.kt

+        val randomNumbers = numberGenerator.generate()
+        val lotto = Lotto(randomNumbers.map { LottoConst.LOTTO_NUMBERS[it - 1] }.toSet())


미리생성한 로또넘버풀을 활용하면 어떨까요?
shuffled, take 등의 키워드를 활용해보아요!

namjackson · 2024-11-28T03:37:57Z

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

+class WinningLotto(
+    val winningNumbers: Lotto,
+    val bonusNumber: LottoNumber,
+) {


bonusNumber와 winningNumbers 중첩되는 상황도 고려하면 좋을거같아요 :)

namjackson · 2024-11-28T03:38:33Z

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

+    fun countMatchingNumbers(targetLotto: Lotto): Int {
+        return targetLotto.numbers.count { it in winningNumbers.numbers }
+    }
+
+    fun matchBonusNumber(targetLotto: Lotto): Boolean {
+        return targetLotto.numbers.contains(bonusNumber)
+    }


외부에서 사용되지 않는 함수는 private으로 하면 어떨까요 :)

namjackson · 2024-11-28T03:51:42Z

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

+            return entries.firstOrNull { it.match(matchCount, isBonusMatch) }
+                ?: throw RuntimeException("일치하는 숫자의 개수에 해당하는 상품이 존재하지 않습니다.")


코틀린에서는 Checked Exception를 강제하지 않는 등의 이유로
예외를 명시적으로 던지는걸 권장하지는 않아요
경우에 따라 Null이나 sealed 클래스(ex> Result)를 많이 사용하곤 해요,

코틀린에서는 nullable인 경우에도 ?. , ?: 같은 키워드를 사용해서 nullSafe한 방법을 많이 활용하긴합니다!

namjackson · 2024-11-28T03:57:30Z

src/main/kotlin/lotto/WinningLottoMatcher.kt

+import lotto.domain.WinningLotto
+import lotto.domain.WinningResult
+
+class WinningLottoMatcher {


WinningLottoMatcher의 책임은 무엇일까요?

WinningLotto를 파라미터로 받아서 WinningLotto의 함수를 실행시켜주고 있네요
WinningLotto의 책임은 아닐까요?

추가로 결과를 확인하기보단 결과를 가공해주는 로직이 더 많은건 아닐까요 :)
추첨 결과를 받아서 가공하는 책임은 따로 분리해도 좋을거같단생각이 들어요!
이부분 추가로 고민해보면 좋을거 같아요

namjackson · 2024-11-28T03:58:04Z

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

+data class LottoResult(
+    val totalCount: Int,
+    val rank: Rank,
+) {


로또의 결과보다는 로또의 통계를 나타내는건 아닐까요 ?:)

namjackson · 2024-11-28T04:01:08Z

src/test/kotlin/lotto/WinningLottoTest.kt

+        val lotto = createLotto(1, 2, 3, 4, 5, 6)
+        val bonusNumber = LottoNumber(7)
+
+        val winningLotto = WinningLotto(lotto, bonusNumber)
+
+        winningLotto.countMatchingNumbers(lotto) shouldBe 6


Suggested change

val lotto = createLotto(1, 2, 3, 4, 5, 6)

val bonusNumber = LottoNumber(7)

val winningLotto = WinningLotto(lotto, bonusNumber)

winningLotto.countMatchingNumbers(lotto) shouldBe 6

// given

val lotto = createLotto(1, 2, 3, 4, 5, 6)

val bonusNumber = LottoNumber(7)

val winningLotto = WinningLotto(lotto, bonusNumber)

// when

val result = winningLotto.countMatchingNumbers(lotto)

result shouldBe 6

개인적으로 테스트 코드 작성시에, given when then 을 분리해서 작성을 하는편이예요!
무엇을 테스트 할지, 무엇을 확인하는지 명확해지는 장점이 있어요!

Seokho-Ham added 5 commits November 25, 2024 23:03

refactor: 패키지 구조 변경

3bac9d0

refactor: 2단계 피드백 반영

7699cd2

refactor: Prize 클래스의 이름을 Rank로 변경 및 판별하는 함수 추가

64b4a78

refactor: 당첨 결과 계산 로직 수정

2e35465

step3: 보너스볼 기능 추가

8ca43a7

Seokho-Ham commented Nov 26, 2024

View reviewed changes

namjackson requested changes Nov 26, 2024

View reviewed changes

Seokho-Ham added 4 commits November 27, 2024 15:24

refactor: WinningResult 클래스 이동 및 총 수익과 수익률 계산 책임 부여

97f69a1

refactor: Rank 결과를 만드는 책임을 WinningLotto 클래스로 이동

fd8bfc2

refactor: 서비스 레이어 제거 및 클래스명 수정

094c279

refactor: 테스트 코드의 가독성을 향상을 위한 헬퍼 함수 추가 및 테스트 변경

82259ca

namjackson reviewed Nov 28, 2024

View reviewed changes

namjackson merged commit d32d509 into next-step:seokho-ham Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step3] 로또(2등) 구현 #1084

[Step3] 로또(2등) 구현 #1084

Seokho-Ham commented Nov 26, 2024

Seokho-Ham Nov 26, 2024

namjackson Nov 26, 2024

namjackson left a comment

namjackson Nov 26, 2024

Seokho-Ham Nov 27, 2024 •

edited

Loading

namjackson Nov 28, 2024

namjackson Nov 26, 2024

Seokho-Ham Nov 27, 2024

namjackson Nov 26, 2024

Seokho-Ham Nov 27, 2024 •

edited

Loading

namjackson Nov 26, 2024

Seokho-Ham Nov 27, 2024 •

edited

Loading

namjackson Nov 26, 2024

Seokho-Ham Nov 27, 2024

namjackson Nov 26, 2024

namjackson Nov 26, 2024

namjackson Nov 26, 2024

Seokho-Ham commented Nov 27, 2024 •

edited

Loading

namjackson left a comment

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

namjackson Nov 28, 2024

		return entries.firstOrNull { it.match(matchCount, isBonusMatch) }
		?: throw RuntimeException("일치하는 숫자의 개수에 해당하는 상품이 존재하지 않습니다.")

		val randomNumbers = numberGenerator.generate()
		val lotto = Lotto(randomNumbers.map { LottoConst.LOTTO_NUMBERS[it - 1] }.toSet())


		import lotto.domain.LottoNumber

		object LottoConst {

[Step3] 로또(2등) 구현 #1084

[Step3] 로또(2등) 구현 #1084

Conversation

Seokho-Ham commented Nov 26, 2024

<구현 사항>

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seokho-Ham 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

Seokho-Ham Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seokho-Ham 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

Seokho-Ham commented Nov 27, 2024 • edited Loading

namjackson 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

Seokho-Ham Nov 27, 2024 •

edited

Loading

Seokho-Ham Nov 27, 2024 •

edited

Loading

Seokho-Ham Nov 27, 2024 •

edited

Loading

Seokho-Ham commented Nov 27, 2024 •

edited

Loading