Step3 - 로또 (2등) #953

sodp5 · 2023-11-16T16:54:52Z

안녕하세요!
현생에 치이느라 과제할 시간이 없어서 늦었습니다..ㅠㅠ

step2 에서 남겨주신 코멘트에 대한 얘기인데,
대부분 잘 이해가 되는 내용이었지만 한 가지 이유가 더 궁금한 코멘트가 있었습니다.

Lotto와 같은 도메인 로직을 가진 모델을 data class로 만들지 않는다는 내용이었는데요! #861 (comment)

개인적인 견해로는 copy 함수를 통해 관리가 어려워지는 부분은
이전 상태를 가지고 다음 상태를 만들기 때문에 copy가 많아지게 되면 어떻게 만들어졌는지 추적이 어려워지는것이라고 생각합니다.

하지만 저는 이것이 도메인 모델만이 해당되는게 아니라 data class로 만드는 모든 모델에 해당된다고 생각합니다.

Lotto가 도메인 정보를 포함하고는 있지만 사실 생성자 property에 같은 값이 존재한다면, 내부의 모든 기능이 동일하게 제공될것이기 때문에
일반적인 data class랑 구분되어 생각할 필요가 없다고 생각하는데요!

이 부분에 대해서 리뷰어님의 견해를 더 들어보고 싶습니다 🙇

ddaaac

안녕하세요 경문님 👋

몇 가지 코멘트 남겼으니 확인해주세요~

ddaaac · 2023-11-17T12:45:26Z

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

 data class Lotto(
    val numbers: List<Int>,
-    val winning: LottoWinning = LottoWinning.Nothing,
+    val bonusNumber: Int,


개인적인 견해로는 copy 함수를 통해 관리가 어려워지는 부분은
이전 상태를 가지고 다음 상태를 만들기 때문에 copy가 많아지게 되면 어떻게 만들어졌는지 추적이 어려워지는것이라고 생각합니다.
하지만 저는 이것이 도메인 모델만이 해당되는게 아니라 data class로 만드는 모든 모델에 해당된다고 생각합니다.
Lotto가 도메인 정보를 포함하고는 있지만 사실 생성자 property에 같은 값이 존재한다면, 내부의 모든 기능이 동일하게 제공될것이기 때문에
일반적인 data class랑 구분되어 생각할 필요가 없다고 생각하는데요!

어차피 프로퍼티의 접근제어자가 public이라면 Lotto 객체 외부에서 Lotto(lotto.numbers, lotto.bonusNumber, LottoWinning.of(correctCount, matchBonus)) 이렇게 객체를 만들수도 있고
data class라면 lotto.copy(winning = LottoWinning.of(correctCount, matchBonus))처럼 만들 수도 있으니 어차피 같지 않냐는 질문일까요?
고민을 많게 하게 하는 질문인 것 같습니다. 객체 외부에서는 copy를 호출하지 않는다라는 룰이 있으면 성립할 수 있는 얘기인 것 같아요.

하지만 몇 가지를 더 생각해볼 수 있을 것 같아요.

copy 기능은 너무 편리합니다. 이미 열려있는 저 기능을 안 쓰기가 쉽지 않을 정도로요.

Data classes in Kotlin are classes whose main purpose is to hold data. 데이터 클래스에 대한 코틀린의 정의입니다. 도메인 객체의 주 목적이 데이터를 들고 있는 것일지 생각해보면 좋을 것 같아요.

copy 대신에 직접 생성자를 통해 객체를 생성하는 기능을 구현해보시면 알겠지만, 굳이 copy를 사용하지 않고도 깔끔한 코드가 나옵니다. copy 기능 하나만 사용하기 위해 data class를 사용해야할 필요가 있을지도 고민해보면 좋을 것 같아요.

말씀주신 내용 흥미롭게 들었습니다 ㅎㅎ!
실제로, data 키워드를 뺀 코드를 살펴보았는데 이 또한 나름의 의미가 있다고 느껴져서 좋았습니다.

다만 추가로 궁금한점이 생기는것이 data class는 copy외에도 equals기능도 정말 강력하다고 느꼈는데요 (테스트가 와장창 깨지는것에서 ..ㅎ 😓)
data 키워드를 제외하고나니 테스트 코드를 내부 numbers list로 비교해야만 하더라구요

실무 환경에서는 더욱 복잡한 모델들이 존재하게 될텐데 이 때 리뷰어님의 테스트 코드 전략이 있을지 궁금합니다!
-> 모든 필드를 다 비교하기는 번거로움이 크다고 생각되어서요!

equals/hashcode를 위해 data 키워드를 쓰는거라면 직접 구현해주셔도 될 것 같네요.

ddaaac · 2023-11-17T12:46:40Z

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

+                Arguments.of(Lotto(listOf(1, 2, 3, 10, 11, 12), 8), LottoWinning.Fifth),
+                Arguments.of(Lotto(listOf(1, 2, 3, 4, 11, 12), 8), LottoWinning.Fourth),
+                Arguments.of(Lotto(listOf(1, 2, 3, 4, 5, 12), 8), LottoWinning.Third),
+                Arguments.of(Lotto(listOf(1, 2, 3, 4, 5, 6), 8), LottoWinning.First),


Second에 대한 테스트도 추가돼야하지 않을까요?

ddaaac · 2023-11-17T12:46:50Z

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


    companion object {
-        fun of(correctCount: Int): LottoWinning {
-            require(correctCount in 0..6) {
+        fun of(countOfMatch: Int, matchBonus: Boolean): LottoWinning {


이 로직에서 버그가 많이 발생하는데 잘 작성해주셨네요 👍

ddaaac · 2023-11-17T12:50:31Z

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

+    override fun generate(): Lotto {
+        val numbers = numberPool.shuffled()
+
+        val winningNumbers = numbers


구매하는 로또 번호에 winningNumber는 어색한 표현인 것 같아요.

ddaaac · 2023-11-17T12:50:41Z

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

+        val bonusNumber = numbers[Lotto.NUMBERS_COUNT]
+
+        return Lotto(winningNumbers, bonusNumber)


구매하는 로또에 보너스 번호가 필요할까요?

말씀주신 내용들을 종합해서 WinningLotto 라는 별개의 로또 클래스를 구현했습니다!

로또의 룰을 최대한 따라가기 위해 Lotto를 갖도록 하였는데요
Lotto 인터페이스를 두고 delegate로 구현할까 하다가 일단 포함하도록 구현만 해두고 리뷰어님 의견을 여쭤보려고 합니다
로또의 룰을 따라가는 방법이 꼭 delegate만 있을것 같진 않은데 어떻게 잘 활용해볼 수 있을까요??

지금 사용해주신 방법은 조합이라는 방식인데요, 중복을 제거하기에 인터페이스보다 조합이 더 낫다고 생각해요.
WinningLotto와 Lotto가 같은 인터페이스를 갖는 클래스들일까 고민해보시면 좋을 것 같습니다.

ddaaac · 2023-11-17T12:51:06Z

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

@@ -2,13 +2,14 @@ package lotto.domain

 data class Lotto(
    val numbers: List<Int>,
-    val winning: LottoWinning = LottoWinning.Nothing,
+    val bonusNumber: Int,


보너스 번호에 46이 들어오면 어떻게 될까요?

ddaaac

안녕하세요 경문님 👋

리뷰 반영 잘 해주셨어요.

다음 단계 진행해주세요~

ddaaac · 2023-11-21T11:56:32Z

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

-    fun match(winningNumbers: List<Int>): Lotto {
-        val correctCount = winningNumbers.count { numbers.contains(it) }
+    fun match(winningLotto: WinningLotto): Lotto {
+        val correctCount = winningLotto.lotto.numbers.count { numbers.contains(it) }


getter가 여러번 사용되면서 객체에서 값을 꺼내서 로직을 구현하고 있어요. 도메인 로직에는 최대한 getter를 지양하는 방향으로 구현해보시길 바랍니다. 자연스럽게 적절한 객체에게 책임이 갈 것 같아요. 테스트 구현도 쉬워지구요.

ddaaac · 2023-11-21T11:58:11Z

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

+        require(bonusNumber in Lotto.MIN_NUMBER..Lotto.MAX_NUMBER) {
+            "보너스 번호는 항상 ${Lotto.MIN_NUMBER}에서 ${Lotto.MAX_NUMBER}사이 값이어야 합니다."


이 로직이 중복으로 발생하네요. LottoNumber라는 값 객체를 만들어보는 건 어떨까요?

ddaaac · 2023-11-21T11:58:52Z

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

 data class Lotto(
    val numbers: List<Int>,
-    val winning: LottoWinning = LottoWinning.Nothing,
+    val bonusNumber: Int,


equals/hashcode를 위해 data 키워드를 쓰는거라면 직접 구현해주셔도 될 것 같네요.

ddaaac · 2023-11-21T12:02:26Z

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

    val numbers: List<Int>,
-    val winning: LottoWinning = LottoWinning.Nothing,
+    val winning: LottoWinning = LottoWinning.Miss,


당첨 번호에도 Lotto를 사용 중입니다. 그 곳에서의 Lotto에는 LottoWinning이 필요하지 않아보여요. 이 프로퍼티도 밖으로 빼서 누군가 구해줘도 될 것 같다는 생각이 드네요 😄

ddaaac · 2023-11-21T12:04:01Z

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

+        val bonusNumber = numbers[Lotto.NUMBERS_COUNT]
+
+        return Lotto(winningNumbers, bonusNumber)


지금 사용해주신 방법은 조합이라는 방식인데요, 중복을 제거하기에 인터페이스보다 조합이 더 낫다고 생각해요.
WinningLotto와 Lotto가 같은 인터페이스를 갖는 클래스들일까 고민해보시면 좋을 것 같습니다.

sodp5 added 7 commits November 17, 2023 01:02

중복비교 distinct로 변경

9e5b8b2

LottoGenerateStrategy가 Lotto 반환하도록 변경

a74c0af

winningNumber로 비교하는것이 아니라 Lotto 와 같은 규칙을 갖도록 winningLotto로 변경

da649a3

기능 명세 readme 업데이트

95db120

로또당첨에 2등을 추가한다.

88a0691

로또에 보너스 번호를 추가한다.

87229fc

2등에 관련된 IO를 구현한다.

8572ec3

ddaaac requested changes Nov 17, 2023

View reviewed changes

보너스 번호를 가진 WinningLotto 클래스 구현

58937ed

ddaaac approved these changes Nov 21, 2023

View reviewed changes

ddaaac merged commit 996caa9 into next-step:sodp5 Nov 21, 2023

sodp5 mentioned this pull request Dec 2, 2023

Step4 - 로또(수동) #997

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step3 - 로또 (2등) #953

Step3 - 로또 (2등) #953

sodp5 commented Nov 16, 2023

ddaaac left a comment

ddaaac Nov 17, 2023

sodp5 Nov 20, 2023 •

edited

Loading

ddaaac Nov 21, 2023

ddaaac Nov 17, 2023

ddaaac Nov 17, 2023

ddaaac Nov 17, 2023

ddaaac Nov 17, 2023

sodp5 Nov 20, 2023

ddaaac Nov 21, 2023

ddaaac Nov 17, 2023

ddaaac left a comment

ddaaac Nov 21, 2023

ddaaac Nov 21, 2023

ddaaac Nov 21, 2023

ddaaac Nov 21, 2023

ddaaac Nov 21, 2023

		val bonusNumber = numbers[Lotto.NUMBERS_COUNT]

		return Lotto(winningNumbers, bonusNumber)

		require(bonusNumber in Lotto.MIN_NUMBER..Lotto.MAX_NUMBER) {
		"보너스 번호는 항상 ${Lotto.MIN_NUMBER}에서 ${Lotto.MAX_NUMBER}사이 값이어야 합니다."

Step3 - 로또 (2등) #953

Step3 - 로또 (2등) #953

Conversation

sodp5 commented Nov 16, 2023

ddaaac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodp5 Nov 20, 2023 • 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

ddaaac 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

sodp5 Nov 20, 2023 •

edited

Loading