[부나] 2단계 로또 제출합니다. #30

tmdgh1592 · 2023-02-20T07:23:21Z

안녕하세요 제리님!
제리님의 피드백 덕분에 구현과 구조에 대해서 더 생각해볼 수 있었습니다.
그런데 궁금한 점이 생겨서 질문을 남겨보려고 합니다..!

1단계 미션의 마지막 피드백으로
rank 의 각 element 가 보너스 번호 검사 여부를 알고 있다면, 아래 조건문도 제거할 수 있지 않을까요? :) 라고 리뷰를 남겨주셨는데

enum class Rank(val countOfMatch: Int, val winningMoney: Int) {
    FIRST(6, 2_000_000_000),
    SECOND(5, 30_000_000),
    THIRD(5, 1_500_000),
    FOURTH(4, 50_000),
    FIFTH(3, 5_000),
    MISS(0, 0),
    ;

    companion object {
        fun valueOf(countOfMatch: Int, matchBonus: Boolean): Rank {
            if (SECOND.countOfMatch == countOfMatch && matchBonus) return SECOND
            if (SECOND.countOfMatch == countOfMatch && !matchBonus) return THIRD

            return Rank.values().find { it.countOfMatch == countOfMatch } ?: MISS
        }
    }
}

제가 이해한 바로는 Rank의 프로퍼티로 matchBonus와 같은 Boolean 타입을 추가해서, valueOf() 메서드 호출시 2등에 대한 비교를 하지 않아도 되는 구조로 변경해보기를 추천하신 것 같습니다!

여기서 궁금한 점은 만약 matchBonus와 같은 프로퍼티가 추가된다면, FIRST부터 MISS까지 요소를 각각 다른 이름으로 하나씩 더 만들어줘야 하는 불편함이 발생할 수도 있지 않을까 입니다..!
그럼에도 valueOf()에서 비교하는 것보다 어떠한 부분에서 이점이 있는지 궁금합니다! 🙂

- lotto 패키지 내 Number 관련 클래스는 lotto.number 패키지로 이동 - Rank 클래스는 Rank 패키지로 분리

- IntTest로 변경

vagabond95 · 2023-02-21T06:23:48Z

valueOf()에서 비교하는 것보다 어떠한 부분에서 이점이 있는지.
좋은 질문입니다 :)

말씀하신 것처럼 제가 제안 드린 방향은 enum 의 모든 element 에 추가 필드를 넣어줘야하는 일이 발생합니다.
반면, 앞으로 어떤 등수가 보너스 번호를 가지게 되더라도 if 문을 추가할 필요 없이, matchBonus 등의 필드만 수정하면 변경사항없이 요구사항을 만족할 수 있게 됩니다.

모든 구현에는 트레이드 오프요소가 존재합니다. 100% 장점만 있는 구현방식은 존재하지 않지요 ㅎㅎ
저는 이 중 후자의 요소를 더 중요하다고 생각하여 코멘트를 남겼습니다. 어떠한 비즈니스 로직을 수행할 때 일관된 규칙(보너스 번호 여부를 검사할 것인가?)이 아니라 예외처리 방식 (2등만 검사)으로 구현을 하게 되면, 세부 히스토리를 파악하는데 드는 에너지가 더 많이들고 휴먼 에러가 자주 발생하더라구요.

vagabond95

2단계 미션도 잘 마무리해주셨네요 👍

질문 주신 내용을 비롯하여 몇가지 코멘트 남겼는데, 확인 후 리뷰요청 부탁드릴게요 :)

vagabond95 · 2023-02-21T06:31:00Z

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

+        private val NUMBERS: Map<Int, LottoNumber> = (MIN_LOTTO_NUMBER..MAX_LOTTO_NUMBER).associateWith(::LottoNumber)
+
+        fun from(number: String): LottoNumber {
+            val lottoNumber = requireNotNull(number.trim().toIntOrNull()) { ERROR_MESSAGE_NOT_NUMERIC_LOTTO_NUMBER }


number 가 int 로 변환할 수 있는 숫자 형태인지는
입력 레벨에서도 검증이 가능할 것 같은데요.

타입에 대한 검증을 view 에서 하는게 적절할지, domain 에서 하는게 적절할지 고민해봐도 좋겠습니다 :)

제리님의 피드백과 미션의 힌트를 함께 참고하여 수정해보았습니다.
논리적인 오류가 아닌, 단순히 값을 잘못 입력받은 상황이기 때문에 예외를 발생시키기보다 에러메시지를 출력해주고 재입력받도록 수정하였습니다!

vagabond95 · 2023-02-21T06:37:08Z

src/main/kotlin/domain/money/Money.kt

    init {
        require(amount >= MIN_AMOUNT) { ERROR_MESSAGE_NEGATIVE_AMOUNT }
    }

-    fun divideByThousand() = amount.div(THOUSAND)
+    fun divideBy(divisor: Money) = amount.div(divisor.amount)


해당 함수도 operator 로 사용해보는 것은 어떨까요?

사칙연산에 대한 일관적인 구현 방식이 제공되면 좋을 것 같아요.

감사합니다 : )
해당 메서드를 operator 로 변경하였습니다!

vagabond95 · 2023-02-21T06:40:20Z

src/main/kotlin/view/ResultView.kt

-        println(PURCHASED_LOTTO_SIZE_FORMAT.format(lottos.size))
-        lottos.map { it.getSortedLotto() }.forEach { sortedNumber ->
+    fun printPurchasedLottos(manualLottos: List<ManualLotto>, autoLottos: List<AutoLotto>) {
+        println()


println(..) 에 대한 구현을 별도 함수로 한번 감싸보는 것은 어떨까요?

만약 표준 입출력에 대한 스펙 혹은 구현사항이 바뀔경우, 현재 구조에서는 println() 이 사용된 모든 코드에 변경이 필요 할 것 같아요.

오.. 미처 거기까지 생각하지 못했습니다!
표준 입출력에 대한 스펙과 구현이 언젠가 변경될 수도 있음을 감안하는 좋은 시야를 얻을 것 같습니다~ 👏

vagabond95 · 2023-02-21T06:42:08Z

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

+
+import domain.lotto.number.LottoNumber
+
+class AutoLotto(lottoNumbers: List<LottoNumber>) : PurchasedLotto(lottoNumbers)


AutoLotto 와 ManualLotto 의 구현상 차이가 없는데, 별도 클래스로 나누는 이유가 있을까요?
클래스로 나눴다면, 관련된 비즈니스 로직을 각 클래스에서 스스로 처리할 수 있도록 역할을 좀 더 부여해보면 좋겠어요.

출력 예시를 보면 수동으로 3장, 자동으로 11개를 구매했습니다.를 출력하고, 그 아래에 수동 로또 번호와 자동 로또 번호를 순서대로 출력하고 있습니다.
만약 ResultView에서 fun printPurchasedLottos(manualLottos: List<PurchasedLotto>, autoLottos: List<PurchasedLotto>)와 같은 형태로 전달받도록 한다면, 타입이 같다보니 실수로 반대로 출력하는 경우가 발생할 수도 있어서 이 둘을 별도의 클래스로 분리하고 상속하였습니다!
하지만 제리님 말씀대로 클래스로 나눴다면, 관련 비즈니스 로직을 포함하고 있으면 더 좋을 듯 싶습니다!
리팩터링 과정에서 고민해보겠습니다 : )

tmdgh1592 · 2023-02-21T08:00:06Z

valueOf()에서 비교하는 것보다 어떠한 부분에서 이점이 있는지.
좋은 질문입니다 :)

말씀하신 것처럼 제가 제안 드린 방향은 enum 의 모든 element 에 추가 필드를 넣어줘야하는 일이 발생합니다. 반면, 앞으로 어떤 등수가 보너스 번호를 가지게 되더라도 if 문을 추가할 필요 없이, matchBonus 등의 필드만 수정하면 변경사항없이 요구사항을 만족할 수 있게 됩니다.

모든 구현에는 트레이드 오프요소가 존재합니다. 100% 장점만 있는 구현방식은 존재하지 않지요 ㅎㅎ 저는 이 중 후자의 요소를 더 중요하다고 생각하여 코멘트를 남겼습니다. 어떠한 비즈니스 로직을 수행할 때 일관된 규칙(보너스 번호 여부를 검사할 것인가?)이 아니라 예외처리 방식 (2등만 검사)으로 구현을 하게 되면, 세부 히스토리를 파악하는데 드는 에너지가 더 많이들고 휴먼 에러가 자주 발생하더라구요.

답변해주셔서 감사합니다!
메서드의 로직을 단순화시키는 방법을 택함으로써 Human Error를 줄일 수 있는 점을 감안하면 제리님께서 말씀하신 방식에 더 눈길이 갑니다 : )

vagabond95

빠르게 코멘트 반영해주셨네요 👍

미처 코멘트 드리지 못했던 부분들도 스스로 개선하시는 부분도 멋지십니다 ㅎㅎ
관련하여 몇가지 고민해볼 점에 대해 추가로 코멘트 남겼는데 확인 부탁드릴게요!

vagabond95 · 2023-02-22T12:49:00Z

src/main/kotlin/controller/LottoGameController.kt

    }

-    private fun purchaseLotto(): Pair<Money, List<PurchasedLotto>> {
+    private fun inputLottos(): Triple<List<PurchasedLotto>, List<PurchasedLotto>, Money> {


인풋을 각 함수로 분리해보는 것은 어떨까요?
현재 구조에서는 input 으로 받는 값이 추가될 때마다 리턴형이 계속 바뀌어야 할 것 같아요.

관련 메서드끼리 묶어보려고 하다보니 오히려 변경에 취약한 구조가 된 것 같습니다..!
별도의 메서드로 분리해보겠습니다!

vagabond95 · 2023-02-22T12:53:43Z

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


-        fun from(number: String): LottoNumber =
-            LottoNumber(requireNotNull(number.toIntOrNull()) { ERROR_MESSAGE_NOT_NUMERIC_LOTTO_NUMBER })
+        private val NUMBERS: Map<Int, LottoNumber> = (MIN_LOTTO_NUMBER..MAX_LOTTO_NUMBER).associateWith(::LottoNumber)


캐시 활용 👍

vagabond95 · 2023-02-22T12:54:22Z

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

-
-            return Rank.values().find { it.countOfMatch == countOfMatch } ?: MISS
-        }
+        fun valueOf(countOfMatch: Int, matchBonus: Boolean): Rank =


보너스 번호를 요구하는 등수가 추가되더라도 해당 함수에는 변경이 발생하지 않겠네요 👍

vagabond95 · 2023-02-22T12:56:36Z

src/main/kotlin/domain/game/LottoMachine.kt

+    ): Pair<Money, List<PurchasedLotto>> {
+        require(lottoSize.value == lottoNumbers.size) { ERROR_MESSAGE_INVALID_PURCHASING_LOTTO_SIZE }
+        require(money.isGreaterThan(Money(LOTTO_PRICE * lottoSize.value))) { ERROR_MESSAGE_INSUFFICIENT_AMOUNT }
+        return Pair(


Pair 를 사용하는 것이 나쁜 것은 아니지만, 부득이한 경우가 아니라면 별도 클래스를 생성하고 그것을 이용해보는 것은 어떨까요?
Pair 단위의 데이터는 왜 묶였는지, 각각이 어떤 역할을 하는지 알기 어려울 수 있을 것 같아요.

확실히 Pair로만 반환한다면, 각 Pair의 요소가 어떤 의미인지 파악하기 어려울 수 있을 것 같습니다!
반환하고자 하는 객체를 담고 있는 별도의 data class를 생성해서 return해보는 방향으로 구현해보겠습니다.

기존 코드가 Pair 객체로 변수를 초기화 할 때 구조 분해 방식을 사용하고 있었기 때문에, 이를 수정하지 않아도 되는 data class를 사용하여 수정해보겠습니다!

…객체 반환하도록 변경

vagabond95

추가적으로 코멘트 드릴 부분이 보이지 않습니다 👍
로또 미션 진행하시느라 고생 많으셨어요.
해당 미션을 통해 얻어가는 것이 있으셨길 바랍니다 :)

남은 미션도 화이팅이십니다!

rhkrwngud445 added 30 commits February 14, 2023 17:04

docs: 기능목록 문서 작성

191e5cd

chore: 불필요한 파일 .gitkeep 제거

98d3d7f

feat: 로또 번호가 1이상 45이하인지 체크하는 기능 구현

ea9203a

docs: 로또를 winning lotto와 purchased lotto로 분리하여 기능 목록 작성

8698c00

refactor: LottoNumber 클래스를 domain의 lotto 패키지로 이동

395fbd6

refactor: 테스트명 띄어쓰기 변경

bce5bd8

feat: 로또 생성 기능 구현

b4286f1

feat: 로또 생성시 로또 번호가 6개인지 검증하는 로직 구현

5ce0145

chore: 에디터 설정 추가

b4b729b

feat: 로또 중복 검증 기능 구현

41cce16

refactor: Lotto 클래스가 numbers 인자를 위임 받는 형태로 변경

26ecd11

feat: Bonus number가 WinningLotto에 포함 여부 검증 기능 구현

02a7750

docs: 기능 목록 중복 제거

7a6a1e1

refactor: WinningLottoTest에 중복 테스트 코드 제거

6f01406

feat: PurchasedLotto에서 정렬된 로또를 반환하는 기능 구현

75a5bc7

refactor: LottoNumber의 equals 메소드 구현

67bd142

refactor: BonusNumber의 LottoNumber 상속을 위한 open class 적용

ff070ab

refactor: WinningLotto와 BonusNumber 분리

7f42ab9

refactor: LottoNumber의 equals 메소드 구현

2969fe6

refactor: WinningLotto에서 BonusNumber 분리

4c26e79

refactor: BonusNumber 포함여부 검증을 WinningLotto 생성시 하도록 변경

9e45c62

refactor: BonusNumber 포함여부 검증을 WinningLotto 생성시 하도록 변경

01ea82d

feat: Rank 클래스에서 맞춘 숫자 개수와 보너스 번호 일치여부가 주어졌을 때 등수를 반환하는 기능 구현

d6a6451

feat: PurchasedLotto에서 winningLotto와 bonusNumber로 당첨 등수를 반환하는 기능 구현

7be89dd

refactor: lotto 패키지내 number, rank 패키지 분리

46627ab

- lotto 패키지 내 Number 관련 클래스는 lotto.number 패키지로 이동 - Rank 클래스는 Rank 패키지로 분리

feat: 중복되지 않는 랜덤 번호 리스트 생성 기능 구현

1c5b3fb

feat: 정수를 1000으로 나누는 기능 구현

a95e3bf

refactor: 로또 범위와 로또 개수에 대해 접근제어자 public으로 변경

aded188

feat: 주어진 금액만큼 PurchasedLotto 리스트를 반환하는 기능 구현

87560f0

refactor: LottoGame -> LottoMachine으로 클래스명 변경

55a0f21

tmdgh1592 added 7 commits February 20, 2023 15:22

feat: LottoMachine 클래스에서 구매금액과 구매할 수동 로또 총 가격 비교 검증 기능 구현

72d1d76

refactor: LottoNumber 클래스에서 from() 메서드 호출시 인자를 trim() 하도록 변경

939e4c8

feat: 수동으로 구매한 로또 개수와 자동으로 구매한 로또 개수를 출력하는 기능 구현

6e7060e

study: SameTest 클래스명 변경

c296373

- IntTest로 변경

feat: PurchasedLotto를 AutoLotto와 ManualLotto로 분리

190bf81

refactor: 중복 코드 간소화 및 코드 포맷팅

8aa22f3

feat: Money 클래스를 비교하고 뺄셈하는 기능 구현

f6325e5

vagabond95 requested changes Feb 21, 2023

View reviewed changes

tmdgh1592 added 13 commits February 21, 2023 17:13

refactor: Rank 클래스의 프로퍼티로 matchBonus 추가 및 valueOf() 메서드 변경

0f5065b

refactor: Money 클래스의 divideBy() 메서드를 operator fun div() 메서드로 변경

a218e7e

refactor: InputView의 println() 관련 코드를 printMessage() 메서드로 분리

9271b63

refactor: 각 도메인의 숫자 여부 검증을 InputView에서 처리하도록 변경

f4ea237

refactor: InputView 클래스에 로또 번호들을 입력받는 중복 코드를 메서드로 분리

2198c33

refactor: WinningLotto가 Lotto를 상속받지 않고 조합하도록 변경

d691a8a

refactor: Lotto 클래스의 생성자 파라미터를 List에서 Set으로 변경

38e77a0

test: 테스트 코드 인자 오류 수정

9026934

refactor: 로또의 크기만 검증하도록 변경

93ae963

refactor: generateManualLotto()에서 인자 전달시 arraylist에서 set으로 변환하도록 변경

6969458

refactor: PurchasedLotto의 동등성 비교를 위해 equals(), hashcode() 메서드 구현

7e90fcc

test: 기존에 Lotto() 클래스의 인자로 list를 받던 테스트 코드를 set을 받도록 변경

9ae9c71

refactor: AutoLotto, ManualLotto 클래스 제거하고 PurchasedLotto로 대체

83a7b9e

vagabond95 requested changes Feb 22, 2023

View reviewed changes

tmdgh1592 added 4 commits February 22, 2023 22:25

refactor: LottoGameController 클래스에서 input 관련 메서드를 분리

526fcc5

refactor: purchaseManualLottos() 메서드 호출시 Pair 대신 LottoPurchaseResult …

93ca6a9

…객체 반환하도록 변경

refactor: purchaseAutoLottos() 메서드 코드 단순화

62c6451

refactor: 로또 매치 결과(맞춘 숫자 개수, 보너스 일치 여부)를 LottoMatchResult에 담아 반환하도록 변경

1eada2a

vagabond95 approved these changes Feb 23, 2023

View reviewed changes

vagabond95 merged commit 1b130ea into woowacourse:tmdgh1592 Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[부나] 2단계 로또 제출합니다. #30

[부나] 2단계 로또 제출합니다. #30

tmdgh1592 commented Feb 20, 2023

vagabond95 commented Feb 21, 2023

vagabond95 left a comment

vagabond95 Feb 21, 2023

tmdgh1592 Feb 22, 2023

vagabond95 Feb 21, 2023

tmdgh1592 Feb 21, 2023

vagabond95 Feb 21, 2023

tmdgh1592 Feb 21, 2023

vagabond95 Feb 21, 2023

tmdgh1592 Feb 21, 2023 •

edited

Loading

tmdgh1592 commented Feb 21, 2023

vagabond95 left a comment

vagabond95 Feb 22, 2023

tmdgh1592 Feb 22, 2023

vagabond95 Feb 22, 2023

vagabond95 Feb 22, 2023

vagabond95 Feb 22, 2023

tmdgh1592 Feb 22, 2023

vagabond95 left a comment


		import domain.lotto.number.LottoNumber

		class AutoLotto(lottoNumbers: List<LottoNumber>) : PurchasedLotto(lottoNumbers)

[부나] 2단계 로또 제출합니다. #30

[부나] 2단계 로또 제출합니다. #30

Conversation

tmdgh1592 commented Feb 20, 2023

vagabond95 commented Feb 21, 2023

vagabond95 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

tmdgh1592 Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

tmdgh1592 commented Feb 21, 2023

vagabond95 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

vagabond95 left a comment

Choose a reason for hiding this comment

tmdgh1592 Feb 21, 2023 •

edited

Loading