[Step2] 로또(자동) #759

songyi00 · 2023-06-23T13:23:30Z

안녕하세요 리뷰어님 😄
피드백 반영 및 2단계 미션 구현 완료하였습니다 이번 리뷰도 잘 부탁드립니다 🙇🏻‍♀️

(질문)
로또 번호 개수를 여러 클래스에서 검증하기 위해 LOTTO_SIZE 라는 상수를 LottoMachine 클래스 외부에 빼서 여러 클래스에서 공통으로 사용하였는데 더 좋은 방법이 있는지 궁금합니다!

malibinYun

2단계 미션 고생 많으셨어요!
빠른 진행을 위해 바로 머지하겠습니다 :)
피드백은 다음 미션에 반영해주세요!
질문은 코멘트로 답변 남겨놓을게요 🙂

malibinYun · 2023-06-24T14:43:00Z

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

+    fun buyTickets(purchaseAmount: Int): LottoTickets {
+        require(purchaseAmount >= 1000) { "최소 구매 금액은 1000원 입니다." }


입력 값이 무엇인지도 메시지에 담아보는 건 어떨까요?
디버깅할 때 매우 용이해집니다 :)

또, 비교문에 사용된 1_000 값은 아래 TICKET_PRICE 상수를 재활용해보는 건 어떨까요?

malibinYun · 2023-06-24T14:56:20Z

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

+    fun getMatchingResult(lottoTickets: LottoTickets): LottoResult {
+        val map = lottoTickets.numbers
+            .map { numbers -> countMatchingNumbers(numbers) }
+            .groupingBy { LottoRank.getLottoRankByMatchCount(it) }
+            .eachCount()
+        return LottoResult(map)
+    }


코틀린 확장함수 체이닝을 깔끔하게 작성해주셨네요!
마지막에 let을 활용하면 아래 처럼 전부 체이닝으로도 작성할 수 있어요!
아래는 참고정도만 하셔도 좋습니다 :)

Suggested change

fun getMatchingResult(lottoTickets: LottoTickets): LottoResult {

val map = lottoTickets.numbers

.map { numbers -> countMatchingNumbers(numbers) }

.groupingBy { LottoRank.getLottoRankByMatchCount(it) }

.eachCount()

return LottoResult(map)

}

fun getMatchingResult(lottoTickets: LottoTickets): LottoResult {

return lottoTickets.numbers

.map { numbers -> countMatchingNumbers(numbers) }

.groupingBy { LottoRank.getLottoRankByMatchCount(it) }

.eachCount()

.let { LottoResult(it) }

}

malibinYun · 2023-06-24T14:57:25Z

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

+
+class LottoNumberGenerator : NumberGenerator {


랜덤 로또 생성기를 interface를 통해 분리해주셨네요 👍

malibinYun · 2023-06-24T14:58:51Z

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

+) {
+    NONE(null, 0),


NONE 보다는, 꽝의 의미를 담아보는 건 어떨까요?
또, null 보다는 0 정도를 활용해도 괜찮다는 생각이 드네요!

@malibinYun 이 부분은 좀 고민했었는데 매치 개수가 1개나 2개여도 0으로 판단되는게 좀 그래서 null로 설정을 해주었는데 상수에 null을 넣는 것에 대해서 리뷰어님은 어떻게 생각하시나요? 👀

엄격히 구분한다면, 모든 경우를 enum으로 구분해야 된다고 생각해요.
저는 꽝이라면, 몇 개를 맞았는지 구분하지 않아도 충분하다는 생각이 들었어요. null을 넣어도 몇 개가 맞았는 지 구분은 어려우니, 까다로운 null 처리를 줄이기 위해 0을 택했어요!

어떤 선택도 정답은 아니니 null을 유지하셔도 좋습니다 :)

malibinYun · 2023-06-24T15:01:36Z

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

+    FOURTH(3, 5000),
+    THIRD(4, 50000),
+    SECOND(5, 1500000),
+    FIRST(6, 2000000000);


코틀린에서는 숫자를 표현할 때 언더바 ( _ )로 구분해 가독성을 더 높일 수 있어요!

Suggested change

FOURTH(3, 5000),

THIRD(4, 50000),

SECOND(5, 1500000),

FIRST(6, 2000000000);

FOURTH(3, 5_000),

THIRD(4, 50_000),

SECOND(5, 1_500_000),

FIRST(6, 2_000_000_000);

혹여나 1등이 2개 이상 발생할 수 있으니, 상금은 Long으로 변경해보는 건 어떨까요?

@malibinYun 제가 이해를 못했는데 1등이 2개 이상 발생하는 것과 Int를 Long으로 타입 변경이 어떤 관계가 있나용?

값을 꺼내 더할 때에 Int인지 Long인지 파악하지 못하고 바로 더해버리는 케이스가 있을 지도 몰라 제안드렸어요!

malibinYun · 2023-06-24T15:07:10Z

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

+    private fun getTotalProfit(): Int {
+        var total = 0
+        map.forEach { (lottoRank, count) ->
+            total += lottoRank.price * count
+        }
+        return total
+    }


아래 처럼 map 확장함수를 활용해볼 수 있겠네요 :)

Suggested change

private fun getTotalProfit(): Int {

var total = 0

map.forEach { (lottoRank, count) ->

total += lottoRank.price * count

}

return total

}

private fun getTotalProfit(): Int {

return map

.map { (lottoRank, count) -> lottoRank.price * count }

.sum()

}

malibinYun · 2023-06-24T15:07:35Z

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

+class Numbers(
+    val values: List<Int>
+) {


로또 번호 한 개를 하나의 값 객체로 표현해보는 건 어떨까요?

@malibinYun
primitive type을 한번 더 감싸서 래퍼 클래스로 만들었을 경우 좀 더 객체지향적인 구현이 가능하다는 장점이 있는데
각 도메인의 함수에서 다른 클래스를 인자로 받을 때 래퍼클래스 보다 원시 타입을 사용하는게 더 효율적인 경우(래퍼 클래스 내부 값에 대한 처리 필요한 경우)가 있는데 도메인 간 연관이 되어있을 때 항상 래퍼클래스로 인자를 받는게 코드 일관성 면에서 더 좋을지 아니면 어느정도 타협이 가능한 상황이 있는지 궁금합니다!

Int의 범위는 로또 번호의 범위를 월등히 뛰어넘어 로또 번호를 표현하기는 어렵다고 생각해요.
또, 메서드에 Int를 받으면 로또의 범위인지 알 수 없기에 검증 로직이 무수히 많이 늘어난다고도 생각해요.
래핑된 값 객체는 그 객체 만으로 로또 번호라는 신뢰를 가질 수 있어 큰 장점이라 생각해요!

내부 값에 대한 처리가 필요한 경우는, 해당 값 객체에 필요한 메서드를 추가해줌으로써 해결될 것으로 기대돼요 :)

malibinYun · 2023-06-24T15:09:31Z

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

+class WinningNumbers(
+    val numbers: List<Int>
+) {


winningNumbers도 결국 Numbers와 본질이 같다는 생각이 들어요 :)
로또 티켓 스스로가 자기 자신을 비교해서, 몇 개의 번호가 맞았는지 찾게끔 변경해보는 건 어떨까요?

Numbers를 그저 데이터를 담는 용도로 두는 것보다 스스로 몇개의 번호가 맞았는지 찾는 역할을 함께 한다면 LottoMatcher의 책임을 덜어낼 수 있을 것 같아요 😁

malibinYun · 2023-06-24T15:11:43Z

src/test/kotlin/lotto/LottoMachineTest.kt

+            shouldThrow<RuntimeException> {
+                lottoMachine.buyTickets(purchaseAmount)
+            }


require는 IlegalArgumentException을 발생시키니, 이 인스턴스를 비교해보는 건 어떨까요?
또, 어떤 에러 메시지가 나왔는 지 검증해보셔도 좋겠어요 :)

malibinYun · 2023-06-24T15:15:44Z

src/test/kotlin/lotto/LottoMachineTest.kt

+        // when
+        val actual = lottoMachine.buyTickets(purchaseAmount)
+
+        // then
+        actual.numbers shouldHaveSize numberOfTickets
+    }


생성된 개수 만을 검증하는 건 테스트에 큰 의미를 갖기 어렵습니다.
어떤 번호를 갖는 티켓을 만들어냈는 지 검증해보는 건 어떨까요?
이미 생성 전략에 대해 랜덤의 요소를 잘 분리해내셔서, 테스트를 작성하시는 데 큰 어려움이 없을 거예요! 😀

malibinYun · 2023-06-24T15:20:40Z

로또 번호 개수를 여러 클래스에서 검증하기 위해 LOTTO_SIZE 라는 상수를 LottoMachine 클래스 외부에 빼서 여러 클래스에서 공통으로 사용하였는데 더 좋은 방법이 있는지 궁금합니다!

LOTTO_SIZE를 가져야하는 객체가 갖고 있게 만들어보는 건 어떨까요?
로또 티켓 정도가 적절한 책임을 가질 것으로 기대되네요 :)
테스트에서는 상수를 직접 활용하기보다 하드코딩을 해보는 건 어떨까요?
테스트 하드 코딩에 대한 좋은 글을 공유드립니다 🙂
https://jojoldu.tistory.com/615?category=1036934

songyi00 added 11 commits June 22, 2023 19:54

refactor: step1 피드백 반영

e1869cc

docs: 기능 목록 작성

1919a70

feat: 입력받은 개수만큼 1-45 사이의 무작위 수를 생성해서 반환한다.

adf04a1

feat: 당첨 번호를 저장할 수 있다.

5e87dc0

feat: 당첨 번호와 일치하는 로또의 숫자 개수를 반환한다.

87e8599

feat: 결과에 대한 수익률을 계산해 반환한다.

fd4b690

feat: 구입 금액을 입력하면 구입 금액에 해당하는 로또 개수를 반환한다.

e7276f8

feat: 로또 1장의 가격(1000)보다 구입 금액이 적다면 RuntimeException 예외가 발생해야 한다.

e997fd3

refactor: 도메인 패키지 분리

69ad2f6

refactor: 전체적인 클래스 기능 리팩터링

fabe124

docs: update

ae8e6e6

malibinYun approved these changes Jun 24, 2023

View reviewed changes

malibinYun merged commit dcf52cd into next-step:songyi00 Jun 24, 2023

songyi00 mentioned this pull request Jun 25, 2023

[Step3] 로또(2등) #769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step2] 로또(자동) #759

[Step2] 로또(자동) #759

songyi00 commented Jun 23, 2023

malibinYun left a comment

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

songyi00 Jun 25, 2023

malibinYun Jun 26, 2023

malibinYun Jun 24, 2023

songyi00 Jun 25, 2023

malibinYun Jun 26, 2023

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

songyi00 Jun 25, 2023 •

edited

Loading

malibinYun Jun 26, 2023

malibinYun Jun 24, 2023

songyi00 Jun 25, 2023 •

edited

Loading

malibinYun Jun 24, 2023

malibinYun Jun 24, 2023

malibinYun commented Jun 24, 2023

		fun buyTickets(purchaseAmount: Int): LottoTickets {
		require(purchaseAmount >= 1000) { "최소 구매 금액은 1000원 입니다." }

[Step2] 로또(자동) #759

[Step2] 로또(자동) #759

Conversation

songyi00 commented Jun 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songyi00 Jun 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songyi00 Jun 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malibinYun commented Jun 24, 2023

songyi00 Jun 25, 2023 •

edited

Loading

songyi00 Jun 25, 2023 •

edited

Loading